[Bug translator/17749] New: stap doesn't recognize "++" as a use

[Bug translator/17749] New: stap doesn't recognize "++" as a use

[tromey at sourceware dot org]

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

            Bug ID: 17749
           Summary: stap doesn't recognize "++" as a use
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: translator
          Assignee: systemtap at sourceware dot org
          Reporter: tromey at sourceware dot org

Consider this script:

global found = 0;
global found_pid = -1;
global sigstop = 19;

probe kernel.trace("signal_deliver") {
  if (pid() == found_pid && $sig == sigstop) {
    printf("%d\n", pid());
    exit();
  }
}

probe kernel.trace("sched_process_exec") {
  if (execname() =~ @1 && ++found == 1) {
    found_pid = pid();
    raise(sigstop);
  }
}

I ran it like so:

pokyo. sudo stap -g preattach.stp '.*ls.*'
29505
found=0x1


It printed out "found=0x1", but I think it should not have.

On irc jlebon pointed out:

<jlebon> normally stap prints written-to but never read-from globals at
     shutdown
<jlebon> I think it doesn't see that it is being read


I modified the script, on his suggestion, to hoist the ++found out of
the condition.  This suppressed the extraneous output.

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[fche at redhat dot com]

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

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

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

--- Comment #1 from Frank Ch. Eigler <fche at redhat dot com> ---
It's a bit more subtle:

 global a probe foo { a++ }    # we'd like a printed at end
 global b probe bar { if (b++ > 5) ... }   # but not b

It's not exactly the ++ operation that's the discriminator
between these two cases, but whether the result is used
an rvalue or whether it's tossed away.

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[jlebon at redhat dot com]

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

Jonathan Lebon <jlebon at redhat dot com> changed:

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

--- Comment #2 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 8066
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8066&action=edit
Possible patch

It seems like the issue is that the varuse_collecting_visitor treats the
following two as equivalent:

  a++ ...
  if (a++) ...

