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)
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.
Updated•10 years ago
|
Component: IPC → Graphics: Layers
Comment 1•10 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•10 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•10 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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
The patch makes it work - now for cosmetics.
Reporter | ||
Comment 7•10 years ago
|
||
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•10 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•10 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));
^
Comment 10•10 years ago
|
||
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•10 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>)
Comment 12•10 years ago
|
||
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•10 years ago
|
||
Yes, with __alignof__() it works - I'll test a build with MOZ_ALIGNOF based on that for gcc.
Reporter | ||
Comment 14•10 years ago
|
||
This is the patch I'm testing next, will take a while.
Attachment #8441972 -
Attachment is obsolete: true
Reporter | ||
Comment 15•10 years ago
|
||
Build completed and result works
Reporter | ||
Updated•10 years ago
|
Attachment #8442168 -
Flags: review?(benjamin)
Comment 16•10 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
Excellent, thank you!
Attachment #8442168 -
Flags: review?(benjamin) → review+
Comment 17•10 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•10 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
Comment 19•10 years ago
|
||
The windows bustage looks like it might just be that we aren't including mozilla/Alignment.h in that translation unit.
Comment 20•10 years ago
|
||
Added to HeaderIncludes but doesn't help.
https://tbpl.mozilla.org/?tree=Try&rev=d0ced40e5c26
Comment 21•10 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•9 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•9 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•9 years ago
|
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.
Description
•