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

[Julien Fischer]

On Tue, 6 Dec 2011, Peter Wang wrote:

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

Most probably.  (Win64 support for Mercury is fairly high up on my todo list,
so I guess we will find out before too long ...)

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

I'll test it with MSVC (although probably not before Thursday) and let
you know how it goes.

> 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.
> +    %

I suggest adding a sentence or two to that comment about *why* we ned to
change it.  Or at the least add a pointer to the comment in mercury_float.h.

> +:- 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),

That looks fine (modulo actually testing it with MSVC.)

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