779013 - Alignment of mValue in ipdl generated files

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Chris Dearman]

Attachment: Align mValue union

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120310193349

Steps to reproduce:

Porting firefox to MIPS target which has strict alignment restrictions


Actual results:

The ipdl script generates a union Value to contain IPC parameters. Eg for PLayers.ipdl:

    union Value {
        char VThebesBuffer[sizeof(ThebesBuffer)];
        char Vnull_t[sizeof(null_t)];
    };

The union is sized correctly but any alignment restrictions of the component types is lost. This can lead to exceptions on architectures with strict alignment and possible performance issues on other targets.



Expected results:

The mValue union should have correct alignment based on the  component types. The patch changes the declarations from 
  char VSubType[sizeof(SubType];
to
  char VSubType[sizeof(SubType] __attribute__((aligned(__alignof__(SubType))));

The patch is a prototype and can surely be improved. It is required to reliably run firefox on MIPS

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

This patch needs review https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews either bsmedberg or smaug <bugs at pettay.fi> should be able to review it. If you have questions #developers or #mobile on irc.mozilla.org should be able to assist.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 647377 [details] [diff] [review]
Align mValue union

This patch as-is is mostly OK, but it's not going to compile on non-gcc platforms (win32).  You need to generate code that uses this helper

http://mxr.mozilla.org/mozilla-central/source/mfbt/Util.h#99

I'm clearing the review request to indicate that the approach is ok, but the patch is not ready to land as-is.  I'd like to see the next version.

Comment 3

[Chris Dearman]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)

I changed the script to use these definitions. The output now looks like:
  union Value {
    MOZ_ALIGNED_DECL(char Vnull_t[sizeof(null_t)], MOZ_ALIGNOF(null_t));
    MOZ_ALIGNED_DECL(char VStorageClone[sizeof(StorageClone)], MOZ_ALIGNOF(StorageClone));
  };

which expands to
  union Value {
    char Vnull_t[sizeof(null_t)] __attribute__((aligned(mozilla::AlignmentFinder<null_t>::alignment)));
    char VStorageClone[sizeof(StorageClone)] __attribute__((aligned(mozilla::AlignmentFinder<StorageClone>::alignment)));
  };

I tried compiling for x86 and the compiler(g++-4.4.real (Ubuntu/Linaro 4.4.4-14ubuntu5.1) 4.4.5) did not like it:

In file included from ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentParent.h:9,
                 from /opt/tamarin/firefox/src/chrome/src/nsChromeRegistryChrome.cpp:6:
../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:157: error: expected ‘)’ before ‘::’ token
../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:157: error: requested alignment is not a constant
../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:158: error: expected ‘)’ before ‘::’ token
../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:158: error: requested alignment is not a constant

A simple type like double also causes the same error - it seems like gcc cannot figure out the value of MOZ_ALIGNOF() at compile time. Looking over the rest of the codebase MOZ_ALIGNOF is used in situations where runtime evaluation can be used.

I'll upload a new version of the patch as it stands.

Thanks

Comment 4

[Chris Dearman]

Attachment: Align mValue union

This patch implements the MOZ_ALIGNED_DECL/MOZ_ALIGNOF changes.
At the moment it generates code that gcc cannot compile.
I've left in comments code to generate alignment based on a simple double type (also fails) and a constant alignment of 8 that does work

Comment 5

[Chris Dearman]

It seems that MSC has had __alignof for a while and gcc has __alignof__ so should MOZ_ALIGNOF use these builtins instead of using the current templated method?
It would probably fix the issue I ran into.

I couldn't find anything in bugzilla for this. If this seems like a reasonable change I can open a separate bug for it.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

AFAICT the intention was to make MOZ_ALIGNOF evaluate to a static constant.  I'd recommend filing a new bug to fix that problem, so we don't have to hack around it here.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In other words, if we need to work around something, we should do it in MOZ_ALIGNOF.

Comment 8

