Revert workaround for a VS2017 <15.6 constexpr pointer math bug

NEW
Assigned to

Status

()

enhancement
P3
normal
Last year
Last year

People

(Reporter: emk, Assigned: bryce)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 affected)

Details

Attachments

(1 attachment)

We are going to require VS 2017 15.6.
Bryce, do you mind reverting the original patch once bug 1424281 lands?
Assignee: nobody → bvandyk
Priority: -- → P2
Sure thing. Looks like that's just happened, so I'll throw this on my immediate queue and try have it sorted in the next few days.
Comment on attachment 8959213 [details]
Bug 1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug.

https://reviewboard.mozilla.org/r/228096/#review234002

I don't expect any problems, but you may want to wait a few more days for 15.6 to stick, just to avoid tempting fate.
Attachment #8959213 - Flags: review?(dmajor) → review+
Comment on attachment 8959213 [details]
Bug 1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug.

https://reviewboard.mozilla.org/r/228096/#review234002

Appreciate the headsup. Will let it bake, have set a reminder for next Tuesday for landing.
Component: Audio/Video → Audio/Video: Playback
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92532463d99c
Revert workaround for a VS2017 <15.6 constexpr pointer math bug. r=dmajor
Original bug mentioned mochitests should pick this up, but it looks like it's the web platform tests which catch this.

Looking more it seems MSVC mainline releases are not yet handling this. Running the test case from the original bug and building central I'm seeing the same issues, tested with:

- 15.6.2 with cl version 19.13.26128
- 15.6.3 with cl version 19.13.26129

Since we're supporting 19.13.26128 in tree I think we're going to have to park this until we get another bump before we can try this again.
Flags: needinfo?(bvandyk)
Honestly I think we've already put in more effort than this deserves. The constexpr->const reduction on this single tiny array is such a miniscule inefficency, we could leave it that way forever and not a single cpu would ever notice.
MS claims that this bug was fixed since VS2017 15.6 Preview 4 (a.k.a. 19.13.26125.1):
https://developercommunity.visualstudio.com/content/problem/60059/code-generation-problem-with-constexpr.html
We should file another bug report if this is not fixed yet. Or fix bug 1443590.

(In reply to David Major [:dmajor] from comment #11)
> Honestly I think we've already put in more effort than this deserves. The
> constexpr->const reduction on this single tiny array is such a miniscule
> inefficency, we could leave it that way forever and not a single cpu would
> ever notice.

Although this workaround is negligible, this bug might affect some more complex use case such as bug 1411469.
I'll hold onto this and look into it some more when I have a moment. If it turns out MS have fixed the other cases but not ours I can file a bug with them and update this bug. This doesn't seem to be causing us any pain if it stays as is for some time longer though.
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.