semantic error: multiple addresses for ...

semantic error: multiple addresses for ...

[Roland McGrath]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1703 bytes --]

This error seems inherently wrong to me.  Multiple matches is a feature,
not an error.

Take the attached source and script.  I'm using gcc-4.4.4-10.fc13.x86_64,
but it probably doesn't matter much.

	$ gcc -g -O1 -o ip1 implicitptr.c
	$ ./run-stap implicitptr.stp ./ip1 -c ./ip1
	semantic error: multiple addresses for implicitptr.c:27 (try implicitptr.c:26 or implicitptr.c:28)
	semantic error: no match while resolving probe point process("/tmp/implicitptr-O1").statement("bar@*+4")
	semantic error: libdw failure (dwarf_entrypc): no matching address range
	semantic error: no match while resolving probe point process("/tmp/implicitptr-O1").statement("add@*+4")
	semantic error: no match while resolving probe point process("/tmp/implicitptr-O1").statement("add@*+7")
	Pass 2: analysis failed.  Try again with another '--vp 01' option.

The line in question is in an inlined function (add) that is inlined twice
into the same caller (bar).  The correct result is two probe locations, one
in each inlined instance.

Frank tells me this error diagnosis was intended for a case where a single
source line got its code spread out throughout the function so that there
would appear to be multiple probe locations for one semantic event.  I have
not seen an example of this.  IMHO, breaking the valid situations of one
source line authentically generating multiple different compiled code
locations that should be probed is far worse than failing to diagnose that
weirder case.

One possibility could be to use the is_stmt state of the line records here.
But in the absence of any of the examples that motivated the current (IMHO
wrong) behavior, it's hard to get very concrete on that.


Thanks,
Roland


