Closed Bug 1269317 Opened 8 years ago Closed 8 years ago

Don't use AlignedStorage2 in RegisterSets.h

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 --- wontfix

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — 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 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+
(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.
Attached patch PatchSplinter Review
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)
Attachment #8747873 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/87c6479a51c6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1245783
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?
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+
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)
You need to log in before you can comment on or make changes to this bug.