Alignment bug in ipc/ipdl/LayersMessages.cpp: AnimationData::Value is not properly aligned

RESOLVED DUPLICATE of bug 1201329

Status

()

RESOLVED DUPLICATE of bug 1201329
5 years ago
3 years ago

People

(Reporter: martin, Unassigned)

Tracking

30 Branch
Sun
NetBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
On alignement critical 64bit architectures like sparc 64 firefox 30 crashes like this:

#0  TransformData (this=0xffffffffffff8bbc)
    at /usr/pkgobj/www/firefox/work/build/ipc/ipdl/LayersMessages.cpp:2031
#1  mozilla::layers::AnimationData::operator= (this=this@entry=0xffffffffffff8bbc, aRhs=...)
    at /usr/pkgobj/www/firefox/work/build/ipc/ipdl/LayersMessages.cpp:2176
#2  0xfffffffffcf45f68 in nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer (
    aLayer=aLayer@entry=0xffffffffe4326400, aBuilder=aBuilder@entry=0xffffffffffff9c30, 
    aItem=aItem@entry=0xffffffffe040bd98, aFrame=0xffffffffe5ef83a0, 
    aProperty=aProperty@entry=eCSSProperty_transform)
    at /usr/pkgobj/www/firefox/work/mozilla-release/layout/base/nsDisplayList.cpp:458
#3  0xfffffffffcf46378 in nsDisplayTransform::BuildLayer (this=0xffffffffe040bd98, 
    aBuilder=0xffffffffffff9c30, aManager=0xffffffffe5341160, aContainerParameters=...)
    at /usr/pkgobj/www/firefox/work/mozilla-release/layout/base/nsDisplayList.cpp:4755
#4  0xfffffffffcef296c in mozilla::ContainerState::ProcessDisplayItems (this=this@entry=0xffffffffffff9190, 
    aList=..., aFlags=aFlags@entry=0)
    at /usr/pkgobj/www/firefox/work/mozilla-release/layout/base/FrameLayerBuilder.cpp:2479

Note that this=0xffffffffffff8bbc is not "good enough" aligned for TransformData on sparc64.

This happens because the declaration does actually not guarantee any alignement at all:

    union Value {
        char Vnull_t[sizeof(null_t)];
        char VTransformData[sizeof(TransformData)];
    };

but this is used to create l-values:

    ptr_TransformData()
    {
        return reinterpret_cast<TransformData*>((&((mValue).VTransformData)));
    }

Obviously this only works by chance.

I do not understand why VTransformData is not just declared as TransformData (which should be an easy fix). If that is not possible, next best idea I would come up with is some __attribute__(__algined__(alignof(TransformData))) magic - but I guess I am missing something here.

Updated

5 years ago
Component: IPC → Graphics: Layers

Comment 1

5 years ago
Oops, not a graphics bug, this is a bug in the IPDL transform.

I really don't know why we're using char arrays there.
Component: Graphics: Layers → IPC
Summary: Alignement bug in ipc/ipdl/LayersMessages.cpp: AnimationData::Value is not properly aligned → Alignment bug in ipc/ipdl/LayersMessages.cpp: AnimationData::Value is not properly aligned
(Reporter)

Comment 2

5 years ago
It comes from lower.py:

    def unionType(self):
        """Type used for storage in generated C union decl."""                                                                                    
        if self.recursive:
            return self.ptrToType()
        else:
            return TypeArray(Type('char'), ExprSizeof(self.internalType()))
(Reporter)

Comment 3

5 years ago
We can not use the straight type, as the union sometimes includes complex types with constructors:

build/ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:2759:19: error: union member 'mozilla::dom::PrefValue::Value::VnsCString' with non-trivial 'nsCString::~nsCString()'
         nsCString VnsCString;
                   ^
(Reporter)

Comment 4

5 years ago
Created attachment 8441447 [details] [diff] [review]
proof of concept/test patch: add alignement attributes to all union members

This is not a real fix, of course (but I will be able to quickly test a build with it).

