Last Comment Bug 657710 - strict-aliasing violation in JS_Stringify / Valueify
: strict-aliasing violation in JS_Stringify / Valueify
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks: 657974
  Show dependency treegraph
 
Reported: 2011-05-17 11:46 PDT by Philip Taylor
Modified: 2011-05-23 14:13 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (sorta) (804 bytes, patch)
2011-05-17 14:45 PDT, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description Philip Taylor 2011-05-17 11:46:29 PDT
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 (:billm) 2011-05-17 14:45:51 PDT
Created attachment 533079 [details] [diff] [review]
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 [:njn] 2011-05-17 16:56:59 PDT
(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] 2011-05-17 17:00:57 PDT
(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 [:njn] 2011-05-17 17:22:05 PDT
(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] 2011-05-17 17:41:24 PDT
(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) 2011-05-17 17:48:12 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-17 21:16:53 PDT
-WX is the MSVC flag to fail on warnings.

You still have to fix your warnings first, of course :-)
Comment 8 Phil Ringnalda (:philor) 2011-05-17 21:31:08 PDT
Though one can put on one's releng hat and just start it running, red, and wash one's hands :)
Comment 9 Nicholas Nethercote [:njn] 2011-05-17 22:06:22 PDT
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 2011-05-18 02:33:41 PDT
(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] 2011-05-18 08:31:35 PDT
(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] 2011-05-18 08:34:16 PDT
(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 2011-05-18 08:46:58 PDT
(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 (:billm) 2011-05-18 10:34:00 PDT
http://hg.mozilla.org/tracemonkey/rev/1c4b22465a5d

(This just fixes the warning.)
Comment 15 Nicholas Nethercote [:njn] 2011-05-18 10:49:10 PDT
(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) 2011-05-23 14:13:31 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/1c4b22465a5d

Note You need to log in before you can comment on or make changes to this bug.