684526 - Figure out what to do about gcc pr50284 by reunifying jsval and JS::Value (né js::Value)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

See http://gcc.gnu.org/PR50284 for a description of the problem.

It is possible that gcc is right and our code is wrong. In which case our options are

* refactor our code (looks like a lot of it).
* disable strict aliasing.
* try to hide the bug and see if we are lucky :-(

If it is really a gcc bug, I think our options are:

*) Fix it (or wait for it to be fixed) and port the change.
*) Port 160947 and see if it "fixes" the problem for us too.

Right now porting 160947 looks like an interesting compromise. It should be easy to do and avoids disabling strict aliasing.

Comment 1

[Mike Hommey [:glandium]]

Just disable strict aliasing and figure out what you want to do later.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Mike Hommey [:glandium] from comment #1)
> Just disable strict aliasing and figure out what you want to do later.

OK, I will write a configure check to disable strict aliasing when the test in the pr fails. Note that last time we did it we got a performance hit.

Comment 3

[Mike Hommey [:glandium]]

Wasn't it disabled, before?

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Currently it is disable outside of js and enabled in it.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #4)
> Currently it is disable outside of js and enabled in it.

And it was disabled in js before bug 680515, that is, until very recently.

Comment 6

[Mike Hommey [:glandium]]

(except on mac if I read history correctly)

Comment 7

[Mike Hommey [:glandium]]

In other words, just make it do what it was doing before bug 680515. I don't think we need more at the moment. When we know how this should be treated, then we can do configure checks if necessary. Not before.
BTW, I don't understand why that bug changed defaults, when that wasn't the stated intent.

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Mike Hommey [:glandium] from comment #7)
> In other words, just make it do what it was doing before bug 680515. I don't
> think we need more at the moment. When we know how this should be treated,
> then we can do configure checks if necessary. Not before.
> BTW, I don't understand why that bug changed defaults, when that wasn't the
> stated intent.

I don't think it did. An earlier version of the patch disabled strict-aliasing. That caused performance regressions (comment 29) and we enabled it again.

Comment 9

[Mike Hommey [:glandium]]

Mmmm interesting, there was -fno-strict-aliasing *and* -fstrict-aliasing on the compiler command lines, in that order. I was misled by the former.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: proposed patch

The gcc bug was closed as invalid. I am still not 100% sure about it, but richi pointed out that we were casting a jsval object to js::Value* because of a copy.

I had missed that, and it is probably reasonable to say that it is at least a strange thing to do. This patch avoids the copy and therefore casting a jsval object to js::Value.

This bug does show that the api we have is fairly brittle. Is there any reason for having jsval and js::Value these days? If so, can we at least make jsval not copyable?

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Remove JSValueArray and move Valueify calls up.

This patch moves calls to Valueify upward and removes JSValueArray to try to make our api a bit safer.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 558200 [details] [diff] [review]
proposed patch

I don't think this helps.

Comment 13

[David Mandelin [:dmandelin]]

What exactly is the bug that this patch is trying to fix?

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to David Mandelin from comment #13)
> What exactly is the bug that this patch is trying to fix?

One (as noted by Jason, not all) strict aliasing violations. In particular, when assertSameCompartment is passed a js::Value and a JSValueArray, gcc assumes that the pointers will not alias.

Comment 15

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 558211 [details] [diff] [review]
Remove JSValueArray and move Valueify calls up.

On IRC we agreed to try to cover a few more cases before.

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: remove unused and unsafe version of Valueify

Comment 17

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Remove the jsval version of AutoArrayRooter

Comment 18

[Luke Wagner [:luke]]

I thought the plan was to make jsval == js::Value #if defined(__cplusplus) and then remove Jsvalify/Valueify altogether?

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Luke Wagner [:luke] from comment #18)
> I thought the plan was to make jsval == js::Value #if defined(__cplusplus)
> and then remove Jsvalify/Valueify altogether?

That is correct, but such a patch would be enormous, so I am trying to break it up. Note that it is not possible to simply typedef one to the other since we have many overloaded functions and templates that then become duplicates.

What I have done in the above patches is
*) Remove one definition of valueify (from a pointer jsval) and try to build
*) Find and are of code or data structure that breaks
*) Put valueify back and fix that chunk of code

Comment 20

[Luke Wagner [:luke]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #19)
> That is correct, but such a patch would be enormous,

Not small, but not enormous.  Also very simple.

> so I am trying to break it up.

IIUC, you aren't since you are making changes that wouldn't be necessary if jsval === Value.

> Note that it is not possible to simply typedef one to the other since
> we have many overloaded functions and templates that then become duplicates.

Oh its possible alright, just kill one (preferably the one that references 'jsval') :)