In case this way should be chosen, we should define a preprocessor symbol with the needed #ifdef dance and let cgen.py output the macro (which then may expand to empty)

Like:

#ifdef (this compiler supports it)
#define TYPEARRALIGN(N) __attribute__((__aligned__(__alignof__(N))))
#else
#define TYPEARRALIGN(N)
#endif
(Reporter)

Comment 5

5 years ago
Created attachment 8441687 [details] [diff] [review]
proof of concept/test patch

Does not make sense to force "char" alignment for everything - try again with the proper type
Attachment #8441447 - Attachment is obsolete: true
(Reporter)

Comment 6

5 years ago
The patch makes it work - now for cosmetics.
(Reporter)

Comment 7

5 years ago
Created attachment 8441972 [details] [diff] [review]
Generate unions with proper alignement info

This is less ugly and still works. Generated code looks like:

    union Value {
        char VPerspective[sizeof(Perspective)] IPDLDECLALIGN(Perspective);
        char VRotationX[sizeof(RotationX)] IPDLDECLALIGN(RotationX);
        char VRotationY[sizeof(RotationY)] IPDLDECLALIGN(RotationY);
        char VRotationZ[sizeof(RotationZ)] IPDLDECLALIGN(RotationZ);
        char VRotation[sizeof(Rotation)] IPDLDECLALIGN(Rotation);
        char VRotation3D[sizeof(Rotation3D)] IPDLDECLALIGN(Rotation3D);
        char VScale[sizeof(Scale)] IPDLDECLALIGN(Scale);
        char VSkew[sizeof(Skew)] IPDLDECLALIGN(Skew);
        char VSkewX[sizeof(SkewX)] IPDLDECLALIGN(SkewX);
        char VSkewY[sizeof(SkewY)] IPDLDECLALIGN(SkewY);
        char VTranslation[sizeof(Translation)] IPDLDECLALIGN(Translation);
        char VTransformMatrix[sizeof(TransformMatrix)] IPDLDECLALIGN(TransformMatrix);
    };

The name of the macro is just a suggestion. I have no idea what other compilers would need to be excluded (because they do not support the alignment attribute).
Attachment #8441687 - Attachment is obsolete: true
Attachment #8441972 - Flags: review?(benjamin)

Comment 8

5 years ago
Comment on attachment 8441972 [details] [diff] [review]
Generate unions with proper alignement info

This should use MOZ_ALIGNOF and MOZ_ALIGNED_DECL, which have already figured out the alignment macrology. But in general the approach looks correct.
Attachment #8441972 - Flags: review?(benjamin) → review-
(Reporter)

Comment 9

5 years ago
Unfortunately this does not work:

A declaration like:

    union Value {
        MOZ_ALIGNED_DECL(char VNormalBlobConstructorParams[sizeof(NormalBlobConstructorParams)], MOZ_ALIGNOF(NormalBlobConstructorParams));
        MOZ_ALIGNED_DECL(char VFileBlobConstructorParams[sizeof(FileBlobConstructorParams)], MOZ_ALIGNOF(FileBlobConstructorParams));
    };

causes this error from gcc:

.../build/ipc/ipdl/_ipdlheaders/mozilla/dom/PBlob.h:52:9: note: in expansion of macro 'MOZ_ALIGNED_DECL'
         MOZ_ALIGNED_DECL(char VNormalBlobConstructorParams[sizeof(NormalBlobConstructorParams)], MOZ_ALIGNOF(NormalBlobConstructorParams));
         ^
Is that a preprocessor error or a compiler error? It seems to be missing important details. jlebar wrote most of this originally, and although MOZ_ALIGNOF is used in the tree, MOZ_ALIGNED_DECL isn't and I wonder if it's just wrong.
(Reporter)

Comment 11

5 years ago
gcc seems to be picky about the supplied alignement expression.
This works:

    union Value {
        MOZ_ALIGNED_DECL(char VCubicBezierFunction[sizeof(CubicBezierFunction)], 8);
        MOZ_ALIGNED_DECL(char VStepFunction[sizeof(StepFunction)], 8);
    };

