Closed
Bug 1269319
Opened 8 years ago
Closed 8 years ago
AlignedStorage/AlignedStorage2's default copy constructor violates strict aliasing
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
6.37 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
jandem
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
Firefox (or SpiderMonkey at least) is crashy with GCC 6, see bug 1245783. I filed a GCC bug [0] and they did fix one issue on their end, but it didn't fix the SpiderMonkey crashes completely. The GCC devs pointed out our code for AlignedStorage/AlignedStorage2 might be wrong (they're not 100% certain yet). AlignedStorage and AlignedStorage2 have a default copy constructor that will copy |Nbytes| from dest to source. Those stores might be reordered with loads that use |T*|. IIUC the char* strict aliasing rule doesn't apply, because we're using an array and not a char* pointer. The JS shell hits both the AlignedStorage and AlignedStorage2 issues. Giving AlignedStorage an explicit copy constructor that uses memcpy seems to fix part of the crashes (not sure if that's 100% correct) and I got rid of some AlignedStorage2 uses (bug 1269317) to fix the other failures, but obviously there are other consumers in the browser that might also be affected. [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70526
Assignee | ||
Comment 1•8 years ago
|
||
See also this comment: https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/mfbt/Alignment.h#107-110 * As an important side effect, pulling the storage into this template is * enough obfuscation to confuse gcc's strict-aliasing analysis into not giving * false negatives when we cast from the char buffer to whatever type we've * constructed using the bytes. It seems the default copy constructor is not a 'false negative'... I noticed Boost's aligned_storage is non-copyable, so they avoid this issue.
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #0) > AlignedStorage and AlignedStorage2 have a default copy constructor that will > copy |Nbytes| from dest to source. Er, from source to dest, obviously. Waldo, any thoughts on this?
Flags: needinfo?(jwalden+bmo)
Comment 3•8 years ago
|
||
Without keeping around a tag for the actual type of the current contents of the union, it looks like aliasing rules just hose us here, for any operation that requires knowing the type of the actual data. I think this means we should probably remove AlignedStorage{,2} and replace every use with more well-typed structures, e.g. Variant in cases where a little extra size doesn't matter. There seem to be around thirty uses -- not great, but certainly not impossible to work through and convert.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > I think this means we > should probably remove AlignedStorage{,2} and replace every use with more > well-typed structures, e.g. Variant in cases where a little extra size > doesn't matter. Variant is built on top of AlignedStorage though, AFAICS :) Well as a first step I can try to delete AlignedStorage{,2}'s copy constructors and see what happens.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 5•8 years ago
|
||
This patch deletes the default copy constructor and assignment operators. That broke 2 places, (1) RInstructionStorage in the JIT (I think the memcpy I added there is safe) and (2) js::HashTable. HashMapEntry::swap used to swap the raw bytes instead of the actual T's. I changed this - it required move assignment operators in a few places (for mozilla::Swap).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8751758 -
Flags: review?(jwalden+bmo)
Comment 6•8 years ago
|
||
Comment on attachment 8751758 [details] [diff] [review] Patch Review of attachment 8751758 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Snapshots.h @@ +506,5 @@ > > +struct RInstructionStorage > +{ > + static const size_t Size = 4 * sizeof(uint32_t); > + mozilla::AlignedStorage<Size> mem; Make this private? ::: js/src/vm/ObjectGroup.cpp @@ +1371,5 @@ > + void operator=(AllocationSiteKey&& key) { > + script = mozilla::Move(key.script); > + offset = key.offset; > + kind = key.kind; > + proto = mozilla::Move(key.proto); Put this function directly beneath the move constructor function. If you want all constructors adjacent, start with the copy constructor, then move constructor, then move assignment operator. ::: mfbt/Alignment.h @@ +123,5 @@ > + > + AlignedStorage() = default; > + > + // AlignedStorage is non-copyable: the default copy constructor violates > + // strict aliasing rules, see bug 1269319. s/see/per/ to avoid a run-on sentence (and below).
Attachment #8751758 -
Flags: review?(jwalden+bmo) → review+
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/82b8884c7ce3 https://hg.mozilla.org/mozilla-central/rev/ac5e8c0b9ec2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 10•8 years ago
|
||
I'd like to uplift bug 1269317 and bug 1269319 to Aurora and ESR45, to fix crashes with GCC 6. The coming months more people and distros will update to newer GCC versions, so it will be great to have this fixed. Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: Lots of crashes with GCC 6 and newer. [Describe test coverage new/current, TreeHerder]: Code is exercised by many tests. [Risks and why]: Low risk. Patch has been on m-c for a week. [String/UUID change made/needed]: None.
Attachment #8758206 -
Flags: review+
Attachment #8758206 -
Flags: approval-mozilla-esr45?
Attachment #8758206 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox48:
--- → affected
status-firefox-esr45:
--- → affected
Comment 11•8 years ago
|
||
Comment on attachment 8758206 [details] [diff] [review] Patch for Aurora Taking it to simplify the life of developers and packagers
Attachment #8758206 -
Flags: approval-mozilla-esr45?
Attachment #8758206 -
Flags: approval-mozilla-esr45+
Attachment #8758206 -
Flags: approval-mozilla-aurora?
Attachment #8758206 -
Flags: approval-mozilla-aurora+
Comment 12•8 years ago
|
||
holding this uplift since it depend on bug 1269317 which has uplift conflicts currently
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b73cfa00dab3
Assignee | ||
Comment 14•8 years ago
|
||
I backed this out from Aurora because it caused MSVC bustage (likely caused by VS 2013) and Android build errors (see below). I don't see a simple way to fix the Windows bustage, so it's probably best to let this ride the trains. Also, yesterday jseward pointed out that Firefox doesn't work with GCC 6 anyway until we fix bug 1232696. build/stlport/stlport/stl/_alloc.h:481:41: error: use of deleted function 'SpecialAllocator<AtForkFuncs>::SpecialAllocator(const SpecialAllocator<AtForkFuncs>&)' build/src/mozglue/build/BionicGlue.cpp:35:8: error: use of deleted function 'mozilla::AlignedStorage2<T>::AlignedStorage2(const mozilla::AlignedStorage2<T>&) [with T = AtForkFuncs]'
You need to log in
before you can comment on or make changes to this bug.
Description
•