Closed
Bug 1269317
Opened 8 years ago
Closed 8 years ago
Don't use AlignedStorage2 in RegisterSets.h
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file, 1 obsolete file)
4.05 KB,
patch
|
nbp
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
AlignedStorage2 is not playing well with GCC 6 (I'll file a separate bug for that), but I noticed the uses in RegisterSets.h are unnecessary and can use a normal union. (There's a similar union that doesn't use AlignedStorage2 in BaselineCacheIR.cpp.)
Attachment #8747655 -
Flags: review?(nicolas.b.pierron)
Comment 1•8 years ago
|
||
Comment on attachment 8747655 [details] [diff] [review] Patch Review of attachment 8747655 [details] [diff] [review]: ----------------------------------------------------------------- This sounds good, but are Unrestricted Union accepted by MSVC yet[1]? [1] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
Attachment #8747655 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #1) > This sounds good, but are Unrestricted Union accepted by MSVC yet[1]? Unrestricted unions are in MSVC 2015 and we still support 2013. MSVC 2013 is happy with this union that contains Register and ValueOperand: https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/js/src/jit/BaselineCacheIR.cpp#34-47 I will test this patch locally with MSVC 2013 just to be sure.
Assignee | ||
Comment 3•8 years ago
|
||
It's good I checked because VS 2013 didn't agree with the previous patch. I had to change |AnyRegister() {}| to |AnyRegister() = default|. I also used |= default| for TypedOrValueRegister, instead of this constructor: TypedOrValueRegister() : type_(MIRType_None) {} I verified this constructor isn't used anywhere so this change shouldn't cause any problems.
Attachment #8747655 -
Attachment is obsolete: true
Attachment #8747873 -
Flags: review?(nicolas.b.pierron)
Updated•8 years ago
|
Attachment #8747873 -
Flags: review?(nicolas.b.pierron) → review+
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87c6479a51c6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8747873 [details] [diff] [review] Patch 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 tested on m-i. [Risks and why]: Low risk. Patch has been on m-c for 3 weeks. [String/UUID change made/needed]: None.
Attachment #8747873 -
Flags: approval-mozilla-esr45?
Attachment #8747873 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox48:
--- → affected
status-firefox-esr45:
--- → affected
Comment 7•8 years ago
|
||
Comment on attachment 8747873 [details] [diff] [review] Patch Taking it to simplify the life of developers and packagers
Attachment #8747873 -
Flags: approval-mozilla-esr45?
Attachment #8747873 -
Flags: approval-mozilla-esr45+
Attachment #8747873 -
Flags: approval-mozilla-aurora?
Attachment #8747873 -
Flags: approval-mozilla-aurora+
Comment 8•8 years ago
|
||
has problems to apply to aurora. Jan could you take a look, thanks! grafting 343918:87c6479a51c6 "Bug 1269317 - Don't use AlignedStorage2 in RegisterSets.h. r=nbp" merging js/src/jit/RegisterSets.h warning: conflicts while merging js/src/jit/RegisterSets.h! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4d613a6d1ca
Assignee | ||
Comment 10•8 years ago
|
||
I had to back this out on Aurora, see bug 1269319 comment 14. https://hg.mozilla.org/releases/mozilla-aurora/rev/8b7e3ca6ddb7
Flags: needinfo?(jdemooij)
You need to log in
before you can comment on or make changes to this bug.
Description
•