But using MOZ_ALIGNOF(type) instead of the constant int does not.

See also: mfbt/Alignemnt.h:

/*
 * We have to specialize this template because GCC doesn't like __attribute__((aligned(foo))) where
 * foo is a template parameter.
 */

template<>
struct AlignedElem<1>
{
    MOZ_ALIGNED_DECL(uint8_t elem, 1);
};

(and the type arg of MOZ_ALIGNOF is used as a  template parameter for AlignemntFinder<T>)
Interesting. Does MOZ_ALIGNED_DECL(char VCubicBezierFunction[sizeof(CubicBezierFunction)], __alignof__(CubicBezierFunction)) work?

Perhaps for GCC we can replace the current implementation of MOZ_ALIGNOF with __alignof__ (get rid of the AlignmentFinder indirection).
(Reporter)

Comment 13

5 years ago
Yes, with __alignof__() it works - I'll test a build with MOZ_ALIGNOF based on that for gcc.
(Reporter)

Comment 14

5 years ago
Created attachment 8442168 [details] [diff] [review]
Make MOZ_ALIGNOF use __aligneof__ with gcc, use MOZ_ALIGNED_DECL to declare union members in ipdl value declarations

This is the patch I'm testing next, will take a while.
Attachment #8441972 - Attachment is obsolete: true
(Reporter)

Comment 15

5 years ago
Build completed and result works
(Reporter)

Updated

5 years ago
Attachment #8442168 - Flags: review?(benjamin)
Comment on attachment 8442168 [details] [diff] [review]
Make MOZ_ALIGNOF use __aligneof__ with gcc, use MOZ_ALIGNED_DECL to declare union members in ipdl value declarations

Excellent, thank you!
Attachment #8442168 - Flags: review?(benjamin) → review+

Comment 17

5 years ago
Comment on attachment 8442168 [details] [diff] [review]
Make MOZ_ALIGNOF use __aligneof__ with gcc, use MOZ_ALIGNED_DECL to declare union members in ipdl value declarations

Review of attachment 8442168 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Alignment.h.orig
@@ +14,5 @@
>  
>  namespace mozilla {
>  
> +#if defined(__GNUC__)
> +#define MOZ_ALIGNOF(T) __alignof__(T)

VS2013 also supports C++11 alignof/alignas keywords. I wonder if doing something similar to mfbt/Atomics.h would be better.

https://tbpl.mozilla.org/?tree=Try&rev=2e778017bff4

Comment 18

5 years ago
Try results indicate there're 2 issues:

(1) some directories build without -std=c++11 breaking Linux (alignof/alignas)
(2) ipc/ipdl change breaks Windows (not sure why), confirmed by second try
    https://tbpl.mozilla.org/?tree=Try&rev=01d1abc23496
The windows bustage looks like it might just be that we aren't including mozilla/Alignment.h in that translation unit.

Comment 20

5 years ago
Added to HeaderIncludes but doesn't help.

https://tbpl.mozilla.org/?tree=Try&rev=d0ced40e5c26

Comment 21

5 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Perhaps for GCC we can replace the current implementation of MOZ_ALIGNOF
> with __alignof__ (get rid of the AlignmentFinder indirection).

Bug 779713 tried to use __alignof__ but was rejected. I wonder if the reasoning there still applies.

Comment 22

3 years ago
Martin, can you confirm comment 0 still happens on Firefox 43 even after bug 1201329? If not I think this bug should be closed as a DUPLICATE or FIXED.
Flags: needinfo?(martin)
(Reporter)

Comment 23

3 years ago
(In reply to Jan Beich from comment #22)
> Martin, can you confirm comment 0 still happens on Firefox 43 even after bug
> 1201329? If not I think this bug should be closed as a DUPLICATE or FIXED.

Yes, review looks like that is the same bug and the fix is better. I also tested FF 43 on sparc64 w/o the local patch from this bug and could not reproduce the alignment issues (at least in casual testing)
Flags: needinfo?(martin)
(Reporter)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1201329
You need to log in before you can comment on or make changes to this bug.