491162 - Dehydra sometimes misses statement variables

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Humphrey (:humph)]

Given:

void f() {
  int *x, *z;
  int y;
  if (x + y > z) {}
}

I expect line 4 |if (x + y > z) {}| to contain 3 variables, but I only get x and z, missing y completely:

{
  statements: [{
    name: "x",
    type: #2#,
    loc: {
      _source_location: 213,
      file: "/tmp/tmpCNehpQ.cpp",
      line: 2,
      column: 8
    }
  },
  {
    name: "z",
    type: #2#,
    loc: {
      _source_location: 217,
      file: "/tmp/tmpCNehpQ.cpp",
      line: 2,
      column: 12
    }
  }],
  loc: {
    _source_location: 464,
    file: "/tmp/tmpCNehpQ.cpp",
    line: 4,
    column: 3
  }

Comment 1

[(dormant account)]

Attachment: fix

Pretty sure this is the first logic flaw(ie where things don't crash, but the wrong thing happens) discovered in dehydra so far...Would've been hard to spot without your testcase.

Can you please do a dxr run with this to see if there are any crashes created by this? This change should expose a bit of code that wasn't previously looked at, hopefully that wont expose new bugs.

Comment 2

[David Humphrey (:humph)]

Comment on attachment 384514 [details] [diff] [review]
fix

I don't think this is right.  As I said on irc, building with this patch gives me 5700 fewer total statements.  I had a look to see what the differences where, and it's strange.

On the one hand, this works such that dehydra now sees the i in modules/libpref/src/nsPref.cpp:592:

prefName = (char *)childArray[i];

Before it only saw prefName and childArray.

However, I lose things that used to be seen.  For example, in modules/libpref/src/prefapi.cpp:420 I used to see length twice, and now I only get it once:

PL_strncpy(return_buffer, stringVal, PR_MIN((size_t)*length - 1, PL_strlen(stringVal) + 1)); 

So perhaps the count of stmts is not reliable, and you've killed off a bug that was over-reporting things?

Not sure what to make of it on the face of the few example diffs I dug into.

Comment 3

[(dormant account)]

(In reply to comment #2)

> So perhaps the count of stmts is not reliable, and you've killed off a bug that
> was over-reporting things?
> 

That's indeed the case. Isn't that a feature?

Comment 4

[David Humphrey (:humph)]

Well, I assumed I'd see more, not fewer stmts on the whole, so this surprises me, yes.  But if it's right, that's cool.  The overall count is meaningless.

Another example, this time modules/libjar/nsZipArchive.cpp:1394

  return (PRUint32)( (ll [0] <<  0) |
                     (ll [1] <<  8) |
                     (ll [2] << 16) |
                     (ll [3] << 24) );

Before I got 4 stmts with name=ll.  Now I get 1.  This sort of thing is what's accounting for the difference in total.  This new way of doing it is going to have a bad side effect for me.  Consider the case of:

if ( (a->x + b->x) > (b->x + x) ) {...}

I'll see 3 name=x, but will have no hope of matching them by position in the line.  Currently I keep a tally of position, knowing that I'd see 4 name=x.

So this might be more accurate, but it's less like source, which will make the mapping back to source harder.

Comment 5

[(dormant account)]

(In reply to comment #4)
> Well, I assumed I'd see more, not fewer stmts on the whole, so this surprises
> me, yes.  But if it's right, that's cool.  The overall count is meaningless.
> 
> Another example, this time modules/libjar/nsZipArchive.cpp:1394
> 
>   return (PRUint32)( (ll [0] <<  0) |
>                      (ll [1] <<  8) |
>                      (ll [2] << 16) |
>                      (ll [3] << 24) );
> 
> Before I got 4 stmts with name=ll.  Now I get 1.  This sort of thing is what's
> accounting for the difference in total.  This new way of doing it is going to
> have a bad side effect for me.  Consider the case of:
> 
> if ( (a->x + b->x) > (b->x + x) ) {...}
> 
> I'll see 3 name=x, but will have no hope of matching them by position in the
> line.  Currently I keep a tally of position, knowing that I'd see 4 name=x.
> 
> So this might be more accurate, but it's less like source, which will make the
> mapping back to source harder.

Yeah i think the prior structure was accidental. I wonder if there is a better way for me to get you how often things are used.

Basic idea is that gcc lets me "iterate" over tree-like graphs with loops in them. It does that by way of a visited-nodes set to keep self from looping. Before I would start-subiterations more often which ended up showing you some nodes more often.

So I wonder if I can somehow to convince GCC to skip that logic for variables.

Comment 6

[David Humphrey (:humph)]

Comment on attachment 384514 [details] [diff] [review]
fix

Given the discussion above, I think this behaviour is probably the one we want.  My issue with missing multiple occurences of a var in the same line is a future issue (i.e., I don't rely on it yet, since the loc.column data is bogus on it and I have to fudge that).  Let's take this and deal with multiple foo per line when we get further along.

Comment 7

[(dormant account)]

(In reply to comment #6)
> (From update of attachment 384514 [details] [diff] [review])
> Given the discussion above, I think this behaviour is probably the one we want.
>  My issue with missing multiple occurences of a var in the same line is a
> future issue (i.e., I don't rely on it yet, since the loc.column data is bogus
> on it and I have to fudge that).  Let's take this and deal with multiple foo
> per line when we get further along.

Ok good. File a bug once you cross that bridge and we'll try to find a solution.
http://hg.mozilla.org/users/tglek_mozilla.com/dehydra-gcc/rev/fcb7be71e586