Open Bug 1445089 Opened 3 years ago Updated 3 years ago
Revert workaround for a VS2017 <15
.6 constexpr pointer math bug
59 bytes, text/x-review-board-request
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/92532463d99c Revert workaround for a VS2017 <15.6 constexpr pointer math bug. r=dmajor
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
Updated push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=92532463d99c16b6abd0ea70ec1df59f1883a59e&tochange=46de8c37c991cf779339cb38b14bd7fc0ce6a2b6&filter-searchStr=wpt&selectedJob=169293254
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.
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
Bug created, let's see where it goes: https://developercommunity.visualstudio.com/content/problem/223146/constexpr-code-gneration-bug.html
You need to log in before you can comment on or make changes to this bug.