[Bug tapsets/13721] New: local variable name collision

[Bug tapsets/13721] New: local variable name collision

[dsmith at redhat dot com]

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

             Bug #: 13721
           Summary: local variable name collision
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tapsets
        AssignedTo: systemtap@sourceware.org
        ReportedBy: dsmith@redhat.com
    Classification: Unclassified


Defining a global array with the name of 'addr' fails:

=======
# stap -vp4 -e 'global addr; probe begin { addr[AF_INET()] = "addr" }'
Pass 1: parsed user script and 81 library script(s) using
200892virt/22856res/2812shr kb, in 140usr/40sys/170real ms.
semantic error: inconsistent arity (1 vs 0): identifier 'addr' at
/usr/local/share/systemtap/tapset/nfs_proc.stp:72:2
        source:     addr = &@cast(cl_xprt, "rpc_xprt", "kernel:sunrpc")->addr
                    ^
semantic error: arity 1 first inferred here: identifier 'addr' at <input>:1:28
        source: global addr; probe begin { addr[AF_INET()] = "addr" }
                                           ^
semantic error: inconsistent arity (1 vs 0): identifier 'addr' at
/usr/local/share/systemtap/tapset/nfs_proc.stp:77:12
        source:     if (@cast(addr, "sockaddr_in")->sin_family != %{ /* pure */
AF_INET %}) {
                              ^
semantic error: inconsistent arity (1 vs 0): identifier 'addr' at
/usr/local/share/systemtap/tapset/nfs_proc.stp:81:15
        source:     return @cast(addr, "sockaddr_in")->sin_addr->s_addr
                                 ^
Pass 2: analyzed script: 1 probe(s), 42 function(s), 1 embed(s), 1 global(s)
using 437688virt/122488res/8384shr kb, in 1560usr/340sys/1927real ms.
Pass 2: analysis failed.  Try again with another '--vp 01' option.
=======

This is happening because the port_from_xprt() function has a local variable
named 'addr' that is somehow clashing with the global 'addr' variable which has
a different type.  Evidently this happens before the port_from_xprt() function
gets optimized away.

This can be worked around by prepending '__' to the name of the
port_from_xprt() local variable.

There are probably other local function variables in the tapset that could
clash with user variables.

(The error message could perhaps use an improvement.  The word 'arity' isn't a
common one and probably doesn't translate well.)

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

[Bug tapsets/13721] local variable name collision

[jistone at redhat dot com]

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

Josh Stone <jistone at redhat dot com> changed:

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

--- Comment #1 from Josh Stone <jistone at redhat dot com> 2012-02-21 22:30:29 UTC ---
Ugh -- this is a common problem in languages without explicit declaration.  We
have declaration of globals, but we leave locals implicit.  So here you
actually are creating the global 'addr' just fine, but then port_from_xprt() is
binding to that same global, rather than its usual local variable.

Note that splitting these global/local instances based on conflicting type is
not enough, since they could accidentally have the same type and still be
wrongly associated.  To really solve this, I think we'd need an (optional)
explicit local declaration, and then use that consistently throughout the
tapsets.

For comparision, consider Python, where variables are bound to the scope in
which they are written.  To write to a variable in an outer scope, one must
explicitly declare this using the "global" or "nonlocal" keywords.  Reading
variables works implicitly with the innermost scope that has bound that name.

It would be too big a change for us to adopt Python semantics now, but we could
resolve the current ambiguity in a similar way with a "local" keyword.

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

[Bug tapsets/13721] local variable name collision

[jistone at redhat dot com]

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

--- Comment #2 from Josh Stone <jistone at redhat dot com> 2012-02-21 22:37:19 UTC ---
Another idea: we could reduce the scope of what "global" means.  For tapset
globals, we probably still want them available to user scripts.  But in
reverse, we could restrict user globals to the scope of the user script only.

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

[Bug tapsets/13721] local variable name collision

[dsmith at redhat dot com]

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

--- Comment #3 from David Smith <dsmith at redhat dot com> 2012-02-23 15:46:20 UTC ---
I'd probably prefer adding the "local" keyword as opposed to reducing the scope
of what "global" means.  The latter seems more surprising.

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

[Bug tapsets/13721] local variable name collision

[jistone at redhat dot com]

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

--- Comment #4 from Josh Stone <jistone at redhat dot com> 2012-02-23 17:18:18 UTC ---
(In reply to comment #3)
> I'd probably prefer adding the "local" keyword as opposed to reducing the scope
> of what "global" means.  The latter seems more surprising.

It seems to me that any tapset that relied on a global from the user script
would be prone to bugs.  i.e. if the user forgot to define the global, then the
tapset would implicitly use a local instead, with quite different behavior.  So
my intention is that restricting global in this way, that user globals are only
available to the user's script, actually has a DWIM flavor -- it's unlikely
that a tapset should ever mean to reference a user's global.  But if the
consensus is that this is too weird or surprising, so be it.

A "local" keyword will do the trick, but I imagine it will make authoring more
tedious.  We end up more verbose, and any time the "local" is forgotten, that
becomes a latent bug in the tapset waiting for a name clash.

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

[Bug tapsets/13721] local variable name collision

[dsmith at redhat dot com]

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

--- Comment #5 from David Smith <dsmith at redhat dot com> 2012-02-23 18:28:03 UTC ---
(In reply to comment #4)
> (In reply to comment #3)
> > I'd probably prefer adding the "local" keyword as opposed to reducing the scope
> > of what "global" means.  The latter seems more surprising.
> 
> It seems to me that any tapset that relied on a global from the user script
> would be prone to bugs.  i.e. if the user forgot to define the global, then the
> tapset would implicitly use a local instead, with quite different behavior.  So
> my intention is that restricting global in this way, that user globals are only
> available to the user's script, actually has a DWIM flavor -- it's unlikely
> that a tapset should ever mean to reference a user's global.  But if the
> consensus is that this is too weird or surprising, so be it.
> 
> A "local" keyword will do the trick, but I imagine it will make authoring more
> tedious.  We end up more verbose, and any time the "local" is forgotten, that
> becomes a latent bug in the tapset waiting for a name clash.

Hmm, I see your point about forgetting a local and being right back where we
are now.

(One advantage to 'local' that I just thought of is that a user could use it in
his own script where reducing the scope of what "global" means doesn't help a
user in his own script.)

But, on balance perhaps reducing "global"s scope make more sense.

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

[Bug tapsets/13721] local variable name collision

[jistone at redhat dot com]

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

--- Comment #6 from Josh Stone <jistone at redhat dot com> 2012-02-29 03:02:38 UTC ---
I discovered that bug #10799 and commit 2e526da intended to solve (or at least
notify about) this very issue.  However, the testcase here was failing on the
arity mismatch before this warning was ever checked.  I just pushed commit
2a7153bf to reorder this, so now you'll get a "WARNING: cross-file global
variable reference" at least before the arity stuff.

The idea of reduced global scope may be as simple as turning that warning into
an error, although that would also make cross-tapset access fail.  (Do we
care?)

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

[Bug tapsets/13721] local variable name collision

[mcermak at redhat dot com]

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

Martin Cermak <mcermak at redhat dot com> changed:

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

--- Comment #7 from Martin Cermak <mcermak at redhat dot com> ---
Reported collision can now be avoided by using the 'private' keyword. Commit
efe0a0a test-covers this scenario.

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

[Bug tapsets/13721] local variable name collision

[mcermak at redhat dot com]

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

Martin Cermak <mcermak at redhat dot com> changed:

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

--- Comment #8 from Martin Cermak <mcermak at redhat dot com> ---
Marking as resolved per the above comment.

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