[-- Attachment #2: C source for test case --]
[-- Type: text/plain, Size: 553 bytes --]

int
foo (int i)
{
  int *j = &i;
  int **k = &j;
  int ***l = &k;
  i++;
  return i;
}

struct S
{
  int *x, y;
};

int u[6];

static inline void
add (struct S *a, struct S *b, int c)
{
  *a->x += *b->x;
  a->y += b->y;
  u[c + 0]++;
  a = (struct S *) 0;
  u[c + 1]++;
  a = b;
  u[c + 2]++;
}

int
bar (int i)
{
  int j = i;
  struct S p[2] = { { &i, i * 2 }, { &j, j * 2 } };
  add (&p[0], &p[1], 0);
  p[0].x = &j;
  p[1].x = &i;
  add (&p[0], &p[1], 3);
  return i + j;
}

int x = 22;

int
main (void)
{
  x = foo (x);
  x = bar (x);
  return 0;
}

[-- Attachment #3: stap script for test case --]
[-- Type: text/plain, Size: 744 bytes --]

probe process(@1).statement("foo@*:7") /* at i++; */
{
  printf("foo: %d %d %d %d\n",
	 $i, $j[0], $k[0][0], $l[0][0][0]);
  newval = 99;
  $i = newval;
  printf("changed foo (%d): %d %d %d %d\n", newval,
	 $i, $j[0], $k[0][0], $l[0][0][0]);
}

probe process(@1).statement("bar@*+3"), /* before first add */
      process(@1).statement("bar@*+4"), /* after first add */
      process(@1).statement("bar@*+6") /* at the return stmt */
{
  printf("p = { { &%d, %d }, { &%d, %d } }\n",
	 $p[0]->x[0], $p[0]->y, $p[1]->x[0], $p[1]->y);
}

probe process(@1).statement("add@*+4"),/* before a = 0 */
      process(@1).statement("add@*+7") /* after a = b */
{
  printf("*a->x=%d, a->y=%d, *b->x=%d, b->y=%d\n",
  	 $a->x[0], $a->y, $b->x[0], $b->y);
}

Re: semantic error: multiple addresses for ...

[Josh Stone]

On 08/20/2010 01:39 PM, Roland McGrath wrote:
> Frank tells me this error diagnosis was intended for a case where a single
> source line got its code spread out throughout the function so that there
> would appear to be multiple probe locations for one semantic event.

For more history, see commit 897820ca and PR1306.

http://sourceware.org/git/gitweb.cgi?p=systemtap.git;h=897820ca
http://sourceware.org/bugzilla/show_bug.cgi?id=1306

In comment #2, some guy named Roland indicates that this may be a
non-trivial problem in the general case...

Josh

Re: semantic error: multiple addresses for ...

[Roland McGrath]

> For more history, see commit 897820ca and PR1306.
> 
> http://sourceware.org/git/gitweb.cgi?p=systemtap.git;h=897820ca
> http://sourceware.org/bugzilla/show_bug.cgi?id=1306

Thanks for these pointers.

> In comment #2, some guy named Roland indicates that this may be a
> non-trivial problem in the general case...

Indeed it is.  The question is how best to resolve it.  
IMHO the current situation is not good.

Off hand, I see two potential approaches:

1. Pay attention to is_stmt.
   I mentioned this earlier.  To consider this, we'd really need to find
   some cases where the current code legitimately complains as in PR1306,
   and check whether the is_stmt flags in that DWARF info are useful.

2. Apply the current rule only when there are multiple hits in the same scope.
   This seems probably safer than relying on is_stmt, though it's not clear.
   This should fix the case I just reported, and ones like it, where the
   multiple hits are in entirely different instances of the same source code.

   This too could possibly go wrong due to unusual optimization choices,
   but it seems far less likely.


Thanks,
Roland

Re: semantic error: multiple addresses for ...

[Josh Stone]

On 08/24/2010 12:55 PM, Roland McGrath wrote:
> Off hand, I see two potential approaches:
> 
> 1. Pay attention to is_stmt.
>    I mentioned this earlier.  To consider this, we'd really need to find
>    some cases where the current code legitimately complains as in PR1306,
>    and check whether the is_stmt flags in that DWARF info are useful.

I'm not sure how to check is_stmt, but here's a reproducer at a similar
location to the original bug report.

kernel-2.6.33.8-149.fc13.x86_64  fs/open.c:
> 1047 long do_sys_open(int dfd, const char __user *filename, int flags, int mode)
> 1048 {
> 1049         char *tmp = getname(filename);
> 1050         int fd = PTR_ERR(tmp);
> 1051 
> 1052         if (!IS_ERR(tmp)) {
> 1053                 fd = get_unused_fd_flags(flags);
> 1054                 if (fd >= 0) {
> 1055                         struct file *f = do_filp_open(dfd, tmp, flags, mode, 0);
> 1056                         if (IS_ERR(f)) {
> 1057                                 put_unused_fd(fd);
> 1058                                 fd = PTR_ERR(f);
> 1059                         } else {
> 1060                                 fsnotify_open(f->f_path.dentry);
> 1061                                 fd_install(fd, f);
> 1062                         }
> 1063                 }
> 1064                 putname(tmp);
> 1065         }
> 1066         return fd;
> 1067 }

$ stap -L 'kernel.statement("do_sys_open@fs/open.c:1053")'
semantic error: multiple addresses for fs/open.c:1053 (try
fs/open.c:1051 or fs/open.c:1058)


> 2. Apply the current rule only when there are multiple hits in the same scope.
>    This seems probably safer than relying on is_stmt, though it's not clear.
>    This should fix the case I just reported, and ones like it, where the
>    multiple hits are in entirely different instances of the same source code.

Isn't it possible for a line to legitimately occur multiple times in the
same scope?  For example, a statement within an unrolled loop *should*
get multiple probes after all, right?

The funny thing is that the current rule is only applied for statement
probes on line numbers (see the use of need_single_match).  Function
probes with line numbers are allowed to have duplicates, probably for
the exact reason you're complaining about, that there may be multiple
inline instances.


Josh

Re: semantic error: multiple addresses for ...

[Roland McGrath]

> On 08/24/2010 12:55 PM, Roland McGrath wrote:
> > Off hand, I see two potential approaches:
> > 
> > 1. Pay attention to is_stmt.
> >    I mentioned this earlier.  To consider this, we'd really need to find
> >    some cases where the current code legitimately complains as in PR1306,
> >    and check whether the is_stmt flags in that DWARF info are useful.
> 
> I'm not sure how to check is_stmt, but here's a reproducer at a similar
> location to the original bug report.

dwarf_linebeginstatement is the call.  There is no easy way to see it in
readelf output, you have to use eu-readelf --debug-dump=line and grok the
line program by eyeball.  I just added the -F (--flags) option to
eu-addr2line to show that kind of stuff.  It shows:

	$ ./src/addr2line -k -F 0xffffffff810fe83d 0xffffffff810fe849
	fs/open.c:1053 (is_stmt)
	fs/open.c:1053 (is_stmt)

So both line entries do have the is_stmt flag, and it's not telling us
anything useful.  In fact, an eyeball check on --debug-dump=line output
shows that the compiler sets is_stmt to true and never toggles it anywhere.
This reminds me that I think this is something that the GCC folks are
planning to work on in the near future, in fact almost certainly motivated
by similar concerns for other debuggers (i.e. GDB).  (I might have
remembered that from a discussion we happened to have elsewhere in the last
week or two, but I didn't.)  So, not something we can use today.

> Isn't it possible for a line to legitimately occur multiple times in the
> same scope?  For example, a statement within an unrolled loop *should*
> get multiple probes after all, right?

Yes, that's true.  But since currently we are breaking all legitimate
cases, including ones with simple concrete examples, breaking only this
subset of legitimate cases (of which we have no known examples off hand)
is clearly an improvement.

For that situation, each code site would have is_stmt true (even if correctly
tracked).  So, if, at some future date, we can rely on is_stmt flags being
useful, then doing that alone will probably be the right choice.  (The
definition of is_stmt in DWARF actually is exactly "is a recommended
breakpoint location."  It really is intended for precisely what we want.)

What seems far more likely today (and is in my concrete test case, albeit a
synthetic one for unrelated purposes) is to have multiple inlined instances
of the same inline function inside the same actual function.  In this case,
the innermost scope of each is the inlined_subroutine, and their PC ranges
will usually be disjoint.  (Note this should be so even if their code is
interleaved--each will have noncontiguous PC sets that indicate the exact
interleaving.)  

Of course, there are lots of optimization possibilities.  But this is a
heuristic to work with the limited information we have.  Ultimately, the
best solution is for the compiler to decide where the breakpoint for a
source location belongs, and that's exactly what is_stmt is for.  But we
don't have the compiler support for that yet.

The status quo is that we punt all cases to the user to work around
manually.  So any heuristic to reduce the cases we punt is safe as long as
it errs on the side of punting cases that might not be intended to have
multiple matching PCs.  To start with, cases in different innermost scopes
that have disjoint ranges seem safe in this sense.  As we come along other
concrete cases where we are punting when we should probe, we can consider
adding to the heuristic.

> The funny thing is that the current rule is only applied for statement
> probes on line numbers (see the use of need_single_match).  Function
> probes with line numbers are allowed to have duplicates, probably for
> the exact reason you're complaining about, that there may be multiple
> inline instances.

The source location in .function is a very different animal.  It's really
not about finding a particular PC at all.  It's about disambiguating which
source function you meant, if there might be multiple by the same name
(e.g. statics in different CUs of a module) or you use wildcards for some
reason.  So a very different logic applies.  (I'm not saying that what the
translator does today necessarily comports with such a logic as we would
articulate it.  But it should not be a surprise if it's closer to that
ideal logic than to the logic used to resolve .statement, either the ideal
logic for that or the manifest behavior of .statement as it is.)  

The source locations yield PCs, and those yield concrete DIE scopes.  The
innermost subprogram or inlined_subroutine scope is the one that is
relevant to identifying the function.  Each inlined_subroutine is only
relevant in its abstract_origin attribute that leads to a subprogram.  If
that final set of subprogram DIEs is all just the same one subprogram, then
there is no ambiguity in the match.  If you have matches in multiple CUs
and all of them are an identical-looking subprogram (i.e. same name,
signature, and same source location after canonicalizing source file
names), then that too is an unambiguous single match.  In that case, it's a
"single" match of an inline function defined in a header file or something
like that, where getting all the inlined instances across all the CUs is
exactly what's intended (including some that call the source file foo.h and
some that call it ../foo/foo.h, etc.).


Thanks,
Roland

Re: semantic error: multiple addresses for ...

[Frank Ch. Eigler]

Josh Stone <jistone@redhat.com> writes:

> [...]
> The funny thing is that the current rule is only applied for statement
> probes on line numbers (see the use of need_single_match).  Function
> probes with line numbers are allowed to have duplicates, probably for
> the exact reason you're complaining about, that there may be multiple
> inline instances.

That's because, for function probes, we hardly even look at the line
numbers.  We just want to identify some enclosing function into whose
prologue the probe will be placed.

- FChE