Comment 21

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Luke Wagner [:luke] from comment #20)
> (In reply to Rafael Ávila de Espíndola (:espindola) from comment #19)
> > That is correct, but such a patch would be enormous,
> 
> Not small, but not enormous.  Also very simple.

I don't think I agree. As I mentioned, I started by deleting Valueify and trying to build, and it touches every file.

> > so I am trying to break it up.
> 
> IIUC, you aren't since you are making changes that wouldn't be necessary if
> jsval === Value.

The calls to Valueify move out, after some patches they will get out of the code.

> > Note that it is not possible to simply typedef one to the other since
> > we have many overloaded functions and templates that then become duplicates.
> 
> Oh its possible alright, just kill one (preferably the one that references
> 'jsval') :)

That is what the patch does!

Comment 22

[Luke Wagner [:luke]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #21)
My point is simplify that your patches are doing work that is unnecessary with jsval === js::Value.  You don't even have to remove Jsvalify/Valueify initially; just make jsval === js::Value as we discussed and fix what breaks.  For example, the patches contain a lot of this:

-    assertSameCompartment(cx, v);
+    assertSameCompartment(cx, Valueify(v));

Here you are *adding* cases to be removed by a later Jsvalify/Valueify-removal patch.

Comment 23

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Here you are *adding* cases to be removed by a later
> Jsvalify/Valueify-removal patch.

Yes, because it is a lot easier than removing the full thing at once.

In any case, as discussed on IRC Luke will split jsvalue.h, moving the definition of js::Value to jsval.h. I will take from there.

Comment 24

[Luke Wagner [:luke]]

Attachment: JS::Value === jsval

This patch moves js::Value into jsapi.h (as JS::Value) and makes jsval a typedef for JS::Value.  To let internal code keep using Value and js::Value, I put a "using namespace JS" inside namespace js (in jsutil.h), just like we did with namespace mozilla.  I also made Jsvalify/Valueify into identify functions, so all strict aliasing problems should be fixed by this patch.  Note that JS::Value is only declared, not defined, in jspubtd.h and, since its a typedef, jsval isn't declared at all in jspubtd.h.  This is the reason for the fixups to code outside js/src.

Next patch will dismantle jsvalue.h and remove Jsvalify/Valueify altogether.

Comment 25

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: work in progres patch.

This is my work in progress patch. Should at least list places where we have overloaded functions/template specializations that need to have the jsval one removed.

Comment 26

[Jeff Walden [:Waldo]]

Luke, jsapi.h is a C header now (or was last I checked).  Doesn't having jsval be a typedef of JS::Value break that?

As far as I'm concerned, being able to assume C++ would only make things easier for SpiderMonkey hackers.  But if we decide to make jsapi.h a C++ header, we should not do it sub silentio.

Comment 27

[Jeff Walden [:Waldo]]

Oh, I see:

  typedef union jsval_layout jsval;

If we are indeed keeping compatibility, my concern disappears.  :-)

Comment 28

[Luke Wagner [:luke]]

Attachment: JS::Value === jsval

Slight tweaks to build on all platforms; with that green on try server.

Comment 29

[Luke Wagner [:luke]]

Attachment: rm Valueify/Jsvalify and jsvalue.h

Comment 30

[Jason Orendorff [:jorendorff]]

Comment on attachment 561303 [details] [diff] [review]
JS::Value === jsval

I guess in the places where you added "js::", we were relying on
argument-dependent lookup, presumably without knowing it? So weird. :)

In jsval.h:
>-JSDOUBLE_IS_NEGZERO(jsdouble d)
>+JSDOUBLE_IS_NEGZERO(double d)

Why can't this continue to use jsdouble?

In jsapi.h:
> extern JS_PUBLIC_API(JSBool)
> JS_UnlockGCThingRT(JSRuntime *rt, void *thing);
> 
> /*
>+ * JS_CallTracer API and related macros for implementors of JSTraceOp, to
>+ * enumerate all references to traceable things reachable via a property or
>+ * other strong ref identified for debugging purposes by name or index or
>+ * a naming callback.
>+ *
>+ * See the JSTraceOp typedef.
>+ */
>+
>+/*
>  * Register externally maintained GC roots.
>  *
>  * traceOp: the trace operation. For each root the implementation should call
>  *          JS_CallTracer whenever the root contains a traceable thing.
>  * data:    the data argument to pass to each invocation of traceOp.
>  */
> extern JS_PUBLIC_API(void)
> JS_SetExtraGCRoots(JSRuntime *rt, JSTraceDataOp traceOp, void *data);
> 
> /*
>- * JS_CallTracer API and related macros for implementors of JSTraceOp, to
>- * enumerate all references to traceable things reachable via a property or
>- * other strong ref identified for debugging purposes by name or index or
>- * a naming callback.
>- *
>- * See the JSTraceOp typedef in jspubtd.h.
>- */

I think this comment was better where it was.

r=me, sorry for the slowness.

Comment 31

[Luke Wagner [:luke]]

Yes, weird indeed

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9a7183a197
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c29c2e92225

Comment 32

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4f9a7183a197
https://hg.mozilla.org/mozilla-central/rev/5c29c2e92225