1243617 - Remove JS_VALUE_IS_CONSTEXPR

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

JS_VALUE_IS_CONSTEXPR has two problems:

1. It represents support for both constexpr *and* defaulted constructors,
which are not necessarily things, but compilers may support one but not the
other.  Nowadays we require support for defaulted constructors everywhere.

2. It is basically a slightly incorrect replica of MOZ_HAVE_CXX11_CONSTEXPR.
This causes an ABI mismatch issue between object files containing usage of
js::Value compiled with MSVC 2013 and clang-cl, where clang thinks js::Value
is constexpr but not MSVC.

It's best to just replace it with MOZ_HAVE_CXX11_CONSTEXPR.

Comment 1

[(no longer active)]

Attachment: Remove JS_VALUE_IS_CONSTEXPR

JS_VALUE_IS_CONSTEXPR has two problems:

1. It represents support for both constexpr *and* defaulted constructors,
which are not necessarily things, but compilers may support one but not the
other.  Nowadays we require support for defaulted constructors everywhere.

2. It is basically a slightly incorrect replica of MOZ_HAVE_CXX11_CONSTEXPR.
This causes an ABI mismatch issue between object files containing usage of
js::Value compiled with MSVC 2013 and clang-cl, where clang thinks js::Value
is constexpr but not MSVC.

It's best to just replace it with MOZ_HAVE_CXX11_CONSTEXPR.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8712987 [details] [diff] [review]
Remove JS_VALUE_IS_CONSTEXPR

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

How does this actually solve your problem?  MSVC 2013 doesn't have constexpr support, but clang-cl does, so you still get ABI mismatches, no?

::: js/public/Value.h
@@ -403,5 @@
> - * features are:
> - *
> - * - constexpr;
> - * - defaulted functions;
> - * - C99-style designated initializers.

Does MSVC 2015 support designated initializers?  Does clang-cl support them when emulating 2013, even if 2013 doesn't support them?

