The rooting analysis reads uninitialized memory on the stack, even more than the conservative scanner. This triggers a large spew of warnings.

This fixes one real uninitialized access to the 'scanned' field of Rooted<>. It also marks the stack as defined, and *temporarily* marks found addresses as defined. If this is more generally useful, it probably ought to be defined somewhere else, I suppose.

Comment on attachment 721938 [details] [diff] [review] Mark memory accessed during dynamic analysis as defined Review of attachment 721938 [details] [diff] [review]: ----------------------------------------------------------------- The basic approach looks ok, but there were enough things I want changed that I'd like to see a second version, please. ::: js/src/gc/Verifier.cpp @@ +45,5 @@ > ThingRootKind kind; > }; > > +template <size_t N> > +struct AutoVGDefine { |AutoMarkDefinedForValgrind| would be a better name. This is an obscure thing, give people a chance to understand it! A comment explaining what it's doing would also help. @@ +46,5 @@ > }; > > +template <size_t N> > +struct AutoVGDefine { > + uintptr_t *mPtr; |ptr_| is more typical JS style than |mPtr|. @@ +47,5 @@ > > +template <size_t N> > +struct AutoVGDefine { > + uintptr_t *mPtr; > + char mDefined[(N+1) / 8]; Nope. There is 1 V bit per actual data bit. So you need |mDefined[N]|. (And if it was 1 V bit per data *byte*, you would have wanted +7 instead of +1 anyway.) @@ +49,5 @@ > +struct AutoVGDefine { > + uintptr_t *mPtr; > + char mDefined[(N+1) / 8]; > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) It's not clear what the 1 means here. Please add a comment indicating that 1 is returned if (a) Valgrind is running and (b) the call succeeded. @@ +50,5 @@ > + uintptr_t *mPtr; > + char mDefined[(N+1) / 8]; > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > + mPtr = NULL; Instead of using mPtr to record failure, please use a bool member with a name like |getVbitsSucceeded|. @@ +51,5 @@ > + char mDefined[(N+1) / 8]; > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > + mPtr = NULL; > + VALGRIND_MAKE_MEM_DEFINED(ptr, N); Mixing |ptr| and |mPtr| within the function body is confusing. Just use |mPtr|. @@ +52,5 @@ > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > + mPtr = NULL; > + VALGRIND_MAKE_MEM_DEFINED(ptr, N); > + } If you parameterize the template by the type instead of the type's size, it'll be clearer, and you'll be able to use |T *| instead of |uintptr_t *|. Please do that.

(In reply to Nicholas Nethercote [:njn] from comment #2) > Comment on attachment 721938 [details] [diff] [review] > Mark memory accessed during dynamic analysis as defined > > Review of attachment 721938 [details] [diff] [review]: > ----------------------------------------------------------------- > > The basic approach looks ok, but there were enough things I want changed > that I'd like to see a second version, please. Heh. I really should've marked this f? instead of r?, but this worked out fine for me. I was just doing this because I'm trying to figure out why ion behavior is varying between runs, and nbp wanted me to check whether it's depending on undefined memory. There are far too many false positives on a root analysis run to see clearly, but I don't want to suppress valid reports by marking everything the stack points to as defined. > > ::: js/src/gc/Verifier.cpp > @@ +45,5 @@ > > ThingRootKind kind; > > }; > > > > +template <size_t N> > > +struct AutoVGDefine { > > |AutoMarkDefinedForValgrind| would be a better name. This is an obscure > thing, give people a chance to understand it! Ok, done. Do you think this is worth hoisting up to somewhere more general? eg, should we be using something like this when we do a conservative stack scan? Right now we're unconditionally marking the stack as defined (in both the conservative scanner and the dynamic analysis scanner). > A comment explaining what it's doing would also help. Ok, let me know if it's clear enough. > @@ +46,5 @@ > > }; > > > > +template <size_t N> > > +struct AutoVGDefine { > > + uintptr_t *mPtr; > > |ptr_| is more typical JS style than |mPtr|. Yeah, I suppose so. > @@ +47,5 @@ > > > > +template <size_t N> > > +struct AutoVGDefine { > > + uintptr_t *mPtr; > > + char mDefined[(N+1) / 8]; > > Nope. There is 1 V bit per actual data bit. So you need |mDefined[N]|. Ok. I was just wildly guessing. The memcheck.h documentation is a bit vague. > (And if it was 1 V bit per data *byte*, you would have wanted +7 instead of > +1 anyway.) Uh... yeah, right. I was just testing you. Making sure our review process is worthwhile. Yes, that's what it was. > @@ +49,5 @@ > > +struct AutoVGDefine { > > + uintptr_t *mPtr; > > + char mDefined[(N+1) / 8]; > > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > > It's not clear what the 1 means here. Please add a comment indicating that > 1 is returned if (a) Valgrind is running and (b) the call succeeded. I did it in a heavyhanded way instead. > @@ +50,5 @@ > > + uintptr_t *mPtr; > > + char mDefined[(N+1) / 8]; > > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > > + mPtr = NULL; > > Instead of using mPtr to record failure, please use a bool member with a > name like |getVbitsSucceeded|. But... but... that's one whole extra word on the stack! It'll ruin everything! > @@ +51,5 @@ > > + char mDefined[(N+1) / 8]; > > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > > + mPtr = NULL; > > + VALGRIND_MAKE_MEM_DEFINED(ptr, N); > > Mixing |ptr| and |mPtr| within the function body is confusing. Just use > |mPtr|. Well, now that the memshrink guy says I can wastefully bloat up my stack frame by ONE WHOLE WORD, I will. > > @@ +52,5 @@ > > + AutoVGDefine(uintptr_t *ptr) : mPtr(ptr) { > > + if (VALGRIND_GET_VBITS(ptr, mDefined, N) != 1) > > + mPtr = NULL; > > + VALGRIND_MAKE_MEM_DEFINED(ptr, N); > > + } > > If you parameterize the template by the type instead of the type's size, > it'll be clearer, and you'll be able to use |T *| instead of |uintptr_t *|. > Please do that. Ok, though it's a bit of a mismatch with the valgrind API, since it takes a pointer and a size (and so can handle arrays of stuff, not just single items.) But given that we don't need the array version, I'll change it. (I suppose I could do template<typename T, size_t N=1>, but we can always do that when we need it.) I don't know why the stupid compiler wouldn't let me do AutoVGDefine<sizeof(*w)> anyway.

Comment on attachment 722359 [details] [diff] [review] Mark memory accessed by dynamic rooting analysis as defined Review of attachment 722359 [details] [diff] [review]: ----------------------------------------------------------------- Much better, thanks! Yeah, the valgrind docs are vague about GET_VBITS/SET_VBITS -- I checked the Valgrind source code to make sure I was right :) Hoisting it and reusing it elsewhere seems like a good idea. I'll leave that up to you :) ::: js/src/gc/Verifier.cpp @@ +60,5 @@ > + uint8_t defined[sizeof(T)]; > + AutoMarkDefinedForValgrind(T *ptr) > + : ptr_(ptr), active(true) > + { > + if (VALGRIND_GET_VBITS(ptr, defined, sizeof(T)) != VALGRIND_RUNNING_AND_REQUEST_SUCCESSFUL) I'd use |ptr_| in the function body to match the destructor. GET_VBITS and SET_VBITS are obscure (not that that's your fault!) Maybe commenting here something like "the 'v bits' are Valgrind's representation of the value's definedness" would help. @@ +61,5 @@ > + AutoMarkDefinedForValgrind(T *ptr) > + : ptr_(ptr), active(true) > + { > + if (VALGRIND_GET_VBITS(ptr, defined, sizeof(T)) != VALGRIND_RUNNING_AND_REQUEST_SUCCESSFUL) > + active = false; You could just do active = VALGRIND_GET_VBITS(ptr, defined, sizeof(T)) == VALGRIND_RUNNING_AND_REQUEST_SUCCESSFUL)

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2358efab23. Initializing 'scanned' enabled the rooting analysis to find some pre-existing errors.

An even better fix would be to make JS_SetTrap's callback take a Handle-ified string.

Comment on attachment 763868 [details] [diff] [review] Make EvaluateUCInStackFrame take a StableCharPtr to avoid hazards Review of attachment 763868 [details] [diff] [review]: ----------------------------------------------------------------- I'd prefer to avoid adding more ensureStable: these have caused real OOM failures in the past. It's not needed yet; hopefully we'll have something better ready before it is.

Comment on attachment 763858 [details] [diff] [review] fix scanned=false failures Review of attachment 763858 [details] [diff] [review]: ----------------------------------------------------------------- #@$@!! I thought I published this. Anyway, I'm fine with what you have, but if you think my patch is ok, I'd rather do that instead. ::: js/src/jsdbgapi.cpp @@ +1333,5 @@ > const jschar *chars, unsigned length, > const char *filename, unsigned lineno, > MutableHandleValue rval) > { > + SkipRoot skipChars(cx, &chars); I think you still need to root the string in shell/js.cpp's TrapHandler. Which makes me a little nervous that we're hiding problems with SkipRoot. I'll upload my somewhat more complete patch for this and see what you think.

And backed out because fixing the verifier causes more failures on inbound than locally: https://hg.mozilla.org/integration/mozilla-inbound/rev/8066ebe02e0b

The crashes are opt-only.

Opt builds happen to have a different GC timing, so we were hitting another missing SkipRoot. I'll just fold this in to the existing patch when landing.