848592 - Eliminate bogus valgrind warnings in dynamic rooting analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Mark memory accessed during dynamic analysis as defined

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 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Steve Fink [:sfink] [:s:]]

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

[Steve Fink [:sfink] [:s:]]

Attachment: Mark memory accessed by dynamic rooting analysis as defined

Comment 5

[Nicholas Nethercote [inactive]]

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)

Comment 6

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55fa2277a702

Comment 7

[Steve Fink [:sfink] [:s:]]

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

Comment 8

[Terrence Cole [:terrence]]

Attachment: fix scanned=false failures

Comment 9

[Terrence Cole [:terrence]]

https://tbpl.mozilla.org/?tree=Try&rev=625e7ed412dd

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Make EvaluateUCInStackFrame take a StableCharPtr to avoid hazards

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

Comment 11

[Terrence Cole [:terrence]]

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 12

[Steve Fink [:sfink] [:s:]]

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.

Comment 13

[Terrence Cole [:terrence]]

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

Comment 14

[Terrence Cole [:terrence]]

And backed out because fixing the verifier causes more failures on inbound than locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8066ebe02e0b

Comment 15

[Terrence Cole [:terrence]]

The crashes are opt-only.

Comment 16

[Terrence Cole [:terrence]]

Attachment: v0

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.

Comment 17

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/039f52615a8f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/039f52615a8f