for review: fix for linkage mismatch bug

[Fergus Henderson]

Tyson, could you please review this one?

--------------------

Estimated hours taken: 1

Ensure that the declarations and definitions for data constants
specify the same linkage (extern or static), because the C standard
says that if the linkage is different, then the behaviour is undefined.
This bug resulted in undefined symbol link errors for the RS/6000 AIX port.

compiler/llds_out.m:
	When printing out declarations for data constants,
	compute the linkage from the data_name, rather than
	always assuming `extern'.  When printing out definitions,
	add a sanity check to ensure that the linkage that would be
	computed from the data_name is the same as the linkage used
	for the definition.

compiler/base_type_layout.m:
	Make base_type_functors and base_type_info structures
	always local to the module, rather than exported.
	This is necessary to ensure that the linkage can be
	computed from the data_name.

compiler/base_typeclass_info.m:
compiler/llds.m:
compiler/stack_layout.m:
	Add comments pointing to the new predicate linkage/2 in
	llds_out.m, to ensure that future maintenance doesn't break
	the assumptions it makes.

Index: compiler/base_type_layout.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/base_type_layout.m,v
retrieving revision 1.32
diff -u -r1.32 base_type_layout.m
--- base_type_layout.m	1998/08/04 02:13:44	1.32
+++ base_type_layout.m	1998/09/03 08:08:35
@@ -346,7 +346,7 @@
 base_type_layout__construct_base_type_data([BaseGenInfo | BaseGenInfos],
 		LayoutInfo0, LayoutInfo) :-
 	BaseGenInfo = base_gen_layout(TypeId, ModuleName, TypeName, TypeArity,
-		Status, HldsType),
+		_Status, HldsType),
 	base_type_layout__set_type_id(LayoutInfo0, TypeId, LayoutInfo1),
 	hlds_data__get_type_defn_body(HldsType, TypeBody),
 	(
@@ -401,13 +401,16 @@
 			)
 		)
 	),	
-	(
-		( Status = exported ; Status = abstract_exported )
-	->
-		Exported = yes
-	;
-		Exported = no
-	),
+
+	%
+	% Note: base_type_layouts and base_type_functors are never exported,
+	% because they should only be accessed via the base_type_info in
+	% the same module.
+	% Accesses to the base_type_layout for a type exported from a
+	% different module should be done via that type's base_type_info,
+	% which will be exported if the type was exported/abstract_exported.
+	%
+	Exported = no,
 
 		% pure abstract types have no layout definition.
 	( 
Index: compiler/base_typeclass_info.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/base_typeclass_info.m,v
retrieving revision 1.4
diff -u -r1.4 base_typeclass_info.m
--- base_typeclass_info.m	1998/03/03 17:33:34	1.4
+++ base_typeclass_info.m	1998/09/03 07:51:17
@@ -106,6 +106,8 @@
 		Rvals = Rvals0,
 
 			% XXX Need we always export it from the module?
+			% (Note that linkage/2 in llds_out.m assumes
+			% that we do.)
 		Status = yes,
 
 		CModule = c_data(ModuleName, DataName, Status, Rvals, Procs),
Index: compiler/llds.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/llds.m,v
retrieving revision 1.227
diff -u -r1.227 llds.m
--- llds.m	1998/07/29 08:53:14	1.227
+++ llds.m	1998/09/03 07:52:22
@@ -84,6 +84,9 @@
 						% qualified with the basename.
 			bool,			% Should this item be exported
 						% from this Mercury module?
+						% XXX Actually this field is
+						% redundant; see linkage/2
+						% in llds_out.m.
 			list(maybe(rval)),	% The arguments of the create.
 			list(pred_proc_id)	% The procedures referenced.
 						% Used by dead_proc_elim.
Index: compiler/llds_out.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/llds_out.m,v
retrieving revision 1.87
diff -u -r1.87 llds_out.m
--- llds_out.m	1998/07/29 08:53:20	1.87
+++ llds_out.m	1998/09/03 08:07:41
@@ -484,6 +484,24 @@
 	io__write_string("\n"),
 	{ DataAddr = data_addr(data_addr(ModuleName, VarName)) },
 	output_cons_arg_decls(ArgVals, "", "", 0, _, DeclSet0, DeclSet1),
+
+	%
+	% sanity check: check that the (redundant) ExportedFromModule field
+	% in the c_data, which we use for the definition, matches the linkage
+	% computed by linkage/2 from the dataname, which we use for any
+	% prior declarations.
+	%
+	{ linkage(VarName, Linkage) },
+	{
+		( Linkage = extern, ExportedFromModule = yes
+		; Linkage = static, ExportedFromModule = no
+		)
+	->
+		true
+	;
+		error("linkage mismatch")
+	},
+	
 		% The code for data local to a Mercury module
 		% should normally be visible only within the C file
 		% generated for that module. However, if we generate
@@ -2132,7 +2150,26 @@
 		FirstIndent, LaterIndent, N0, N) -->
 	output_indent(FirstIndent, LaterIndent, N0),
 	{ N is N0 + 1 },
-	io__write_string("extern "),
+
+	%
+	% Previously we used to always write `extern' here, but
+	% declaring something `extern' and then later defining it as
+	% `static' causes undefined behavior -- on many systems, it
+	% works, but on some systems such as RS/6000s running AIX
+	% it results in link errors.
+	%
+	{ linkage(VarName, Linkage) },
+	globals__io_lookup_bool_option(split_c_files, SplitFiles),
+	(
+		( { Linkage = extern }
+		; { SplitFiles = yes }
+		)
+	->
+		io__write_string("extern ")
+	;
+		io__write_string("static ")
+	),
+
 	globals__io_get_globals(Globals),
 
 		% Don't make decls of base_type_infos `const' if we
@@ -2152,6 +2189,27 @@
 	io__write_string("\t"),
 	output_data_addr(ModuleName, VarName), 
 	io__write_string(";\n").
+
+%
+% Note that we need to know the linkage not just at the definition,
+% but also at every use, because if the use is prior to the definition,
+% then we need to declare the name first, and the linkage used in that
+% declaration must be consistent with the linkage in the definition.
+% For this reason, the field in c_data (which holds the information about
+% the definition) which says whether or not a data name is exported
+% is not useful.  Instead, we need to determine whether or not something
+% is exported from its `data_name'.
+%
+
+:- type linkage ---> extern ; static.
+
+:- pred linkage(data_name::in, linkage::out) is det.
+linkage(base_typeclass_info(_, _), extern).
+linkage(base_type(info, _, _),     extern).
+linkage(base_type(layout, _, _),   static).
+linkage(base_type(functors, _, _), static).
+linkage(common(_),                 static).
+linkage(stack_layout(_),           static).
 
 %-----------------------------------------------------------------------------%
 
Index: compiler/stack_layout.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/stack_layout.m,v
retrieving revision 1.17
diff -u -r1.17 stack_layout.m
--- stack_layout.m	1998/07/20 10:01:26	1.17
+++ stack_layout.m	1998/09/03 07:53:52
@@ -317,6 +317,8 @@
 	{ Exported = no },	% XXX With the new profiler, we will need to
 				% set this to `yes' if the profiling option
 				% is given and if the procedure is exported.
+				% Beware however that linkage/2 in llds_out.m
+				% assumes that this is `no'.
 	{ CModule = c_data(ModuleName, stack_layout(EntryLabel), Exported,
 		MaybeRvals, []) },
 	stack_layout__add_cmodule(CModule, EntryLabel).

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.