Don't use AlignedStorage2 in RegisterSets.h

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox-esr45 wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
Posted 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+
Assignee

Comment 2

3 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

3 years ago
Posted 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+

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87c6479a51c6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Updated

3 years ago
Blocks: 1245783
Assignee

Comment 6

3 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?
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.