[PATCH] Add function name and file to semantic error messages for not found local variables.

[PATCH] Add function name and file to semantic error messages for not found local variables.

[Przemysław Pawełczyk]

Hi,

Few weeks ago I found out SystemTap and from that time I'm slowly
discovering its features. I've already joined discussion at
#systemtap.
I have a patch analogous to Mark Wielaard's commit 194c6687 (Add
function name and file to semantic error messages for $return).

Some background:

His patch was a side-effect of question about my simple script:
stap -e 'global errors = 0; probe syscall.*.return {if ($return < 0)
errors++} probe end {printf("Syscalls with errors: %d\n", errors);
delete errors}'
because I said that analysis had failed on compat_sys_recvmsg. Mark
noticed this also, but he asked me how I had found such information
and I pointed at stap -vv and comparing pc. Mark gave me also a link
to http://sourceware.org/bugzilla/show_bug.cgi?id=5890, which he
commented.

Day before that I had other problem, caused by script:
stap -e 'probe syscall.* {printf("%s(%s)\n", probefunc(), argstr)}' -c 'echo'
and Mark helped me then also (explanation:
http://sourceware.org/bugzilla/show_bug.cgi?id=5434#c2).
Couple of days later, writing strace-like script
(http://research.pawelczyk.it/systemtap/strace.stp) on clean systemtap
I spotted, that more meaningful error message in mentioned case would
be useful as well. I put some info on channel, but probably it was
overlooked. Now I'm trying more formal way.

If I forgot about something, please let me know. It's my first message here.


diff --git a/tapsets.cxx b/tapsets.cxx
index 5acf50c..fd81927 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -1690,6 +1690,8 @@ struct dwflpp
 	print_locals (scopes, alternatives);
 	throw semantic_error ("unable to find local '" + local + "'"
 			      + " near pc " + lex_cast_hex<string>(pc)
+			      + " for " + dwarf_diename (scope_die)
+			      + "(" + dwarf_diename (cu) + ")"
 			      + (alternatives.str() == "" ? "" : (" (alternatives:" +
alternatives.str () + ")")));
       }


-- 
Przemysław Pawełczyk

Re: [PATCH] Add function name and file to semantic error messages for not found local variables.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

Hi Przemysław,

On Tue, 2008-11-11 at 01:57 +0100, Przemysław Pawełczyk wrote:
> Couple of days later, writing strace-like script
> (http://research.pawelczyk.it/systemtap/strace.stp) on clean systemtap
> I spotted, that more meaningful error message in mentioned case would
> be useful as well. I put some info on channel, but probably it was
> overlooked. Now I'm trying more formal way.
> 
> If I forgot about something, please let me know. It's my first message here.
> 
> diff --git a/tapsets.cxx b/tapsets.cxx
> index 5acf50c..fd81927 100644
> --- a/tapsets.cxx
> +++ b/tapsets.cxx
> @@ -1690,6 +1690,8 @@ struct dwflpp
>  	print_locals (scopes, alternatives);
>  	throw semantic_error ("unable to find local '" + local + "'"
>  			      + " near pc " + lex_cast_hex<string>(pc)
> +			      + " for " + dwarf_diename (scope_die)
> +			      + "(" + dwarf_diename (cu) + ")"
>  			      + (alternatives.str() == "" ? "" : (" (alternatives:" +
> alternatives.str () + ")")));
>        }

Thanks. I like patches that improve our error messages.

In this case we cannot always just print the diename of the scope we are
looking at since that might be NULL (see just above in the function). In
that case we are just looking for the scope by address. And while we are
improving the error message, lets also add the same for the other error
case just above it. So I think we want something like the attached. Does
that work for you?

Cheers,

Mark

[-- Attachment #2: find_variable_and_frame_base.patch --]
[-- Type: text/x-patch, Size: 899 bytes --]

diff --git a/tapsets.cxx b/tapsets.cxx
index 8d371a8..f3b6d3f 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -1677,6 +1677,9 @@ struct dwflpp
       {
 	throw semantic_error ("unable to find any scopes containing "
 			      + lex_cast_hex<string>(pc)
+			      + ((scope_die == NULL) ? ""
+				 : (string (" in ") + dwarf_diename (scope_die)
+			      	    + "(" + dwarf_diename (cu) + ")"))
 			      + " while searching for local '" + local + "'");
       }
 
@@ -1690,6 +1693,9 @@ struct dwflpp
 	print_locals (scopes, alternatives);
 	throw semantic_error ("unable to find local '" + local + "'"
 			      + " near pc " + lex_cast_hex<string>(pc)
+			      + ((scope_die == NULL) ? ""
+				 : (string (" in ") + dwarf_diename (scope_die)
+			      	    + "(" + dwarf_diename (cu) + ")"))
 			      + (alternatives.str() == "" ? "" : (" (alternatives:" + alternatives.str () + ")")));
       }
 

Re: [PATCH] Add function name and file to semantic error messages for not found local variables.

[Przemysław Pawełczyk]

> In this case we cannot always just print the diename of the scope we are
> looking at since that might be NULL (see just above in the function). In
> that case we are just looking for the scope by address. And while we are
> improving the error message, lets also add the same for the other error
> case just above it. So I think we want something like the attached. Does
> that work for you?

Yes, I didn't noticed NULL problem and unconsciously omitted second
error message in this function, so your patch is definitely better
(and works, of course).
Here it was a rather silly mistake (because of the precedent code, as
you mentioned), but yet I must say that dataflow in systemtap is
(currently) somewhat cryptic to me (I didn't spend enough time looking
into it and sorry for this). I mean e.g. it's not obvious (to me)
where scope_die in tapsets.cxx is not NULL for sure. However, that is
my problem, no systemtap's.

Regards.

-- 
Przemysław Pawełczyk

Re: [PATCH] Add function name and file to semantic error messages for not found local variables.

[Mark Wielaard]

On Tue, 2008-11-11 at 13:06 +0100, Przemysław Pawełczyk wrote:
> > In this case we cannot always just print the diename of the scope we are
> > looking at since that might be NULL (see just above in the function). In
> > that case we are just looking for the scope by address. And while we are
> > improving the error message, lets also add the same for the other error
> > case just above it. So I think we want something like the attached. Does
> > that work for you?
> 
> Yes, I didn't noticed NULL problem and unconsciously omitted second
> error message in this function, so your patch is definitely better
> (and works, of course).

Thanks for testing. Committed as:

2008-11-11  Przemysław Pawełczyk <przemyslaw@pawelczyk.it>
            Mark Wielaard <mjw@redhat.com>

    * tapsets.cxx (find_variable_and_frame_base): Add scope name to
    semantic error messages if available.

> Here it was a rather silly mistake (because of the precedent code, as
> you mentioned), but yet I must say that dataflow in systemtap is
> (currently) somewhat cryptic to me (I didn't spend enough time looking
> into it and sorry for this). I mean e.g. it's not obvious (to me)
> where scope_die in tapsets.cxx is not NULL for sure. However, that is
> my problem, no systemtap's.

I admit to be confused myself how the precise dataflow is at times. In
this case the NULL check just above this call was the hint. The
INTERNALS file has some hints. But most information does indeed come
from trying to read and track the code by hand.

Cheers,

Mark