Closed Bug 1026499 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: IPC, defect)

30 Branch
Sun
NetBSD
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1201329

People

(Reporter: martin, Unassigned)

Details

Attachments

(1 file, 3 obsolete files)

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.
Component: IPC → Graphics: Layers
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
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()))
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; ^
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
Attached patch proof of concept/test patch (obsolete) — Splinter Review
Does not make sense to force "char" alignment for everything - try again with the proper type
Attachment #8441447 - Attachment is obsolete: true
The patch makes it work - now for cosmetics.
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 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-
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.
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).
Yes, with __alignof__() it works - I'll test a build with MOZ_ALIGNOF based on that for gcc.
This is the patch I'm testing next, will take a while.
Attachment #8441972 - Attachment is obsolete: true
Build completed and result works
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 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
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.
Added to HeaderIncludes but doesn't help. https://tbpl.mozilla.org/?tree=Try&rev=d0ced40e5c26
(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.
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)
(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)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: