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)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
106 Branch
People
(Reporter: emk, Assigned: RyanVM)
References
Details
Attachments
(2 files)
We are going to require VS 2017 15.6.
Comment 1•7 years ago
|
||
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 hidden (mozreview-request) |
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 6•7 years ago
|
||
mozreview-review-reply |
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.
Updated•7 years ago
|
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
Comment 8•7 years ago
|
||
Backed out changeset 92532463d99c (bug 1445089) for web platform test failures on HTMLTrackElement/kind.html.
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=92532463d99c16b6abd0ea70ec1df59f1883a59e&tochange=46de8c37c991cf779339cb38b14bd7fc0ce6a2b6
Failures log: https://treeherder.mozilla.org/logviewer.html#?job_id=169293254&repo=autoland&lineNumber=7056
Backout link: https://hg.mozilla.org/integration/autoland/rev/46de8c37c991cf779339cb38b14bd7fc0ce6a2b6
Flags: needinfo?(bvandyk)
Comment 9•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
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.
Reporter | ||
Comment 12•7 years ago
|
||
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
Bug created, let's see where it goes: https://developercommunity.visualstudio.com/content/problem/223146/constexpr-code-gneration-bug.html
Comment 15•2 years ago
|
||
Unassigning bugs assigned to Bryce because he no longer works at Mozilla.
Assignee: brycebugemail → nobody
Assignee | ||
Comment 16•2 years ago
|
||
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
Assignee | ||
Comment 17•2 years ago
|
||
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.
Comment 18•2 years ago
|
||
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
Comment 19•2 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox106:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•