[Bug translator/14924] New: warn on complex $ptr->foo expressions in .return probes

[Bug translator/14924] New: warn on complex $ptr->foo expressions in .return probes

[fche at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=14924

             Bug #: 14924
           Summary: warn on complex $ptr->foo expressions in .return
                    probes
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: translator
        AssignedTo: systemtap@sourceware.org
        ReportedBy: fche@redhat.com
    Classification: Unclassified


Some users are confused by stap's automatic function-call entry-time
snapshotting of $foo->bar->baz expressions in .return probes.  While it's
probably not a good idea to change the behavior (maybe not even with
--compatible), we could emit a warning, referencing @entry() (see also bug
#14437).

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

Martin Cermak <mcermak at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcermak at redhat dot com

--- Comment #1 from Martin Cermak <mcermak at redhat dot com> ---
Do I get it right that the gist of the confusion is the situation described in
PR5899 (with a solution noted in comment #4)?  If so, is printing a warning
when @entry() is called on something that involves dereference (".*->.*"), the
right thing to do?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

Josh Stone <jistone at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jistone at redhat dot com

--- Comment #2 from Josh Stone <jistone at redhat dot com> ---
If we make it a warning, then anyone who wants to be strict (stap -W) will have
to use @entry($foo->bar) whenever that entry-time snapshotting is really what
they want.  I think that will be annoying to those who are used to the current
behavior.

Perhaps this needs something softer than a warning, like a note.

FWIW, autocast should make ($foo)->bar possible, for a nicer workaround than
the suggestion involving explicit @cast.  Since this breaks up the expression,
only the part inside the parentheses will be entry-saved.  Or perhaps that's
even more confusing, that $foo->bar and ($foo)->bar have different behavior. 
Plus, it doesn't yet work for @entry($foo)->bar -- see PR18579.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #3 from Martin Cermak <mcermak at redhat dot com> ---
Created attachment 9612
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9612&action=edit
working version of a patch

Funnily, I'm still a bit lost in this problem.  Attached patch demonstrates
where I am now ;-)  But it does something:

=======
$ stap  -e 'probe process("./a.out").function("set2").return { println($y->x)
}' -c './a.out > /dev/null' -p2 > /dev/null
WARNING: WARN WARN WARN
$ stap  -e 'probe process("./a.out").function("set2").return {
println(@entry($y->x)) }' -c './a.out > /dev/null' -p2 > /dev/null
$
=======

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[fche at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

Frank Ch. Eigler <fche at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fche at redhat dot com

--- Comment #4 from Frank Ch. Eigler <fche at redhat dot com> ---
Created attachment 9616
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9616&action=edit
possible patch

A more blunt but not crazy approach, which warns on any

  probe FOO.return { $var }

usage.  We could suppress this for tapsets and/or switch them over to @entry().

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #5 from Josh Stone <jistone at redhat dot com> ---
If you're going to make a broad warning, please do switch the tapsets to share
in the users' pain.  (i.e. don't suppress the warning -- dogfood it)

But again, remember PR18579 that autocast @entry($foo)->bar doesn't work,
whereas it does work with implicit entry-value saving.  Maybe this can be
partially kludged by special-casing @entry on a plain target value.

IIRC, @entry doesn't use kretprobe pouches either, for the similar reason that
the expression type may not be fully known yet.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #6 from Martin Cermak <mcermak at redhat dot com> ---
Created attachment 9619
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9619&action=edit
possible patch

(In reply to Frank Ch. Eigler from comment #4)
> Created attachment 9616 [details]
> possible patch
> 
> A more blunt but not crazy approach, which warns on any
> 
>   probe FOO.return { $var }
> 

So the above would warn whenever the autosaving feature gets used.  What if the
warning message would only pop up in case a dereference is involved within the
expression in question?  Like in $foo->bar->baz per Comment#0 ?  The attached
patch does that.  But of course, it's simply extendable to apply to any target
$var in a .return probe.

Also an "info" output/log level might be introduced as an analogy to "warning"
[session.print_warning()] with an option to turn it off, or make it a regular
warning?  Or maybe better yet, it might be turned off by default with an option
to turn it on on demand - like additional hints if the user wants it.  Just
thoughts.  Too verbose.  Sorry :)

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[fche at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #7 from Frank Ch. Eigler <fche at redhat dot com> ---
Comment on attachment 9619
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9619
possible patch

Nice.  IMHO, we should warn even for plain  .return {$var}  constructs, for
people have found that confusing too, expecting it to be return-time-current
values.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #8 from Martin Cermak <mcermak at redhat dot com> ---
Created attachment 9622
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9622&action=edit
possible patch

(In reply to Frank Ch. Eigler from comment #7)
> IMHO, we should warn even for plain  .return {$var}  constructs

Attached patch does that. I've -p2'd all the return probes and it looks fine
except one problem: For example:

Probe signal.send.return is only defined in the tapset for systemtap_v <=
"2.1".  So, when a user runs modern stap without --compatible, and her script
refers to probe signal.send.return, then this probe gets constructed using
probe signal.send, which actually is defined in the tapset. But since it is an
"entry" probe used as a "return" probe, the translator shows misleading warning
like this:

=======
$ stap --poison-cache  -e "probe signal.send.return {exit()}"  -p2 >/dev/null
WARNING: confusing usage, consider @entry($t) in .return probe: identifier '$t'
at /usr/local/share/systemtap/tapset/linux/signal.stp:76:28
 source:     task = @choose_defined($t, $p)
[ ... stuff deleted ... ]
=======

Let's list the linux/signal.stp:76 neighbourhood:

=======
  73 probe __signal.send.send_sigqueue = kernel.function("send_sigqueue")       
  74 {                                                                          
  75     name = "send_sigqueue"                                                 
  76     task = @choose_defined($t, $p)                                         
  77     sig = @choose_defined($q->info->si_signo, $sig)                        
  78     sinfo = @choose_defined($q->info, 0)                                   
  79     shared = 0                                                             
  80 %( systemtap_v <= "2.1" %?                                                 
  81     send2queue = 1                                                         
  82 %)                                                                         
  83 }
=======

So, that's misleading, since adding @entry() there would break the probe.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #9 from Josh Stone <jistone at redhat dot com> ---
We can block that path by using kernel.function("send_sigqueue").{call,inline}

But I'm not sure we should...

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #10 from Martin Cermak <mcermak at redhat dot com> ---
Created attachment 9630
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9630&action=edit
possible patch

First round of full testing and fixes.  ITSM there is now a slight syntactical
issue with @defined in return probes:  What was earlier easily expressible like
e.g. this:

=======
        if (@defined($s)) {
                bytes_req = $s->size
                ...
        }
        else
        ...
=======

does now probably need following handling:

=======
        if (@entry(@defined($s))) {
                bytes_req = @entry(@choose_defined($s->size, 0))
                ...
        }
        else
        ...
=======

I'm not sure I like this, because it's not syntactically terse enough, and it
requires a "default" value which possibly can cause collisions.  Thoughts?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

--- Comment #11 from Martin Cermak <mcermak at redhat dot com> ---
Created attachment 9647
  --> https://sourceware.org/bugzilla/attachment.cgi?id=9647&action=edit
possible patch

Attached patch solves the above issue by allowing the @defined(@entry($var))
construct in return probes.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/14924] warn on complex $ptr->foo expressions in .return probes

[mcermak at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=14924

Martin Cermak <mcermak at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Martin Cermak <mcermak at redhat dot com> ---
Fixed in commit b3b87c605f48045d30f2021026b05e2b86f5a5b0.

-- 
You are receiving this mail because:
You are the assignee for the bug.