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).
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?
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.
Created attachment 9612 [details] 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 $ =======
Created attachment 9616 [details] 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().
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.
Created attachment 9619 [details] 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 :)
Comment on attachment 9619 [details] 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.
Created attachment 9622 [details] 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.
We can block that path by using kernel.function("send_sigqueue").{call,inline} But I'm not sure we should...
Created attachment 9630 [details] 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?
Created attachment 9647 [details] possible patch Attached patch solves the above issue by allowing the @defined(@entry($var)) construct in return probes.
Fixed in commit b3b87c605f48045d30f2021026b05e2b86f5a5b0.