I.e. when 'a++' is visited, it doesn't matter whether it's part of an if
statement or not. In both cases, the 'a' referent is simply added to the 'read'
and 'write' sets but NOT to the 'used' set, causing it to get printed. (The
'used' set contains the vars that are read while not in a rmw context -- this
distinction is necessary because a rmw operation is both a read and a write, so
the rule for global var printing can't just be "all vars written to but never
read" otherwise expr_statements like 'a++' would not trigger auto printing).

Note BTW that this is also an issue with for/while loop conditionals.

The following patch provides a potential fix for this issue. It overrides the
if/for visit methods so that their conditionals are visited as if they were
lvalues (and thus cause the symbols to be added to the 'used' set). This
technique is already used for visit_print_format() and visit_delete_statement()
(see also commit d20a83f8).

I have yet to do a full test run with the patch above, but at the very least
global_end.exp passes. Also thinking about how to clean up this part of the
code to be more explicit.

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[jistone at redhat dot com]

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

Josh Stone <jistone at redhat dot com> changed:

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

--- Comment #3 from Josh Stone <jistone at redhat dot com> ---
On saving last_lvalue_read - it should be a complete surprise to enter
visit_if_statement with current_lvalue_read=true in the first place.  An
if_statement never has a value, so no-one should be indicating that they want
to read it!  In fact, it ought to be initially false for all statement types,
and perhaps this should be asserted.

Anyway, some additional expressions which should always be considered used:
- functioncall->args
- foreach_loop->array_slice
- foreach_loop->limit
- return_statement->value
- ternary_expression->cond
- entry_op->operand (unsure, may be translated away already)

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[jlebon at redhat dot com]

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

Jonathan Lebon <jlebon at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #8066|0                           |1
        is obsolete|                            |

--- Comment #4 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 8078
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8078&action=edit
new patch

Hey Josh,

Thanks for all the extra cases to check. Just also noticed now that there is an
issue with current_lvalue_read and functioncalls.

global a

function foo() {
   a++
   return "foo"
}

probe oneshot {
   println(foo())
}

The script above interprets the a++ in foo() as used because visit_print_format
sets current_lvalue_read to true.

The following patch fixes the above as well as all the cases mentioned so far.

Here is my testing script:

global a // if condition
global b // function arg

global arr // used in foreach
global c // foreach array slice
global d // foreach limit

global e // return value
global f // ternary cond
global g // while condition

function foo(b) {
   if (b)
      println("b")
}

function bar() {
   return ++e;
}

probe oneshot {

   if (++a)
      println("a")

   foo(++b)

   arr[1,1] = 1
   foreach ([i,j] in arr[*,++c] limit ++d) {
      println("c")
      println("d")
   }

   if (bar()) {
      println("e")
   }

   if (++f ? 1 : 0) // can only be tested without the patch for if_statement
applied
      println("f")

   while (!g++)
      println("g")
}

Without patch applied:

a
b
c
d
e
f
g
a=0x1
b=0x1
c=0x1
d=0x1
e=0x1
f=0x1
g=0x2

With patch applied:

a
b
c
d
e
f
g

(Not being printed automatically indicates that it was added to the used set).
I will doublecheck that all spots have been covered and run it through the
tester.

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[jlebon at redhat dot com]

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

Jonathan Lebon <jlebon at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #8066|0                           |1
        is obsolete|                            |

--- Comment #4 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 8078
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8078&action=edit
new patch

Hey Josh,

Thanks for all the extra cases to check. Just also noticed now that there is an
issue with current_lvalue_read and functioncalls.

global a

function foo() {
   a++
   return "foo"
}

probe oneshot {
   println(foo())
}

The script above interprets the a++ in foo() as used because visit_print_format
sets current_lvalue_read to true.

The following patch fixes the above as well as all the cases mentioned so far.

Here is my testing script:

global a // if condition
global b // function arg

global arr // used in foreach
global c // foreach array slice
global d // foreach limit

global e // return value
global f // ternary cond
global g // while condition

function foo(b) {
   if (b)
      println("b")
}

function bar() {
   return ++e;
}

probe oneshot {

   if (++a)
      println("a")

   foo(++b)

   arr[1,1] = 1
   foreach ([i,j] in arr[*,++c] limit ++d) {
      println("c")
      println("d")
   }

   if (bar()) {
      println("e")
   }

   if (++f ? 1 : 0) // can only be tested without the patch for if_statement
applied
      println("f")

   while (!g++)
      println("g")
}

Without patch applied:

a
b
c
d
e
f
g
a=0x1
b=0x1
c=0x1
d=0x1
e=0x1
f=0x1
g=0x2

With patch applied:

a
b
c
d
e
f
g

(Not being printed automatically indicates that it was added to the used set).
I will doublecheck that all spots have been covered and run it through the
tester.

--- Comment #5 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 8079
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8079&action=edit
new patch

Let me try that again as a diff for bugzilla.

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[fche at redhat dot com]

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

--- Comment #6 from Frank Ch. Eigler <fche at redhat dot com> ---
Is the complementary case already well-covered by tests, ie
those where a++ etc. exist in various contexts that are not
meant to trigger the 'do-not-print-because-it's-used'
heuristic?

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[jlebon at redhat dot com]

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

--- Comment #7 from Jonathan Lebon <jlebon at redhat dot com> ---
(In reply to Frank Ch. Eigler from comment #6)
> Is the complementary case already well-covered by tests, ie
> those where a++ etc. exist in various contexts that are not
> meant to trigger the 'do-not-print-because-it's-used'
> heuristic?

It's probably sort of tested indirectly, but I will make a testcase testing
both sides explicitly.

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

[Bug translator/17749] stap doesn't recognize "++" as a use

[jlebon at redhat dot com]

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

Jonathan Lebon <jlebon at redhat dot com> changed:

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

--- Comment #8 from Jonathan Lebon <jlebon at redhat dot com> ---
Also found the same issue with arrayindex->indexes.

Fixed in commit f24849b.
Added subtest to global_end.exp in commit 7077306.

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