Closed Bug 1408695 Opened 2 years ago Closed 2 years ago

Work around a VS2017 constexpr pointer math bug in HTMLTrackElement.cpp

Categories

(Core :: Audio/Video, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

In a constexpr context, VS2017 thinks that &foo[N] is equal to ((char*)foo)+N, rather than ((char*)foo)+N*sizeof(foo)

------------ Reproducer ------------

#include <iostream>

struct Entry {
    const char* tag;
    int16_t value;
};

static constexpr Entry table[] = {
  { "foo", 123 },
  { "bar", 42 },
};

static constexpr const Entry* x = &table[1];
static const Entry* const y = &table[1];

void main()
{
   std::cout << "x: " << x->value << std::endl;
   std::cout << "y: " << y->value << std::endl;
}

/*
C:\>cl -EHsc test.cpp && test
Microsoft (R) C/C++ Optimizing Compiler Version 19.11.25547 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cpp
Microsoft (R) Incremental Linker Version 14.11.25547.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:test.exe
test.obj
x: 0
y: 42
*/
Attached patch patchSplinter Review
Assignee: nobody → dmajor
Note that this breaks mochitests if it's not correct, so I didn't see the need to add warning comments etc. 

There are a number of other code locations that use this invalid-default nsAttrValue::EnumTable* pattern, but HTMLTrackElement.cpp is the only one that tried to use constexpr: https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsAttrValue%3A%3AParseEnumValue%28const+nsAString+%26%2C+const+nsAttrValue%3A%3AEnumTable+%2A%2C+bool%2C+const+nsAttrValue%3A%3AEnumTable+%2A%29%22
Did you report the bug to Microsoft?
(In reply to Masatoshi Kimura [:emk] from comment #3)
> Did you report the bug to Microsoft?

Not yet. I will, but it shouldn't block this landing.
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d3df91ae2f
Work around a VS2017 constexpr pointer math bug in HTMLTrackElement.cpp. r=gerald
https://hg.mozilla.org/mozilla-central/rev/60d3df91ae2f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I hit this MSVC bug in bug 1411469. Unfortunately, I need the `constexpr` so I've had to use a nastier workaround :(
The MS bug report says "We've taken a look and fixed the bug in VS2017 15.6 Preview 4 release."
So this workaround should be limited to `_MSC_VER >= 1910 && _MSC_VER < 1913`. (Or remove the workaround and drop support for earlier versions of VS2017.)
Depends on: 1445089
You need to log in before you can comment on or make changes to this bug.