696220 - js1_8_5/extensions/reflect-parse.js is failing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The location information is bogus. Valgrind finds uninitialized memory reads. I have a fix.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jason Orendorff [:jorendorff]]

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]]

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]]

(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]]

Comment on attachment 568521 [details] [diff] [review]
v1

Oops, didn't see the r?.

Comment 6

[Jason Orendorff [:jorendorff]]

Thanks, Luke.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa6410c1a3a

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fa6410c1a3a