[Justin Lebar (not reading bugmail)]

> it seems like gcc cannot figure out the value of MOZ_ALIGNOF() at compile time.

I don't think this is a bug in MOZ_ALIGNOF, or at least, it's not one I know how to solve.  The problem AIUI isn't that MOZ_ALIGNOF isn't a static constant, but that GCC doesn't compute the static constant early enough.

See the comment in xpcom/glue/nsTArray.h that starts with "Declare mAutoBuf".  Is that a solution which would be workable for you here?

Comment 9

[Chris Dearman]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> In other words, if we need to work around something, we should do it in
> MOZ_ALIGNOF.

The limitations on what values can be used in gcc __attribute__((aligned())) is intentional. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10479#c4

I've created a new bug + patch to use compiler alignof keyword
https://bugzilla.mozilla.org/show_bug.cgi?id=779713

If using alignof is not acceptable I'll have to use some trick like the one in nsTArray.h - but that will force worst case alignment for all uses of mValue instead of generating the exact alignment needed for each one.

Comment 10

[Justin Lebar (not reading bugmail)]

> If using alignof is not acceptable I'll have to use some trick like the one in nsTArray.h - but that > will force worst case alignment for all uses of mValue instead of generating the exact alignment 
> needed for each one.

Could you elaborate on what exactly is the problem that you can't solve with a union?

Comment 11

[Justin Lebar (not reading bugmail)]

To be concrete: I think I understand that you want to avoid doing a nested union, as in

    union Value1 {
        union V1 {
          char VThebesBuffer[sizeof(ThebesBuffer)];
          mozilla::AlignedElem<MOZ_ALIGNOF(ThebesBuffer)>
        };
        union V2 {
          char Vnull_t[sizeof(null_t)];
          mozilla::AlignedElem<MOZ_ALIGNOF(null_t)>;
        };
    };

Because using this would require big changes to the codegen.

