Bug 18462 - macro deprecation
Summary: macro deprecation
Status: RESOLVED FIXED
Alias: None
Product: systemtap
Classification: Unclassified
Component: translator (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Martin Cermak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-29 08:06 UTC by Martin Cermak
Modified: 2015-06-23 06:52 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
patch that seems to work for me (819 bytes, patch)
2015-06-10 17:31 UTC, Martin Cermak
Details | Diff
updated patch (1.84 KB, patch)
2015-06-19 12:24 UTC, Martin Cermak
Details | Diff
updated patch (1.87 KB, patch)
2015-06-22 05:59 UTC, Martin Cermak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Cermak 2015-05-29 08:06:38 UTC
To-be-deprecated tapset functions or probes are being wrapped in %( systemtap_v <= "1.3" %?    function foo() {}   %)

Attempt to do this with library macro results in "parse error: library macro file contains non-@define construct" in pass 1, where no conditionals are being evaluated.

Following is an attempt to solve the issue uncleanly using intentional syntax error:

> =======                                                                          
> @define __int32_compat(arg)                                                     
> %(                                                                              
>     %( systemtap_v <= "2.5" %?                                                  
>         %( CONFIG_64BIT == "y" %?                                               
>             (%{ /* pure */ _stp_is_compat_task() %} ? __int32(@arg) : @arg)        
>         %:                                                                      
>             (__int32(@arg))                                                     
>         %)                                                                      
>     %:                                                                          
>         *** ERROR ***                                                           
>     %)                                                                          
> %)                                                                               
> =======

This appears to be syntactically acceptable, but here, systemtap_v <= "whatever" always effectively evaluates as true (but e.g. "aa" <= "a" would evaluate as false). Not sure whether it's a bug or feature, but certainly not a way to go.

So I think we're missing mechanism for macro deprecation.
Comment 1 Josh Stone 2015-05-29 16:31:35 UTC
(In reply to Martin Cermak from comment #0)
> Attempt to do this with library macro results in "parse error: library macro
> file contains non-@define construct" in pass 1, where no conditionals are
> being evaluated.

I think it would be perfectly reasonable to relax this constraint, to allow preprocessor conditionals at the top level of macro files.
Comment 2 Martin Cermak 2015-06-08 10:43:16 UTC
(In reply to Josh Stone from comment #1)
> I think it would be perfectly reasonable to relax this constraint, to allow
> preprocessor conditionals at the top level of macro files.

If I'm not missing something, this isn't going be a oneliner. My impression is that parser::scan_pp1() will need to get extended in a way that it understands conditionals. Which essentially means to duplicate parts of logic implemented in parser::scan_pp(), or resp. in eval_pp_conditional(), into parser::scan_pp1(). Does it sound reasonably?
Comment 3 Josh Stone 2015-06-08 16:59:48 UTC
Can you just change parse_library_macros to call scan_pp() instead?
Comment 4 Martin Cermak 2015-06-10 17:31:39 UTC
Created attachment 8356 [details]
patch that seems to work for me

(In reply to Josh Stone from comment #3)
> Can you just change parse_library_macros to call scan_pp() instead?

This works in a sense, that the macro declaration surrounded by a conditional, that evaluates to false, gets skipped. Also this doesn't cause any regression in the (whole) testsuite. So far so good.

But when the unwanted macro is being skipped in skip_pp(), scan_pp1() gets called, putting the macro into the pp1_namespace --- so later on the macro gets inadvertently expanded anyway: 

=======
 7.1 S x86_64 # cat /root/systemtap/blah/share/systemtap/tapset/linux/aaa.stpm
%( 1 == 2 %?
@define __mymacro
%(
    "blah"
%)
%)
 7.1 S x86_64 # cat xxx.stp 
probe oneshot {
    println(@__mymacro)
}
 7.1 S x86_64 # gdb -q -args stap -p1 -g xxx.stp 
Reading symbols from /root/systemtap/stap...done.
(gdb) b parse.cxx:484
Breakpoint 1 at 0x43ad3e: file parse.cxx, line 484.
(gdb) r
Starting program: /root/systemtap/stap -p1 -g xxx.stp
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Breakpoint 1, parser::scan_pp1 (this=this@entry=0x7fffffffc490) at parse.cxx:484
484               macrodecl* decl = (pp1_namespace[name] = new macrodecl);
Missing separate debuginfos, use: ... stuff deleted ...
(gdb) bt
#0  parser::scan_pp1 (this=this@entry=0x7fffffffc490) at parse.cxx:484
#1  0x000000000043e318 in parser::skip_pp (this=this@entry=0x7fffffffc490) at parse.cxx:1152
#2  0x000000000043e51e in parser::scan_pp (this=this@entry=0x7fffffffc490) at parse.cxx:1037
#3  0x000000000043f597 in parser::parse_library_macros (this=this@entry=0x7fffffffc490) at parse.cxx:721
#4  0x00000000004407bd in parse_library_macros (s=..., name="/root/systemtap/blah/share/systemtap/tapset/linux/aaa.stpm") at parse.cxx:260
#5  0x0000000000412a47 in passes_0_4 (s=...) at main.cxx:567
#6  0x000000000040f45d in main (argc=<optimized out>, argv=<optimized out>) at main.cxx:1207
(gdb)
=======

So what seems to fix the issue for me is to modify skip_pp() to call newly created scan_pp1_temp(), which behaves like scan_pp1() except of putting stuff into pp1_namespace. Using it, reference to a deprecated macro results in:

=======
 7.1 S x86_64 # stap -p1 -g xxx.stp 
parse error: unknown operator @__mymacro
        saw: operator '@__mymacro' at xxx.stp:2:13
     source:     println(@__mymacro)
                         ^

1 parse error.
Pass 1: parse failed.  [man error::pass1]
 7.1 S x86_64 #
=======

Which looks about fine to me.
Comment 5 Josh Stone 2015-06-11 19:25:49 UTC
(In reply to Martin Cermak from comment #4)
> But when the unwanted macro is being skipped in skip_pp(), scan_pp1() gets
> called, putting the macro into the pp1_namespace --- so later on the macro
> gets inadvertently expanded anyway: 

Good catch!  We definitely don't want that.

A similar problem would be when both the true and false side of a conditional are defining the same macro name -- these should not be considered duplicates since only one is actually defined in use.

> So what seems to fix the issue for me is to modify skip_pp() to call newly
> created scan_pp1_temp(), which behaves like scan_pp1() except of putting
> stuff into pp1_namespace.

Instead of duplicating any scan_pp1 logic, how about just adding a "skip" parameter to short-circuit it when needed.

You have an XXX added to the library macro error - just for your debugging?

Other than that, just need some new tests covering macros within conditionals.
Comment 6 Martin Cermak 2015-06-19 12:24:39 UTC
Created attachment 8375 [details]
updated patch

Should address the above comments.
Comment 7 Josh Stone 2015-06-20 00:53:31 UTC
Please use a true "bool" for the skip parameter.

Why did you make the library_macro_file error say "unexpected end"?  Doesn't t!=0 mean we found something other than @define or preprocessor tokens?
Comment 8 Martin Cermak 2015-06-22 05:59:12 UTC
Created attachment 8380 [details]
updated patch

(In reply to Josh Stone from comment #7)
> Please use a true "bool" for the skip parameter.

Updated it.

> Why did you make the library_macro_file error say "unexpected end"?  Doesn't
> t!=0 mean we found something other than @define or preprocessor tokens?

I attempted to follow error handling in parser::next(), but misunderstood it. Updated the error Message.

Also updated the testcase by pointing SYSTEMTAP_TAPSET to a clean temporary dirctory.
Comment 9 Josh Stone 2015-06-22 18:23:33 UTC
Sorry I didn't notice before, but the comment about the skip parameter needs to be able to stand alone.  It's fine to note the PR for people who want further background, but the inline comment still needs to clarify it directly.  Something like "PR18462 don't catalog preprocessor-disabled macros".  Maybe calling the parameter "ignore_macros" instead of "skip" will help too.

Please test the boolean with "!" instead of "== false".

The error "unexpected preprocessor token" is still odd to me.  AIUI scan_pp() actually scans and does preprocessing until it reaches something which is a normal token.  Maybe just say "unexpected token" then.

The tempdir change is a good idea, but I think you missed removing it after.
Comment 10 Martin Cermak 2015-06-23 06:52:24 UTC
Fixed in commit ed891cf3ae12be8163eb283371d5696c6f7ff1ad