If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Dehydra sometimes misses statement variables

RESOLVED FIXED

Status

()

Core
Rewriting and Analysis
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: humph, Assigned: (dormant account))

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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
  }
(Assignee)

Comment 1

8 years ago
Created attachment 384514 [details] [diff] [review]
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.
Attachment #384514 - Flags: review?(david.humphrey)
(Reporter)

Comment 2

8 years ago
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.
Attachment #384514 - Flags: review?(david.humphrey) → review-
(Assignee)

Comment 3

8 years ago
(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?
(Reporter)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
(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.
(Reporter)

Comment 6

8 years ago
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.
Attachment #384514 - Flags: review- → review+
(Assignee)

Comment 7

8 years ago
(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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.