Open Bug 779013 Opened 8 years ago Updated 2 years ago

Alignment of mValue in ipdl generated files

Categories

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

Other
Android
defect

Tracking

()

ASSIGNED

People

(Reporter: chris, Assigned: chris)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Align mValue union (obsolete) — Splinter Review
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
OS: Linux → Android
Hardware: x86_64 → Other
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #647377 - Flags: review?(jones.chris.g)
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.
Attachment #647377 - Flags: review?(jones.chris.g)
(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
Attached patch Align mValue union (obsolete) — Splinter Review
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
Attachment #647456 - Flags: review?(jones.chris.g)
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.
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.
In other words, if we need to work around something, we should do it in MOZ_ALIGNOF.
> 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?
(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.
> 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?
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?
(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
This version of the patch uses a single mozilla::AlignedElem variable to set the alignment of the union
Attachment #647377 - Attachment is obsolete: true
Attachment #647456 - Attachment is obsolete: true
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
Attachment #648514 - Flags: review?(jones.chris.g)
Attachment #648514 - Flags: feedback+
Thanks for the feedback. I've installed git-bz-moz and will use it for future submissions
Assignee: nobody → chris
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.
Attachment #648514 - Flags: review?(jones.chris.g)
Signed-off-by: Chris Dearman <chris@mips.com>
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.
Attachment #649537 - Flags: review?(chris)
Attachment #649537 - Flags: review?(chris) → review?(jones.chris.g)
Attachment #649537 - Flags: review?(jones.chris.g) → review+
Thanks for the review. I can't commit this patch - please could you do that for me?
Status: NEW → ASSIGNED
Attachment #649537 - Flags: checkin?(jones.chris.g)
Attachment #648514 - Attachment is obsolete: true
Comment on attachment 649537 [details] [diff] [review]
Align mValue union

Just use checkin-needed.
Attachment #649537 - Flags: checkin?(jones.chris.g)
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.
Keywords: checkin-needed
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

        ]
> 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.
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.