[Bug translator/16165] New: @perf operator in error msgs is corrupted

[Bug translator/16165] New: @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

            Bug ID: 16165
           Summary: @perf operator in error msgs is corrupted
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: translator
          Assignee: systemtap at sourceware dot org
          Reporter: jlebon at redhat dot com

The following command:

$ stap -e '
global x

probe perf.hw.cpu_cycles.process("/home/yyz/jlebon/towers.x").counter("a") {} 

probe process("/home/yyz/jlebon/towers.x").function("main")
{
   # wrong type
   x = "b" . @perf("a")
}
'

gives as semantic error this:

semantic error: type mismatch: expected string but found long: identifier 'a'
at /home/yyz/jlebon/perf.stp:8:14
        source:    x = "b" . aperf("a")
                             ^

Pass 2: analysis failed.  [man error::pass2]

Notice the corruption from '@' to 'a'.

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

--- Comment #1 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7274
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7274&action=edit
only color token if it matches source exactly

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

--- Comment #2 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7275
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7275&action=edit
use original content when expanding perf_op

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

--- Comment #3 from Jonathan Lebon <jlebon at redhat dot com> ---
This is due to the @perf operator getting expanded to the counter symbol (see
dwarf_var_expanding_visitor::visit_perf_op()). The issue is that the symbol's
token content is set to the literal value passed to the @perf operator.

What then happens is that when we try to color the offending token, the token's
content ('a') doesn't match the original source line ('@perf').

There are (at least) two possible ways to fix this:
Patch 1. Only color stuff whose contents would match exactly the source line.
Patch 2. Make sure the new token the expanded perf symbol uses has the original
content.

Not sure what's the right way. Patch 1 is more general in that other things of
this sort will not be colored. On the other hand, patch 2 allows the '@perf'
keyword to get colored, but I'm not entirely sure what other repercussions
replacing the content may have (and whether this is something done in other
places).

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jistone at redhat dot com]

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

Josh Stone <jistone at redhat dot com> changed:

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

--- Comment #4 from Josh Stone <jistone at redhat dot com> ---
(In reply to Jonathan Lebon from comment #3)
> This is due to the @perf operator getting expanded to the counter symbol
> (see dwarf_var_expanding_visitor::visit_perf_op()). The issue is that the
> symbol's token content is set to the literal value passed to the @perf
> operator.

I think this is what should be fixed - I don't see why it should create a new
token at all; just reuse the incoming perf_op's tok.

In fact, if we could make it so *no one* outside parse.cxx creates new tokens,
I think that would be best.  The only other time I know this is currently
necessary is when synthesizing printfs, because print_format uses tok->content
directly.  Fix that, and I think you could restrict token::token to parse.cxx
for good.

> What then happens is that when we try to color the offending token, the
> token's content ('a') doesn't match the original source line ('@perf').
> 
> There are (at least) two possible ways to fix this:
> Patch 1. Only color stuff whose contents would match exactly the source line.
> Patch 2. Make sure the new token the expanded perf symbol uses has the
> original content.
> 
> Not sure what's the right way. Patch 1 is more general in that other things
> of this sort will not be colored. On the other hand, patch 2 allows the
> '@perf' keyword to get colored, but I'm not entirely sure what other
> repercussions replacing the content may have (and whether this is something
> done in other places).

Patch 3. Don't make a new token: @perf will get colored with no repercussions.
:)

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

--- Comment #5 from Jonathan Lebon <jlebon at redhat dot com> ---
(In reply to Josh Stone from comment #4)
> Patch 3. Don't make a new token: @perf will get colored with no
> repercussions. :)

Thanks. Fixed in commit 4653caf.

> In fact, if we could make it so *no one* outside parse.cxx creates new
> tokens, I think that would be best.  The only other time I know this is
> currently necessary is when synthesizing printfs, because print_format uses
> tok->content directly.  Fix that, and I think you could restrict
> token::token to parse.cxx for good.

Leaving this bug report open to track the printf issue.

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

--- Comment #6 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7278
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7278&action=edit
extend print_format::create()

Tentative patch to get rid of the token creation syndrome outside of parse.cxx.
We basically extend print_format::create to accept a string denoting the type
of print statement we want, rather than always deriving it from the token's
content.

I will be testing this with the testsuite tonight.

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

--- Comment #7 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7281
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7281&action=edit
make constructors private

Testsuite came back clean. As far as I can tell, only the parser and lexer
classes create tokens now. This patch makes the token constructors private and
only friends with the above classes.

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

[Bug translator/16165] @perf operator in error msgs is corrupted

[jlebon at redhat dot com]

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

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> ---
Merged into master in commits 1c922ad and a340930.

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