Last Comment Bug 696220 - js1_8_5/extensions/reflect-parse.js is failing
: js1_8_5/extensions/reflect-parse.js is failing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 696268 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 14:32 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-11-17 12:22 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.35 KB, patch)
2011-10-20 14:34 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-10-20 14:32:13 PDT
The location information is bogus. Valgrind finds uninitialized memory reads. I have a fix.
Comment 1 Jason Orendorff [:jorendorff] 2011-10-20 14:34:17 PDT
Created attachment 568521 [details] [diff] [review]
v1
Comment 2 Jason Orendorff [:jorendorff] 2011-10-20 14:45:26 PDT
Here's what happened here.

struct TokenPos previously had no constructors declared. I added this:
    TokenPos() {}

But there is a struct Token that includes a TokenPos, and a class TokenStream
that has a member `Token tokens[N];'. TokenStream's constructor did this:
      : tokens()

This apparently had the effect of zeroing out all the members of each Token, though I have a hard time constructing a mental model of C++ in which that makes any sense... By adding the do-nothing constructor, I changed the meaning from "fill with zeroes" to "don't initialize".

We um... we probably shouldn't be depending on that, but for today let's just restore the status quo rather than charging forward unwarily.
Comment 3 Brendan Eich [:brendan] 2011-10-23 15:15:35 PDT
See bug 588061. If only jsreflect.cpp needs zero-init, and only of the not-yet-got "first token", we should hack only that file, or find a better fix for bug 588061.

/be
Comment 4 Luke Wagner [:luke] 2011-10-24 09:17:12 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> This apparently had the effect of zeroing out all the members of each Token,
> though I have a hard time constructing a mental model of C++ in which that
> makes any sense... By adding the do-nothing constructor, I changed the
> meaning from "fill with zeroes" to "don't initialize".

To wit, (8.5.10) "An object whose initializer is an empty set of parenthesis, i.e., (), shall be value-initialized."  8.5.7 then specifies value-initialization to be: if you don't have a user-provided constructor, your zero-initialize your members and, if you do, then you just call the user-provided constructor without zero-initialization.
Comment 5 Luke Wagner [:luke] 2011-10-26 16:26:25 PDT
Comment on attachment 568521 [details] [diff] [review]
v1

Oops, didn't see the r?.
Comment 6 Jason Orendorff [:jorendorff] 2011-10-27 14:54:52 PDT
Thanks, Luke.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa6410c1a3a
Comment 7 Ed Morley [:emorley] 2011-10-28 04:30:34 PDT
https://hg.mozilla.org/mozilla-central/rev/9fa6410c1a3a
Comment 8 Jason Orendorff [:jorendorff] 2011-11-17 12:22:43 PST
*** Bug 696268 has been marked as a duplicate of this bug. ***

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