% stap -p4 -vv -e 'probe kernel.function("sys_open") {current = 2}' -u fails: /var/tmp/stapvN2tok/stap_a7fd41cd8eb3d886b2ce6c104c151d60_840.c:102: error: function declaration isn’t a prototype /var/tmp/stapvN2tok/stap_a7fd41cd8eb3d886b2ce6c104c151d60_840.c:102: error: field ‘get_current’ declared as a function /var/tmp/stapvN2tok/stap_a7fd41cd8eb3d886b2ce6c104c151d60_840.c: In function ‘probe_1751’: /var/tmp/stapvN2tok/stap_a7fd41cd8eb3d886b2ce6c104c151d60_840.c:129: error: lvalue required as left operand of assignment /var/tmp/stapvN2tok/stap_a7fd41cd8eb3d886b2ce6c104c151d60_840.c:133: error: lvalue required as left operand of assignment Local variables in the context should be mangled similarly as we do for globals - prefixed with l_/s_, to avoid namespace collisions with common kernel symbols.
For script-level names, we can certainly mangle however we like, but how will this work with embedded-C functions? function inc:long(current:long) %{ THIS->__retvalue = THIS->current + 1; %} Do we want to mangle that too and break the current syntax of THIS? Or do we make a special case that embedded functions aren't mangled?
If we decide to mangle parameters to embedded-C too, then we might want to add ARG(foo) to PR10300.
*** Bug 13434 has been marked as a duplicate of this bug. ***
There are effectively four possible behaviours for systemtap with respect to local variables: - current behaviour: locals are not mangled. The tapset writer has to take care to avoid collisions. - platonically ideal behaviour: locals are all prefixed with "l_" to avoid collisions. - Josh Stone's suggested compromise: locals are mangled in script functions, but not in embedded-C functions. Fairly straightforward, though any embedded-C expressions that access locals *must* be rewritten (if I understand correctly). - another possibility: some ugly preprocessor hacking is used to temporarily #define foo l_foo at the beginning of each embedded C block for any locals foo used within the block; then restore any old definition that foo might have had at the end. This lets us allow use of *either* mangled or unmangled names in all embedded code. Then we can gradually transition over to mangled names, deprecate the unmangled usage, and then take the ugly preprocessor hack back out (if we want). The specific hack I have in mind here requires #pragma pushdef/popdef, which is only available as of GCC 4.4.7. This whole mess of possibilities definitely needs to be hidden behind an ARG(foo) macro. It'll be necessary to do some more mulling over of how we want to do this, especially in terms of compatibility with old gcc versions, how quickly we can deprecate the old behaviour, any special backwards-compatibility options we want to enable, etc.
(In reply to comment #4) > There are effectively four possible behaviours for systemtap with respect to > local variables: > > - current behaviour: locals are not mangled. The tapset writer has to take care > to avoid collisions. Yes, but it's not just tapset writers, rather *anybody* writing stap script. Even comment 0 is showing just a command-line script triggering problems. > - platonically ideal behaviour: locals are all prefixed with "l_" to avoid > collisions. > > - Josh Stone's suggested compromise: locals are mangled in script functions, > but not in embedded-C functions. Fairly straightforward, though any embedded-C > expressions that access locals *must* be rewritten (if I understand correctly). I don't think we want embedded-C expressions to use locals at all - do we have any in-tree cases that do so? IMO this should be discouraged, if not outright forbidden. The problem with that is akin to asm("") blocks, but those have the benefit of input/output constraints. We have no idea which locals are used within an embedded-C block, so right now we assume none, and our optimization passes act accordingly. > - another possibility: some ugly preprocessor hacking is used to temporarily > #define foo l_foo at the beginning of each embedded C block for any locals foo > used within the block; then restore any old definition that foo might have had > at the end. This lets us allow use of *either* mangled or unmangled names in > all embedded code. Then we can gradually transition over to mangled names, > deprecate the unmangled usage, and then take the ugly preprocessor hack back > out (if we want). The specific hack I have in mind here requires #pragma > pushdef/popdef, which is only available as of GCC 4.4.7. I think this is fragile. Imagine if the name is something like "do", "unsigned", or "void". Each of these would break in pretty uninteresting ways if #defined to l_foo, but I worry what someone with more than a few minutes might come up with. > This whole mess of possibilities definitely needs to be hidden behind an > ARG(foo) macro. Even if we leave embedded-c functions alone for now, I like the idea of starting to abstract them into ARG(foo) so we are more flexible to change it in the future. In fact, I don't think the l_ prefix should ever be documented to script/tapset writers, rather left as an implementation detail. > It'll be necessary to do some more mulling over of how we want to do this, > especially in terms of compatibility with old gcc versions, how quickly we can > deprecate the old behaviour, any special backwards-compatibility options we > want to enable, etc. I think at least for embedded-C functions, we can just introduce #define ARG(foo) THIS->foo, with a deprecation note that this will be the only available form in the future.
Created attachment 6440 [details] Basic update of translator naming behaviour This is (tentatively speaking) a patch that changes translate.cxx to generate mangled local names. It seems to work well, excepting of course when we use existing code with embedded-C functions. It might be possible to just update all of the tapsets to use some ARG(foo) macro, add a command line flag (or similar) to cause the translator to return to the old mangling behaviour, and that would probably wrap up work on the bug. The other viable alternative is jistone's suggestion of using different mangling behaviour for embedded-C functions versus regular functions. It will be a bit tricky to get this information available at the time when we do mangling in the current design.
Planning to commit a series of patches which make the following incompatible changes: - Instead of using THIS->foo to access a local variable in embedded-C, use STAP_ARG_foo. - Instead of using THIS->__retvalue in embedded-C, use STAP_RETVALUE. - SystemTap internal code which outputs identifiers MUST be careful to go through the appropriate mangling functions defined in translate.h/translate.cxx -- c_localname, c_globalname, c_varname, and similar. These are defined in the unparser object, which can be accessed from most places in the code using session.up->c_localname() or similar. - (The actual internal mangling scheme uses "l_" as a prefix, but this is now considered subject to change arbitrarily and should as a rule not be relied on.) - Code already in the repository will be rewritten to work correctly with the above changes. Use stap --compatible=1.7 or similar to get back the old behaviour which allows using THIS->foo.
The patches are in the repository, and people haven't been complaining. I've also tested to make sure that the mangling scheme can indeed be changed quickly without breaking anything, and that seems to work. Hence, I'm closing the bug. % stap -p4 -vv -e 'probe kernel.function("sys_open") {current = 2}' Systemtap translator/driver (version 1.8/0.153 commit release-1.7-322-g9e1830b + changes) Copyright (C) 2005-2012 Red Hat, Inc. and others This is free software; see the source for copying conditions. enabled features: AVAHI LIBRPM LIBSQLITE3 NSS BOOST_SHARED_PTR TR1_UNORDERED_MAP NLS Created temporary directory "/tmp/stapMCuGLH" Session arch: x86_64 release: 3.3.5-2.fc16.x86_64 Searched: " /usr/share/systemtap/tapset/x86_64/*.stp ", found: 3, processed: 3 Searched: " /usr/share/systemtap/tapset/*.stp ", found: 27, processed: 27 Searched: " /opt/codebase/install/share/systemtap/tapset/x86_64/*.stp ", found: 4, processed: 4 Searched: " /opt/codebase/install/share/systemtap/tapset/*.stp ", found: 81, processed: 81 Pass 1: parsed user script and 115 library script(s) using 234048virt/56356res/2924shr/54216data kb, in 200usr/10sys/307real ms. Attempting to extract kernel debuginfo build ID from /lib/modules/3.3.5-2.fc16.x86_64/build/vmlinux.id focused on module 'kernel' = [0xffffffff81000000-0xffffffff823b8000, bias 0 file /usr/lib/debug/lib/modules/3.3.5-2.fc16.x86_64/vmlinux ELF machine |x86_64 (code 62) probe sys_open@fs/open.c:997 kernel reloc=.dynamic pc=0xffffffff811807f0 WARNING: Eliding assignment to current at operator '=' at <input>:1:44 WARNING: Eliding side-effect-free expression : identifier 'current' at <input>:1:36 source: probe kernel.function("sys_open") {current = 2} ^ WARNING: side-effect-free probe 'probe_9261': keyword at :1:1 source: probe kernel.function("sys_open") {current = 2} ^ Pass 2: analyzed script: 1 probe(s), 0 function(s), 0 embed(s), 0 global(s) using 404984virt/179476res/92704shr/87364data kb, in 350usr/20sys/366real ms. function recursion-analysis: max-nesting 0 non-recursive probe kernel.function("sys_open@fs/open.c:997") locks nothing dump_unwindsyms kernel index=0 base=0xffffffff81000000 Found build-id in kernel, length 20, start at 0xffffffff816004b4 Pass 3: translated to C into "/tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761_src.c" using 404984virt/179608res/92836shr/87364data kb, in 10usr/0sys/3real ms. Running env -uARCH -uKBUILD_EXTMOD -uCROSS_COMPILE -uKBUILD_IMAGE -uKCONFIG_CONFIG -uINSTALL_PATH PATH=/usr/bin:/bin:/opt/codebase/install/bin:/home/yyz/smakarov/bin:/home/yyz/smakarov/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/yyz/smakarov/bin make -C /lib/modules/3.3.5-2.fc16.x86_64/build M=/tmp/stapMCuGLH modules ARCH=x86_64 CONFIG_DEBUG_INFO= --no-print-directory -j9 CC [M] /tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761_src.o LD [M] /tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761.o Building modules, stage 2. MODPOST 1 modules CC /tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761.mod.o LD [M] /tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761.ko Spawn waitpid result (0x0): 0 /root/.systemtap/cache/83/stap_835e1ac105d3528f36d608d96109cb8e_761.ko Pass 4: compiled C into "stap_835e1ac105d3528f36d608d96109cb8e_761.ko" in 3210usr/570sys/4103real ms. Cleaning cache, interval reached 87263 s > 30 s. Copying /tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761.ko to /root/.systemtap/cache/83/stap_835e1ac105d3528f36d608d96109cb8e_761.ko Copying /tmp/stapMCuGLH/stap_835e1ac105d3528f36d608d96109cb8e_761_src.c to /root/.systemtap/cache/83/stap_835e1ac105d3528f36d608d96109cb8e_761.c Copying /tmp/stapMCuGLH/stapconf_ae17bb025e3b423cb8eefe4ebb19525c_563.h to /root/.systemtap/cache/ae/stapconf_ae17bb025e3b423cb8eefe4ebb19525c_563.h Running rm -rf /tmp/stapMCuGLH Spawn waitpid result (0x0): 0 Removed temporary directory "/tmp/stapMCuGLH" ... success!
Oops, that should be: [root:/opt/codebase/systemtap]:stap$ stap -p4 -vv -e 'probe kernel.function("sys_open") {current = 2}' -u Systemtap translator/driver (version 1.8/0.153 commit release-1.7-322-g9e1830b + changes) Copyright (C) 2005-2012 Red Hat, Inc. and others This is free software; see the source for copying conditions. enabled features: AVAHI LIBRPM LIBSQLITE3 NSS BOOST_SHARED_PTR TR1_UNORDERED_MAP NLS Created temporary directory "/tmp/stapwiwBse" Session arch: x86_64 release: 3.3.5-2.fc16.x86_64 Searched: " /usr/share/systemtap/tapset/x86_64/*.stp ", found: 3, processed: 3 Searched: " /usr/share/systemtap/tapset/*.stp ", found: 27, processed: 27 Searched: " /opt/codebase/install/share/systemtap/tapset/x86_64/*.stp ", found: 4, processed: 4 Searched: " /opt/codebase/install/share/systemtap/tapset/*.stp ", found: 81, processed: 81 Pass 1: parsed user script and 115 library script(s) using 234048virt/56360res/2924shr/54216data kb, in 210usr/10sys/220real ms. Attempting to extract kernel debuginfo build ID from /lib/modules/3.3.5-2.fc16.x86_64/build/vmlinux.id focused on module 'kernel' = [0xffffffff81000000-0xffffffff823b8000, bias 0 file /usr/lib/debug/lib/modules/3.3.5-2.fc16.x86_64/vmlinux ELF machine |x86_64 (code 62) probe sys_open@fs/open.c:997 kernel reloc=.dynamic pc=0xffffffff811807f0 Pass 2: analyzed script: 1 probe(s), 0 function(s), 0 embed(s), 0 global(s) using 404984virt/179472res/92704shr/87364data kb, in 350usr/10sys/363real ms. function recursion-analysis: max-nesting 0 non-recursive probe kernel.function("sys_open@fs/open.c:997") locks nothing dump_unwindsyms kernel index=0 base=0xffffffff81000000 Found build-id in kernel, length 20, start at 0xffffffff816004b4 Pass 3: translated to C into "/tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805_src.c" using 404984virt/179588res/92820shr/87364data kb, in 0usr/10sys/2real ms. Pass 4: using cached /root/.systemtap/cache/ae/stapconf_ae17bb025e3b423cb8eefe4ebb19525c_563.h Running env -uARCH -uKBUILD_EXTMOD -uCROSS_COMPILE -uKBUILD_IMAGE -uKCONFIG_CONFIG -uINSTALL_PATH PATH=/usr/bin:/bin:/opt/codebase/install/bin:/home/yyz/smakarov/bin:/home/yyz/smakarov/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/yyz/smakarov/bin make -C /lib/modules/3.3.5-2.fc16.x86_64/build M=/tmp/stapwiwBse modules ARCH=x86_64 CONFIG_DEBUG_INFO= --no-print-directory -j9 CC [M] /tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805_src.o LD [M] /tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805.o Building modules, stage 2. MODPOST 1 modules CC /tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805.mod.o LD [M] /tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805.ko Spawn waitpid result (0x0): 0 /root/.systemtap/cache/81/stap_811128a7973f9d21c9aef6247e099d3e_805.ko Pass 4: compiled C into "stap_811128a7973f9d21c9aef6247e099d3e_805.ko" in 710usr/130sys/897real ms. Cleaning cache, interval reached 82 s > 30 s. Copying /tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805.ko to /root/.systemtap/cache/81/stap_811128a7973f9d21c9aef6247e099d3e_805.ko Copying /tmp/stapwiwBse/stap_811128a7973f9d21c9aef6247e099d3e_805_src.c to /root/.systemtap/cache/81/stap_811128a7973f9d21c9aef6247e099d3e_805.c Copying /tmp/stapwiwBse/stapconf_ae17bb025e3b423cb8eefe4ebb19525c_563.h to /root/.systemtap/cache/ae/stapconf_ae17bb025e3b423cb8eefe4ebb19525c_563.h Running rm -rf /tmp/stapwiwBse Spawn waitpid result (0x0): 0 Removed temporary directory "/tmp/stapwiwBse" ... so 'current' doesn't get elided. Still success!