657710 - strict-aliasing violation in JS_Stringify / Valueify

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Philip Taylor]

Building TM tip with GCC 4.4.5 on x86_64 (with --enable-optimize --disable-debug) gives:

  js/src/jsapi.cpp: In function ‘JSBool JS_Stringify(JSContext*, jsval*, JSObject*, jsval, JSBool (*)(const jschar*, uint32, void*), void*)’:
  js/src/jsapi.cpp:5525: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules
  js/src/jsapi.cpp:5525: note: initialized from here

which is the line

  if (!js_Stringify(cx, Valueify(vp), replacer, Valueify(space), sb))

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix (sorta)

This makes the warning go away.

Does anyone know why warnings-as-errors has been re-enabled? I thought it was turned off.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> Does anyone know why warnings-as-errors has been re-enabled? I thought it
> was turned off.

It was turned back on, but only for Tinderbox builds.  It's on because there's a general consensus that the benefits of warnings outweight their costs.  But only for Tinderbox builds because otherwise you get too much breakage on obscure platforms.

Comment 3

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> (In reply to comment #1)
> > 
> > Does anyone know why warnings-as-errors has been re-enabled? I thought it
> > was turned off.
> 
> It was turned back on, but only for Tinderbox builds. 

Really? Yesterday, on Windows I built from a green Tinderbox, and got errors. So it seems that on Windows, it's on, but only for non-Tinderbox builds.

> It's on because there's a general consensus that the benefits of warnings 
> outweight their costs.  But only for Tinderbox builds because otherwise you 
> get too much breakage on obscure platforms.

FTR, I don't agree. I've been fixing Windows warnings as they come up for a while, which is fine with me. But pulling from TM and then finding I *must* fix some warnings before I can do my work seems like a pretty bad state. But maybe that's not what was intended.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> > > 
> > It was turned back on, but only for Tinderbox builds. 

I was wrong; it's only on for the "SM (e)" builds.  But it appears that we don't have those builds on Windows, I thought we did.  philor, do you know what's happening there?


> Really? Yesterday, on Windows I built from a green Tinderbox, and got
> errors. So it seems that on Windows, it's on, but only for non-Tinderbox
> builds.

You got warnings that were turned into errors?  That shouldn't happen unless you configured with --enable-sm-fail-on-warnings.  Are you building in an old directory that might still have configure options set from the time when we did have warnings-as-errors on all the time?


> FTR, I don't agree. I've been fixing Windows warnings as they come up for a
> while, which is fine with me. But pulling from TM and then finding I *must*
> fix some warnings before I can do my work seems like a pretty bad state. But
> maybe that's not what was intended.

You certainly shouldn't have to do that.  The current setup is meant to ensure that if someone else introduces a Windows warnings, then "SM (e)" will go red and they'll fix it.  And if they're slow about it, it shouldn't cause you problems in the meantime because warnings-as-errors shouldn't be on in your local builds.  I think the design of the setup is valid, but it sounds like the current implementation is flawed.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> (In reply to comment #3)
> > > > 
> > > It was turned back on, but only for Tinderbox builds. 
> 
> I was wrong; it's only on for the "SM (e)" builds.  But it appears that we
> don't have those builds on Windows, I thought we did.  philor, do you know
> what's happening there?
> 
> 
> > Really? Yesterday, on Windows I built from a green Tinderbox, and got
> > errors. So it seems that on Windows, it's on, but only for non-Tinderbox
> > builds.
> 
> You got warnings that were turned into errors?  That shouldn't happen unless
> you configured with --enable-sm-fail-on-warnings.  Are you building in an
> old directory that might still have configure options set from the time when
> we did have warnings-as-errors on all the time?

It could be that. I thought I reconfigured, but maybe something was left over. If I get a warning-as-error again, I'll make a fresh build dir and try again.

> > FTR, I don't agree. I've been fixing Windows warnings as they come up for a
> > while, which is fine with me. But pulling from TM and then finding I *must*
> > fix some warnings before I can do my work seems like a pretty bad state. But
> > maybe that's not what was intended.
> 
> You certainly shouldn't have to do that.  The current setup is meant to
> ensure that if someone else introduces a Windows warnings, then "SM (e)"
> will go red and they'll fix it.  And if they're slow about it, it shouldn't
> cause you problems in the meantime because warnings-as-errors shouldn't be
> on in your local builds.  I think the design of the setup is valid, but it
> sounds like the current implementation is flawed.

OK, I'll reserve judgment until it's implemented correctly. And it seems like Windows is mostly as it was, aside from hopefully just configuration bustage on my side.

Comment 6

[Phil Ringnalda (:philor)]

We don't have SM(e) on Windows because in bug 609532, you fixed up the warnings on Mac and Linux before turning on -Werror on Mac and Linux before having to turn it off again, so I got you substitute shell builds, again on Mac and Linux, where I knew you had made them green and I knew how to do them because you had already done so once.

As you said there, "If anyone wants to supply a patch for MSVC that would be great." Personally, I don't know how to build with warnings-as-errors on MSVC, I don't know what warnings we currently have on MSVC, and if the attachment in bug 609532 is a current list of them, I don't know how to fix them.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

-WX is the MSVC flag to fail on warnings.

You still have to fix your warnings first, of course :-)

