[m-rev.] for review: fix float field structure padding/packing problems

[Peter Wang]

On Tue, 6 Dec 2011 10:32:19 +1100, Peter Wang <novalazy at gmail.com> wrote:
> On 2011-12-06, Julien Fischer <juliensf at csse.unimelb.edu.au> wrote:
> > 
> > By itself the above does not work with MSVC.  We also need to pass the -Zp4
> > as a flag to cl, then the test case works.
> 
> Thanks.  It looks like __declspec(align) didn't do what I thought
> and is actually unnecessary here.

On second thought, it would probably be necessary for single precision
floats on Win64.

The following is again untested on MSVC.  Tested with clang.

---

Branches: main

Fix bug #240 and bug #211.  When MR_Float is wider than MR_Word, the C compiler
may introduce padding for the structure field following the MR_Float.  When
MR_Float is narrower than MR_Word the C compiler may pack consecutive MR_Float
fields.  In both cases the C structure layout does not match what was intended
by the Mercury compiler.

A test case was committed earlier.

runtime/mercury_float.h:
	Add a typedef `MR_Float_Aligned' which forces word alignment using
	C extensions provided by gcc/clang/MSVC.

compiler/llds_out_global.m:
compiler/mlds_to_c.m:
	Use `MR_Float_Aligned' in place of `MR_Float' in structure definitions.

	Output #pragma pack directives around scalar cell group structures.
	This is for MSVC.

compiler/c_util.m:
	Add helper predicates to output #pragma pack directives for MSVC.

runtime/mercury_conf.h.in:
	Define MR_BYTES_PER_WORD.  The necessary code in configure.in already
	exists.

diff --git a/compiler/c_util.m b/compiler/c_util.m
index a8d2bb8..087e5fc 100644
--- a/compiler/c_util.m
+++ b/compiler/c_util.m
@@ -168,6 +168,15 @@
     io::di, io::uo) is det.
 
 %-----------------------------------------------------------------------------%
+
+    % Output #pragma pack directives to change the packing alignment value
+    % for MSVC.
+    %
+:- pred output_pragma_pack_push(io::di, io::uo) is det.
+
+:- pred output_pragma_pack_pop(io::di, io::uo) is det.
+
+%-----------------------------------------------------------------------------%
 %
 % Utility predicates for working with C code
 %
@@ -631,6 +640,24 @@ convert_bool_to_string(yes) = "yes".
 
 %-----------------------------------------------------------------------------%
 
+    % We could hide these blocks behind macros using the __pragma keyword
+    % introduced in MSVC 9 (2008):
+    %
+    %   #define MR_PRAGMA_PACK_PUSH  __pragma(pack(push, MR_BYTES_PER_WORD))
+    %   #define MR_PRAGMA_PACK_POP   __pragma(pack(pop))
+    %
+output_pragma_pack_push(!IO) :-
+    io.write_string("\n#ifdef MR_MSVC\n", !IO),
+    io.write_string("#pragma pack(push, MR_BYTES_PER_WORD)\n", !IO),
+    io.write_string("#endif\n", !IO).
+
+output_pragma_pack_pop(!IO) :-
+    io.write_string("#ifdef MR_MSVC\n", !IO),
+    io.write_string("#pragma pack(pop)\n", !IO),
+    io.write_string("#endif\n", !IO).
+
+%-----------------------------------------------------------------------------%
+
 is_valid_c_identifier(S) :-
     string.index(S, 0, Start),
     char.is_alpha_or_underscore(Start),
diff --git a/compiler/llds_out_global.m b/compiler/llds_out_global.m
index 0807291..cc1b092 100644
--- a/compiler/llds_out_global.m
+++ b/compiler/llds_out_global.m
@@ -416,6 +416,7 @@ output_common_type_defn(TypeNum, CellType, !DeclSet, !IO) :-
     ( decl_set_is_member(TypeDeclId, !.DeclSet) ->
         true
     ;
+        output_pragma_pack_push(!IO),
         io.write_string("struct ", !IO),
         output_common_cell_type_name(TypeNum, !IO),
         io.write_string(" {\n", !IO),
@@ -427,6 +428,7 @@ output_common_type_defn(TypeNum, CellType, !DeclSet, !IO) :-
             output_cons_arg_group_types(ArgGroups, "\t", 1, !IO)
         ),
         io.write_string("};\n", !IO),
+        output_pragma_pack_pop(!IO),
         decl_set_insert(TypeDeclId, !DeclSet)
     ).
 
@@ -516,7 +518,12 @@ common_group_get_rvals(common_cell_ungrouped_arg(_, Rval)) = [Rval].
 output_cons_arg_types([], _, _, !IO).
 output_cons_arg_types([Type | Types], Indent, ArgNum, !IO) :-
     io.write_string(Indent, !IO),
