Closed Bug 1445089 Opened 7 years ago Closed 2 years ago

Revert workaround for a VS2017 <15.6 constexpr pointer math bug

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox61 --- wontfix
firefox106 --- fixed

People

(Reporter: emk, Assigned: RyanVM)

References

Details

Attachments

(2 files)

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

Unassigning bugs assigned to Bryce because he no longer works at Mozilla.

Assignee: brycebugemail → nobody

Given that we don't support MSVC at all anymore, there should be nothing from blocking this. And Try agrees:
https://treeherder.mozilla.org/jobs?repo=try&revision=eb88424469b54bbff3504ffbd5e3c7017e378dac

Assignee: nobody → ryanvm

Bug 1408695 introduced a workaround in HTMLTrackElement for a bug in VS2017 <
15.6. We don't support MSVC at all anymore, so this issue should be moot even
if it was fixed upstream since.

Pushed by rvandermeulen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8ca81485a950 Revert workaround for a VS2017 <15.6 constexpr pointer math bug. r=alwu
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: