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

[Peter Wang]

Can someone apply this and test with MSVC and MinGW?
I've only tested the single precision floats on 64-bit Linux part.

Would it be worth avoiding the C extensions when
sizeof(MR_Float) == sizeof(MR_Word)?  That can't be evaluated by the C
preprocessor, though.

---

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.

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.

tests/hard_coded/Mercury.options:
tests/hard_coded/Mmakefile:
tests/hard_coded/bug240.exp:
tests/hard_coded/bug240.m:
	Add test case.

diff --git a/compiler/llds_out_global.m b/compiler/llds_out_global.m
index 0807291..85a253f 100644
--- a/compiler/llds_out_global.m
+++ b/compiler/llds_out_global.m
@@ -516,7 +516,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..6999ee5 100644
--- a/compiler/mlds_to_c.m
+++ b/compiler/mlds_to_c.m
@@ -1583,7 +1583,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_float.h b/runtime/mercury_float.h
index 049477f..28d5728 100644
--- a/runtime/mercury_float.h
+++ b/runtime/mercury_float.h
@@ -29,6 +29,21 @@
 #define MR_FLOAT_WORDS		((sizeof(MR_Float) + sizeof(MR_Word) - 1) \
 					/ sizeof(MR_Word))
 
+  /*
+  ** MR_Float_Aligned is a word-aligned version of MR_Float.  This is necessary
+  ** when MR_Float is single precision on a 64-bit platform to avoid packing
+  ** consecutive float fields, or when MR_Float is double precision on some
+  ** 32-bit platforms (e.g. Win32) to avoid introducing padding.
+  */
+#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_Word))) MR_Float MR_Float_Aligned;
+#else
+  /* Hope for the best. */
+  typedef MR_Float MR_Float_Aligned;
+#endif
+
 #ifdef MR_BOXED_FLOAT 
 
 #define MR_word_to_float(w)	(* (MR_Float *) (w))
diff --git a/tests/hard_coded/Mercury.options b/tests/hard_coded/Mercury.options
index 2e6cb07..28f1613 100644
--- a/tests/hard_coded/Mercury.options
+++ b/tests/hard_coded/Mercury.options
@@ -10,6 +10,7 @@ MCFLAGS-bigtest =		--intermodule-optimization -O3
 MCFLAGS-boyer =			--infer-all
 MCFLAGS-bug103 =		--optimize-constructor-last-call
 MCFLAGS-bug160 =		-w --optimize-peep-mkword
+MCFLAGS-bug240 =		-O1
 MCFLAGS-checked_nondet_tailcall	= --checked-nondet-tailcalls
 MCFLAGS-checked_nondet_tailcall_noinline = --checked-nondet-tailcalls --no-inlining
 MCFLAGS-cc_and_non_cc_test = 	--no-inlining
diff --git a/tests/hard_coded/Mmakefile b/tests/hard_coded/Mmakefile
index 3d2a2dc..07db8d3 100644
--- a/tests/hard_coded/Mmakefile
+++ b/tests/hard_coded/Mmakefile
@@ -18,6 +18,7 @@ ORDINARY_PROGS=	\
 	brace \
 	bug103 \
 	bug160 \
+	bug240 \
 	builtin_inst_rename \
 	c_write_string \
 	calendar_test \
diff --git a/tests/hard_coded/bug240.exp b/tests/hard_coded/bug240.exp
new file mode 100644
index 0000000..b858b82
--- /dev/null
+++ b/tests/hard_coded/bug240.exp
@@ -0,0 +1,6 @@
+123.0
+456.0
+789
+123.0
+456.0
+789
diff --git a/tests/hard_coded/bug240.m b/tests/hard_coded/bug240.m
new file mode 100644
index 0000000..16ea1e7
--- /dev/null
+++ b/tests/hard_coded/bug240.m
@@ -0,0 +1,49 @@
+% Regression test for bug #240 and bug #211.
+% When a structure definition has a MR_Float member the C compiler could lay
+% the structure out differently from that which is expected by the Mercury
+% compiler.
+
+:- module bug240.
+
+:- interface.
+
+:- import_module io.
+
+:- type t
+    --->    a(int)
+    ;       b(int)
+    ;       c(int)
+    ;       d(int)
+    ;       fc(float, float, int).
+
+:- pred main(io::di, io::uo) is det.
+
+:- implementation.
+
+main(!IO) :-
+    X = fc(123.0, 456.0, 789),
+    write_fc(X, !IO),
+    Z = mkfc(123.0, 456.0, 789),
+    write_fc(Z, !IO).
+
+:- func mkfc(float::in, float::in, int::in) =
+    (t::out(bound(fc(ground, ground, ground)))) is det.
+
+:- pragma no_inline(mkfc/3).
+
+mkfc(F1, F2, I) = G :-
+    G = fc(F1, F2, I).
+
+:- pred write_fc(t::in(bound(fc(ground, ground, ground))), io::di, io::uo)
+    is det.
+
+:- pragma no_inline(write_fc/3).
+
+write_fc(X, !IO) :-
+    X = fc(Y1, Y2, Y3),
+    io.write(Y1, !IO),
+    io.nl(!IO),
+    io.write(Y2, !IO),
+    io.nl(!IO),
+    io.write(Y3, !IO),
+    io.nl(!IO).

--------------------------------------------------------------------------
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
--------------------------------------------------------------------------