1319971 - js\src\ds/OrderedHashTable.h(775): error C2131: expression did not evaluate to a constant when using VS2017 RC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Makoto Kato [:m_kato]]

When using VS2017 RC, it cannot resolve constexpr of OrderedHashTable.h.

c:\development\hg.mozilla.org\mozilla-win64\js\src\ds/OrderedHashTable.h(775): error C2131: expression did not evaluate to a constant
c:\development\hg.mozilla.org\mozilla-win64\js\src\ds/OrderedHashTable.h(532): note: failure was caused by non-constant arguments or reference to a non-constant symbol
c:\development\hg.mozilla.org\mozilla-win64\js\src\ds/OrderedHashTable.h(532): note: see usage of 'js::detail::OrderedHashTable<js::OrderedHashMap<js::HashableValue,js::HeapPtr<JS::Value>,js::HashableValue::Hasher,js::RuntimeAllocPolicy>::Entry,js::OrderedHashMap<js::HashableValue,js::HeapPtr<JS::Value>,js::HashableValue::Hasher,js::RuntimeAllocPolicy>::MapOps,AllocPolicy>::Data::element'
        with
        [
            AllocPolicy=js::RuntimeAllocPolicy
        ]
c:/Development/hg.mozilla.org/mozilla-win64/js/src/jit/CodeGenerator.cpp(6394): note: see reference to function template instantiation 'void js::jit::RangePopFront<OrderedHashTable>(js::jit::MacroAssembler &,js::jit::Register,js::jit::Register,js::jit::Register,js::jit::Register)' being compiled
        with
        [
            OrderedHashTable=js::ValueMap
        ]
c:/Development/hg.mozilla.org/mozilla-win64/js/src/jit/CodeGenerator.cpp(6419): note: see reference to function template instantiation 'void js::jit::CodeGenerator::emitGetNextEntryForIterator<js::MapIteratorObject,js::ValueMap>(js::jit::LGetNextEntryForIterator *)' being compiled
c:\development\hg.mozilla.org\mozilla-win64\js\src\ds/OrderedHashTable.h(794): note: see reference to class template instantiation 'js::detail::OrderedHashTable<T,js::OrderedHashSet<T,js::HashableValue::Hasher,js::RuntimeAllocPolicy>::SetOps,AllocPolicy>' being compiled
        with
        [
            T=js::HashableValue,
            AllocPolicy=js::RuntimeAllocPolicy
        ]
 :
 :

Comment 1

[Makoto Kato [:m_kato]]

Reported https://connect.microsoft.com/VisualStudio/feedback/details/3113051

Comment 2

[Makoto Kato [:m_kato]]

new URL: https://developercommunity.visualstudio.com/content/problem/22196/static-assert-cannot-compile-constexprs-method-tha.html

Comment 3

[(Away)]

Attachment: Convert static_assert to MOZ_ASSERT

Here's a simple patch that converts static_assert to MOZ_ASSERT.

I considered adding something like a JS_OFFSETOF_ASSERT and letting that happen at compile-time everywhere except VS2017, but it felt like overkill. Let me know if you disagree.

Comment 4

[Luke Wagner [:luke]]

From my brief reading of the bug, it looks like the issue is offsetof-in-a-constexpr-method and so a static_assert of just an offsetof would work.  If that's right, then perhaps we could just move the static_asserts to be inside the relevant classes where they can name the private fields directly instead of going through a constexpr method.  I kinda like having the static_asserts next to the definition for readability, so I think this would be preferable.

Comment 5

[(Away)]

(In reply to Luke Wagner [:luke] from comment #4)
> From my brief reading of the bug, it looks like the issue is
> offsetof-in-a-constexpr-method and so a static_assert of just an offsetof
> would work.

Oh, good point. I had assumed that all compile-time things were busted with offsetof in VS2017, but it looks like that's not the case. Nicer patch coming up!

Comment 6

[(Away)]

Attachment: Use offsetof directly

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8845162 [details] [diff] [review]
Use offsetof directly

Review of attachment 8845162 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jit/CodeGenerator.cpp
@@ -6464,5 @@
>  {
>      masm.loadPtr(Address(range, ValueMap::Range::offsetOfHashTable()), front);
>      masm.loadPtr(Address(front, ValueMap::offsetOfImplData()), front);
>  
> -    static_assert(ValueMap::offsetOfImplDataElement() == 0, "offsetof(Data, element) is 0");

These static asserts are also useful for locally reasoning about the following codegen, so could you turn them into MOZ_ASSERT()s (keeping the new static asserts).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/261a33fad371
Re-work some static_asserts to get VS2017 compiling. r=luke

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/261a33fad371