But I don't understand under what circumstances the nested union would be worse than

    union Value2 {
      char VThebesBuffer[sizeof(ThebesBuffer)];
      char Vnull_t[sizeof(null_t)];
      mozilla::AlignedElem<max(MOZ_ALIGNOF(ThebesBuffer), MOZ_ALIGNOF(null_t)>;
    };

Aren't Value1 and Value2 exactly equivalent in layout and alignment?  All types in the union start at the beginning of the union, so that's no different.  And alignof(Value1) == alignof(Value2), right?

Comment 12

[Chris Dearman]

(In reply to Justin Lebar [:jlebar] from comment #11)
>     union Value2 {
>       char VThebesBuffer[sizeof(ThebesBuffer)];
>       char Vnull_t[sizeof(null_t)];
>       mozilla::AlignedElem<max(MOZ_ALIGNOF(ThebesBuffer),
> MOZ_ALIGNOF(null_t)>;
>     };
> 
> Aren't Value1 and Value2 exactly equivalent in layout and alignment?  All
> types in the union start at the beginning of the union, so that's no
> different.  And alignof(Value1) == alignof(Value2), right?

I was thinking I'd end up having to include en element like 
      mozilla::AlignedElem<double>;
which would force worst case alignment regardless of the alignment requirements of 
the other components.

I've hand edited one of the files to use your suggestion.
Most of the mValue unions have more than 2 components so I've ended up with this:

    union Value { 
            char VDeviceStorageAddParams[sizeof(DeviceStorageAddParams)]; 
            char VDeviceStorageGetParams[sizeof(DeviceStorageGetParams)]; 
            char VDeviceStorageDeleteParams[sizeof(DeviceStorageDeleteParams)]; 
            char VDeviceStorageEnumerationParams[sizeof(DeviceStorageEnumerationParams)]; 
            mozilla::AlignedElem<PR_MAX(MOZ_ALIGNOF(DeviceStorageGetParams), PR_MAX(MOZ_ALIGNOF(DeviceStorageAddParams), PR_MAX(MOZ_ALIGNOF(DeviceSto\
rageDeleteParams), MOZ_ALIGNOF(DeviceStorageEnumerationParams))))> _align; 
    }; 

which is kind of ugly but probably easy enough to include in the ipdl generator; I'll try making this change

Comment 13

[Chris Dearman]

Attachment: Use mozilla::AlignedElem to force correct alignment of the union

This version of the patch uses a single mozilla::AlignedElem variable to set the alignment of the union

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 648514 [details] [diff] [review]
Use mozilla::AlignedElem to force correct alignment of the union

Just a few points of order:

Most Mozilla hackers are not set up to push git-formatted patches to hg, so would have some difficulty with this patch.  You can convert git patches to hg patches using git-patch-to-hg-patch in moz-git-tools [1].

It's also customary to attach patches with 8 lines of context.

git-bz (also in moz-git-tools) is a bit of a pain to set up, but will do all this for you.

Also, if you don't set review?someone, there's a good chance your patch will end up ignored.  (An empty review? is not sufficient.)  If you're in doubt of whom to ask for review, incorrect guesses are not looked down upon.  :)

[1] https://github.com/jlebar/git-bz-moz

Comment 15

[Chris Dearman]

Thanks for the feedback. I've installed git-bz-moz and will use it for future submissions

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 648514 [details] [diff] [review]
Use mozilla::AlignedElem to force correct alignment of the union

>diff --git a/ipc/ipdl/ipdl/cxx/ast.py b/ipc/ipdl/ipdl/cxx/ast.py
>index 90c87e9..a3b28b0 100644

>+    def visitExprMozAlignof(self, es):
>+        self.visitExprCall(es)
>+
>+    def visitExprPrmax(self, es):
>+        self.visitExprCall(es)

These don't have special semantics.  Just use ExprCall() directly.

>@@ -358,9 +364,18 @@ class TypeUnion(Node):
>         Node.__init__(self)
>         self.name = name
>         self.components = [ ]           # [ Decl ]
>+        self.alignment = None

Document the type.

>+    def addAlignment(self):
>+        self.components.append(Decl(Type('mozilla::AlignedElem', T=self.alignment), '_align'))

Nit: the local style is two leading underscores for "generated" names, '__align'.

Looks good.  I'd like to take a look at an updated patch.

Comment 17

[Chris Dearman]

Attachment: Align mValue union

Signed-off-by: Chris Dearman <[e-mail]>

Comment 18

[Chris Dearman]

Comment on attachment 649537 [details] [diff] [review]
Align mValue union

Removed helper functions as requested, documented new alignment variable and use __ prefix for generated name.

Comment 19

[Chris Dearman]

Thanks for the review. I can't commit this patch - please could you do that for me?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 649537 [details] [diff] [review]
Align mValue union

Just use checkin-needed.

Comment 21

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7158af024a9b

Not to pile on the procedural things, but in the future, please put the bug number and r=<whoever> in the commit message.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

On a more important procedural note, confirming that the patch doesn't break things by using the Try server before requesting checkin is strongly recommended.

Oh, and this was backed out for breaking Windows builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8f8fbe9aee

https://tbpl.mozilla.org/php/getParsedLog.php?id=14240702&tree=Mozilla-Inbound

ShadowLayers.cpp
c:\tools\msvs10\vc\include\vector(870) : error C2719: '_Val': formal parameter with __declspec(align('8')) won't be aligned

        e:/builds/moz2_slave/m-in-w32/build/gfx/layers/ipc/ShadowLayers.cpp(95) : see reference to class template instantiation 'std::vector<_Ty>' being compiled

        with

        [

            _Ty=mozilla::layers::Edit

        ]

Comment 23

[Justin Lebar (not reading bugmail)]

> On a more important procedural note, confirming that the patch doesn't break things by using the Try 
> server before requesting checkin is strongly recommended.

That's my fault.

Comment 24

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 25

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.