@@ -413,5 @@
> -#elif defined(__GNUC__)
> -/*
> - * We need 4.5 for defaulted functions, 4.6 for constexpr, 4.7 because 4.6
> - * doesn't understand |(X) { .field = ... }| syntax, and 4.7.3 because
> - * versions prior to that have bugs in the C++ front-end that cause crashes.

This makes me nervous; our minimum supported version, according to Compiler.h, is 4.7.0, and if we change this to only require constexpr support, then people compiling with < 4.7.3 will experience peculiar crashes.

I don't know how many people actually do this; I see that Debian wheezy ("oldstable") only supports 4.7.2...

Comment 3

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8712987 [details] [diff] [review]
> Remove JS_VALUE_IS_CONSTEXPR
> 
> Review of attachment 8712987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How does this actually solve your problem?  MSVC 2013 doesn't have constexpr
> support, but clang-cl does, so you still get ABI mismatches, no?

This <https://dxr.mozilla.org/mozilla-central/source/js/public/Value.h?case=true&from=js%3A%3AValue#1043> being ifdefed out causes the type to not be POD, which changes the ABI requirements at least in cases where you return a js::Value from a function.

> ::: js/public/Value.h
> @@ -403,5 @@
> > - * features are:
> > - *
> > - * - constexpr;
> > - * - defaulted functions;
> > - * - C99-style designated initializers.
> 
> Does MSVC 2015 support designated initializers?

Yes.

> Does clang-cl support them
> when emulating 2013, even if 2013 doesn't support them?

Yeah, clang-cl doesn't change what parts of the language it supports to match the missing stuff from each MSVC version.

> @@ -413,5 @@
> > -#elif defined(__GNUC__)
> > -/*
> > - * We need 4.5 for defaulted functions, 4.6 for constexpr, 4.7 because 4.6
> > - * doesn't understand |(X) { .field = ... }| syntax, and 4.7.3 because
> > - * versions prior to that have bugs in the C++ front-end that cause crashes.
> 
> This makes me nervous; our minimum supported version, according to
> Compiler.h, is 4.7.0, and if we change this to only require constexpr
> support, then people compiling with < 4.7.3 will experience peculiar crashes.
> 
> I don't know how many people actually do this; I see that Debian wheezy
> ("oldstable") only supports 4.7.2...

Fair.  Can you please suggest an alternative that you'd like to see?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > How does this actually solve your problem?  MSVC 2013 doesn't have constexpr
> > support, but clang-cl does, so you still get ABI mismatches, no?
> 
> This
> <https://dxr.mozilla.org/mozilla-central/source/js/public/Value.
> h?case=true&from=js%3A%3AValue#1043> being ifdefed out causes the type to
> not be POD, which changes the ABI requirements at least in cases where you
> return a js::Value from a function.

Ah, that makes sense.

> > @@ -413,5 @@
> > > -#elif defined(__GNUC__)
> > > -/*
> > > - * We need 4.5 for defaulted functions, 4.6 for constexpr, 4.7 because 4.6
> > > - * doesn't understand |(X) { .field = ... }| syntax, and 4.7.3 because
> > > - * versions prior to that have bugs in the C++ front-end that cause crashes.
> > 
> > This makes me nervous; our minimum supported version, according to
> > Compiler.h, is 4.7.0, and if we change this to only require constexpr
> > support, then people compiling with < 4.7.3 will experience peculiar crashes.
> > 
> > I don't know how many people actually do this; I see that Debian wheezy
> > ("oldstable") only supports 4.7.2...
> 
> Fair.  Can you please suggest an alternative that you'd like to see?

Can we just remove the ifdef around the defaulted constructors?  Since all our compilers support that, that would at least let everybody get along from the ABI side of things.  Do you know if the |= default| constructors are more or less efficient than the alternative?

Comment 5

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> > > @@ -413,5 @@
> > > > -#elif defined(__GNUC__)
> > > > -/*
> > > > - * We need 4.5 for defaulted functions, 4.6 for constexpr, 4.7 because 4.6
> > > > - * doesn't understand |(X) { .field = ... }| syntax, and 4.7.3 because
> > > > - * versions prior to that have bugs in the C++ front-end that cause crashes.
> > > 
> > > This makes me nervous; our minimum supported version, according to
> > > Compiler.h, is 4.7.0, and if we change this to only require constexpr
> > > support, then people compiling with < 4.7.3 will experience peculiar crashes.
> > > 
> > > I don't know how many people actually do this; I see that Debian wheezy
> > > ("oldstable") only supports 4.7.2...
> > 
> > Fair.  Can you please suggest an alternative that you'd like to see?
> 
> Can we just remove the ifdef around the defaulted constructors?  Since all
> our compilers support that, that would at least let everybody get along from
> the ABI side of things.

Sure.

> Do you know if the |= default| constructors are
> more or less efficient than the alternative?

Hmm, not quite sure what you mean.  What alternative?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #5)
> > Do you know if the |= default| constructors are
> > more or less efficient than the alternative?
> 
> Hmm, not quite sure what you mean.  What alternative?

Sorry, that was worded unclearly.  I mean if you have:

class ExplicitlyDefaultedValue {
  ExplicitlyDefaultedValue() = default;
  ExplicitlyDefaultedValue(const ExplicitlyDefaultedValue&) = default;
  ...
};

vs.

class ImplicitlyDefinedValue {
  // compiler provides copy constructor and () constructor as necessary
  ...
};

which one is more efficient for argument-passing and returning on Windows?

(I'm going to assume that ni? was directed at me and clear it appropriately.)

Comment 7

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to :Ehsan Akhgari from comment #5)
> > > Do you know if the |= default| constructors are
> > > more or less efficient than the alternative?
> > 
> > Hmm, not quite sure what you mean.  What alternative?
> 
> Sorry, that was worded unclearly.  I mean if you have:
> 
> class ExplicitlyDefaultedValue {
>   ExplicitlyDefaultedValue() = default;
>   ExplicitlyDefaultedValue(const ExplicitlyDefaultedValue&) = default;
>   ...
> };
> 
> vs.
> 
> class ImplicitlyDefinedValue {
>   // compiler provides copy constructor and () constructor as necessary
>   ...
> };
> 
> which one is more efficient for argument-passing and returning on Windows?

These two programs are semantically equivalent, therefore they should generate the exact same code.  If your question is about a non-POD versus POP class, the non-POD class would be more expensive as the caller would need to set up the object at call site and pass its address to the callee.

Let me explain more.  The issue about js::Value is that if JS_VALUE_IS_CONSTEXPR is defined, it has a constexpr constructor that constructs it from a jsval_layout.  Therefore, without the defaulted constructors it would cease to have a default constructor (because of the presence of the user-defined constructor) and would turn into non-POD.

Which actually makes me realized that I was confused in comment 5.  Your suggestion doesn't actually fix anything since MSVC still treats this as non-POD, and clang-cl would continue to treat it as POD.  The real fix is to not define JS_VALUE_IS_CONSTEXPR in clang-cl (which is the reason why I replaced this with MOZ_HAVE_CXX11_CONSTEXPR.)  So, should we revisit this patch?

> (I'm going to assume that ni? was directed at me and clear it appropriately.)

Nope, it was against me as a reminder to keep this in my queue.  :-)  Luckily I also had the patch in my queue which is how I remembered to look at this again!

Comment 9

[Nathan Froyd [:froydnj]]

I think I would be OK with:

1. Adding = default to constructors unconditionally;
2. Keeping JS_VALUE_IS_CONSTEXPR for __GCC__ only; having this avoids a raft of static initializers in the bindings code and is sadly the best solution until we upgrade our minimum supported GCC version.  (Once we move to 4.8 or so, we can just make things us the MOZ_HAVE_CXX11_CONSTEXPR as you suggest.)

Sound fair?

Comment 10

[Chris Peterson [:cpeterson]]

MOZ_HAVE_CXX11_CONSTEXPR was removed in bug 1277775 (Nightly 50) because all our compilers support constexpr.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add constructor for JS::Value::layout.

|data({ .asDouble = d })| throws syntax error on VisualStudio:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41dcef9b2151&selectedJob=29459906

So added constexpr constructor to the layout union, so that we can use it instead for constexpr JS::Value constructor.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove JS_VALUE_IS_CONSTEXPR.

Other things are available on all compilers.
just removed macro.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8802819 [details] [diff] [review]
Part 2: Remove JS_VALUE_IS_CONSTEXPR.

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

::: js/public/Value.h
@@ +917,5 @@
>          JS_STATIC_ASSERT(sizeof(JSWhyMagic) <= 4);
>          JS_STATIC_ASSERT(sizeof(Value) == 8);
>      }
>  
> +    friend Value constexpr (JS::UndefinedValue)();

Put constexpr before the return type, and remove the parentheses around JS::UndefinedValue.

Comment 14

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eda1ba4d8bedd18200201a185a85de2de7d3f709
Bug 1243617 - Part 1: Add constructor for JS::Value::layout. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b3f4302b4f5cb915224507464130c0c647cbea
Bug 1243617 - Part 2: Remove JS_VALUE_IS_CONSTEXPR. r=jwalden

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eda1ba4d8bed
https://hg.mozilla.org/mozilla-central/rev/a6b3f4302b4f