920318 - Add a user-provided constructor to the Token class to work around an MSVC 2013/VC12 compiler bug

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Add a user-provided zeroing (ish) constructor

SpiderMonkey has approximately this code in it:

struct TokenPos
{
  uint32_t begin;
  uint32_t end;
  TokenPos() {}
  TokenPos(uint32_t b, uint32_t e) : begin(b), end(e) {}
};

struct Token
{
  TokenPos pos;
};

struct TokenStream
{
  Token tokens[4];

  TokenStream(...) : tokens() {}
};

We instantiate the Parser class on the stack, and Parser contains a TokenStream.

When we construct that Parser, this invokes the TokenStream constructor.  That ()-initializes TokenStream::tokens.  Per C++11 [dcl.init]p10, this value-initializes TokenStream::tokens.  Per [dcl.init]p7 this value-initializes each element.  Token is a non-union class type without a user-provided constructor.  Also, TokenPos's implicitly-declared default constructor is non-trivial.  (This is because it has a non-static data member TokenPos, and Token has a non-trivial default constructor because it is user-provided.)  Thus again per [dcl.init]p7, each element is zero-initialized, then the implicit TokenPos::TokenPos() is called on each (although this call does nothing).

The gist of the above is that, in C++11, TokenStream::tokens is zeroed here.  But in MSVC 2013, for whatever reason, this zeroing doesn't happen.  Some searching suggests MSVC is just kind of buggy in how it initializes stuff (although, apparently buggier in 2013 than in past versions).  This, for instance, contains echoes of the above bugginess:

http://connect.microsoft.com/VisualStudio/feedback/details/790146/visual-c-11-0-fails-to-zero-initialize-instance-of-derived-class#details

So if we want proper zeroing in MSVC 2013, we should add a user-provided Token constructor to get the zeroing behavior we depend upon.  Because Token is only handed out by pointer/reference, this won't affect anything other than Token fields in TokenStream, so this shouldn't affect perf anywhere.

(Interestingly, C++11 changed from what C++98 did.  In C++98, "if [Token] is a non-union class type without a user-declared constructor, then every non-static data member and base-class component of [Token] is value-initialized", per C++98 [dcl.init]p5, which would result in TokenPos() leaving its fields uninitialized.  I don't know if we ever compiled as C++98 *and* relied on C++11-style behavior from gcc/clang.)

dmajor said he'd file a bug for this with Microsoft, and/or comment on an existing one on file for it.  I'll also upload a minimal testcase, that works fine with gcc/clang and MSVC 2010/2012, but fails with MSVC 2013.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: MSVC 2013 testcase (should print zeroes, instead prints garbage)

Comment 2

[(Away)]

I filed https://connect.microsoft.com/VisualStudio/feedback/details/802160

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 809568 [details] [diff] [review]
Add a user-provided zeroing (ish) constructor

Wow, I didn't know C++11 changed this rule; I generally assume the C++98 rules.  w/o being too familiar with TokenStream, I'm surprised we depend on the initial state of tokens; seems like, since lookahead is 0 initially, we won't touch tokens until someone ungets a token and then only the ungot token, which has been intiaialized... do you know otherwise?  If we don't depend on the value, I'd rather leave it either uninitialized or poisoned.  r+ on either resolution.

Comment 4

[(Away)]

The uninitialized data appears to be read in the pos() call here:

    Node pn = handler.newStatementList(pc->blockid(), pos());

At:
js!js::frontend::Parser<js::frontend::FullParseHandler>::statements+0x48
js!js::frontend::Parser<js::frontend::FullParseHandler>::parse+0xe3
js!JS_BufferIsCompilableUnit+0x174

Comment 5

[Luke Wagner [:luke]]

(In reply to David Major [:dmajor] from comment #4)
Ah, thanks.  So it looks like we are just querying the position of the token stream before we've even read the first character and expecting that to be 0.  Trixie.

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bbf2e0e80b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9bbf2e0e80b

Comment 8

[(Away)]

Visual Studio 2013 RTM has been released, and it still contains the bug, so we'll need to keep this workaround.

Comment 9

[(Away)]

MS says this bug has been fixed for the next major compiler release. Sounds like 2013 will remain affected, though.