Comment 8

[Phil Ringnalda (:philor)]

Though one can put on one's releng hat and just start it running, red, and wash one's hands :)

Comment 9

[Nicholas Nethercote [inactive]]

dmandelin, it sounds like you fix MSVC warnings -- what's the current status for SpiderMonkey?  Are we close to being able to turn on -WX?  In other words, are there many or few (or zero?) warnings on Windows?  If the number is low, we should spin off a new bug.

Comment 10

[Paul Biggar]

(In reply to comment #8)
> Though one can put on one's releng hat and just start it running, red, and
> wash one's hands :)

We should do this. It's perfectly fine for that build to be red temporarily, and it will be hard to sync a plan for turning it on with a time in which it is green.

Comment 11

[David Mandelin [:dmandelin]]

(In reply to comment #9)
> dmandelin, it sounds like you fix MSVC warnings -- what's the current status
> for SpiderMonkey?  Are we close to being able to turn on -WX?  In other
> words, are there many or few (or zero?) warnings on Windows?  If the number
> is low, we should spin off a new bug.

There are only two, but they are nontrivial to fix. One is bug 656282, which I think I have a good fix for but seems review-worthy at least. The other is a warning about this in jswrappers.h:

class JS_FRIEND_API(ForceFrame)
{
  public:
    JSContext * const context;
    JSObject * const target;
  private:
    DummyFrameGuard frame;

  public:
    ForceFrame(JSContext *cx, JSObject *target);
    bool enter();
};

The compiler wants DummyFrameGuard to be JS_FRIEND_API as well. Just doing that isn't enough: JS_FRIEND_API then must spread to the base classes and classes of data members recursively. So fixing that might require refactoring. Or turning off that particular warning at that site since it's just supposed to be an ugly hack and apparently it works.

Comment 12

[David Mandelin [:dmandelin]]

(In reply to comment #10)
> (In reply to comment #8)
> > Though one can put on one's releng hat and just start it running, red, and
> > wash one's hands :)
> 
> We should do this. It's perfectly fine for that build to be red temporarily,
> and it will be hard to sync a plan for turning it on with a time in which it
> is green.

It's not perfectly fine to me. I need to be able to work on sg:criticals and other high-priority stuff without having to wade through redness that was introduced as part of dealing with relatively minor issues.

Comment 13

[Paul Biggar]

(In reply to comment #12)
> > We should do this. It's perfectly fine for that build to be red temporarily,
> > and it will be hard to sync a plan for turning it on with a time in which it
> > is green.
> 
> It's not perfectly fine to me. I need to be able to work on sg:criticals and
> other high-priority stuff without having to wade through redness that was
> introduced as part of dealing with relatively minor issues.

I don't understand why a red SM(e) build would affect your priorities, but I certainly agree it shouldn't.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/1c4b22465a5d

(This just fixes the warning.)

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> If the number is low, we should spin off a new bug.

Bug 657974.

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/1c4b22465a5d