Closed Bug 920318 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)




(2 files)

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:

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.
Attachment #809568 - Flags: review?(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.
Attachment #809568 - Flags: review?(luke) → review+
The uninitialized data appears to be read in the pos() call here:

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

(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.
Closed: 6 years ago
Resolution: --- → FIXED
Visual Studio 2013 RTM has been released, and it still contains the bug, so we'll need to keep this workaround.
MS says this bug has been fixed for the next major compiler release. Sounds like 2013 will remain affected, though.
Depends on: 1289987
You need to log in before you can comment on or make changes to this bug.