-    output_llds_type(Type, !IO),
+    ( Type = lt_float ->
+        % Ensure float structure members are word-aligned.
+        io.write_string("MR_Float_Aligned", !IO)
+    ;
+        output_llds_type(Type, !IO)
+    ),
     io.write_string(" f", !IO),
     io.write_int(ArgNum, !IO),
     io.write_string(";\n", !IO),
diff --git a/compiler/mlds_to_c.m b/compiler/mlds_to_c.m
index e1add26..8e1bed0 100644
--- a/compiler/mlds_to_c.m
+++ b/compiler/mlds_to_c.m
@@ -1569,13 +1569,14 @@ mlds_output_scalar_cell_group_decl(Opts, Indent, MangledModuleName,
 
 mlds_output_scalar_cell_group_struct_defn(Opts, Indent, MangledModuleName,
         TypeRawNum, ElemTypes, !IO) :-
-    mlds_indent(Indent, !IO),
-    io.format("\nstruct %s_scalar_cell_group_%d {\n",
+    output_pragma_pack_push(!IO),
+    io.format("struct %s_scalar_cell_group_%d {\n",
         [s(MangledModuleName), i(TypeRawNum)], !IO),
     list.foldl2(mlds_output_scalar_cell_group_struct_field(Opts, Indent + 1),
         ElemTypes, 1, _, !IO),
     mlds_indent(Indent, !IO),
-    io.write_string("};\n", !IO).
+    io.write_string("};\n", !IO),
+    output_pragma_pack_pop(!IO).
 
 :- pred mlds_output_scalar_cell_group_struct_field(mlds_to_c_opts::in,
     indent::in, mlds_type::in, int::in, int::out, io::di, io::uo) is det.
@@ -1583,7 +1584,12 @@ mlds_output_scalar_cell_group_struct_defn(Opts, Indent, MangledModuleName,
 mlds_output_scalar_cell_group_struct_field(Opts, Indent, FieldType,
         Num, Num + 1, !IO) :-
     mlds_indent(Indent, !IO),
-    mlds_output_type_prefix(Opts, FieldType, !IO),
+    ( FieldType = mlds_native_float_type ->
+        % Ensure float structure members are word-aligned.
+        io.write_string("MR_Float_Aligned", !IO)
+    ;
+        mlds_output_type_prefix(Opts, FieldType, !IO)
+    ),
     io.format(" f%d;\n", [i(Num)], !IO).
 
 :- pred mlds_output_scalar_cell_group_type_and_name(mlds_to_c_opts::in,
diff --git a/runtime/mercury_conf.h.in b/runtime/mercury_conf.h.in
index 2707687..1fc14e1 100644
--- a/runtime/mercury_conf.h.in
+++ b/runtime/mercury_conf.h.in
@@ -48,6 +48,11 @@
 */
 #undef	MR_WORD_TYPE
 
+/*
+** MR_BYTES_PER_WORD: the number of bytes per MR_WORD_TYPE.
+*/
+#undef	MR_BYTES_PER_WORD
+
 /* 
 ** MR_INTEGER_LENGTH_MODIFIER: the printf() length modifier for a MR_Integer.
 */
diff --git a/runtime/mercury_float.h b/runtime/mercury_float.h
index 049477f..d600045 100644
--- a/runtime/mercury_float.h
+++ b/runtime/mercury_float.h
@@ -29,6 +29,23 @@
 #define MR_FLOAT_WORDS		((sizeof(MR_Float) + sizeof(MR_Word) - 1) \
 					/ sizeof(MR_Word))
 
+  /*
+  ** MR_Float_Aligned and #pragma pack are used convince the C compiler to lay
+  ** out structures containing MR_Float members as expected by the Mercury
+  ** compiler, without additional padding or packing.
+  **
+  ** For MSVC, __declspec(align) only increases alignment, e.g. for single
+  ** precision floats on 64-bit platforms. #pragma pack is required to
+  ** reduce packing size, e.g. double precision floats on 32-bit platform.
+  */
+#if defined(MR_GNUC) || defined(MR_CLANG)
+  typedef MR_Float MR_Float_Aligned __attribute__((aligned(sizeof(MR_Word))));
+#elif defined(MR_MSVC)
+  typedef __declspec(align(sizeof(MR_BYTES_PER_WORD))) MR_Float MR_Float_Aligned;
+#else
+  typedef MR_Float MR_Float_Aligned;
+#endif
+
 #ifdef MR_BOXED_FLOAT 
 
 #define MR_word_to_float(w)	(* (MR_Float *) (w))


--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------