We need to add stapdyn to the testsuite! This may include specific tests for stapdyn, but most of the runtime functionality is supposed to work identically, so we should also reuse as many of those tests as we can. Perhaps start with a couple helpers in lib/systemtap.exp, answering (a) is dyninst enabled? and (b) what are all enabled --runtimes? Then tests which are generally applicable / runtime-agnostic can loop over that runtime list.
test
Created attachment 6640 [details] Alpha (non-working) patch. Here's a patch that implements a 'dyninst_p' function and a function that returns the list of supported runtimes. However, the testcase that has been modified to use the new functions doesn't work, for several reasons (some listed in comments in the patch): - The dyninst runtime requires a target executable. Finding an appropriate one is tricky (on 32-bit x86 at least), and I'm having trouble with redirection. The latter is probably a tcl problem. - Since end probes aren't currently working with the dyninst runtime, the test scripts themselves are failing (since the map_hash.exp testcase uses the auto end variable printing feature). Comments on the general approach would still be appreciated.
I've also been looking at modifying testsuite/systemtap.pass1-4/buildok.exp. We've got 2 main problems here: - Several of the test files buildok.exp operates on in testsuite/buildok/ aren't really systemtap scripts but shell scripts (i.e. they start with "#! /bin/sh"). I don't see any way of passing in '--runtime=dyninst' to those testcases. The affected test files are: testsuite/buildok/cmdline01.stp:#!/bin/sh testsuite/buildok/fortytwo.stp:#! /bin/sh testsuite/buildok/oldlocals01.stp:#! /bin/sh testsuite/buildok/scsi-detailed.stp:#! /bin/sh testsuite/buildok/thirtythree.stp:#! /bin/sh testsuite/buildok/thirtytwo.stp:#! /bin/sh testsuite/buildok/two.stp:#! /bin/sh - The builok.exp testcase uses 'stap_run_batch' (from testsuite/lib/systemtap.exp). You can pass as many arguments to that function as you like, but it only looks at the first arg (as a filename). So, passing in the extra "--runtime=dyninst" argument will mean some changes to 'stap_run_batch'. (This might be a problem for systemtap.server/client.exp, since it tries to pass extra args to 'stap_run_batch'.)
(In reply to comment #2) > Created attachment 6640 [details] > Alpha (non-working) patch. > > Here's a patch that implements a 'dyninst_p' function and a function that > returns the list of supported runtimes. > > However, the testcase that has been modified to use the new functions doesn't > work, for several reasons (some listed in comments in the patch): > > - The dyninst runtime requires a target executable. Finding an appropriate one > is tricky (on 32-bit x86 at least), and I'm having trouble with redirection. > The latter is probably a tcl problem. From your patch: +# FIXME: For dyninst, we currently have to have a target executable +# (even though the script doesn't actually need one). A first attempt +# was something simple like '/bin/true'. However, on x86 that gets +# you: +# +# arch-x86.C[4377]: invalid immediate size 0 in insn +# symtab.C[1848]: failed to getAnnotations here [main] +# symtab.C[1881]: failed to getAnnotations here [main] These sorts of errors should be fixed now in dyninst.git, and I'll be updating Fedora packages with a new snapshot soon. (both in official F18/rawhide and in http://repos.fedorapeople.org/repos/jistone/dyninst/ for F16/17) +# Sigh. So, we'll use 'stap' instead. Notice we've got to get rid of +# stap's output, to avoid confusing expect. +# +# stap_runtimes_run2 $srcdir/$subdir/$test -c "stap -V > /dev/null 2>&1" +# +# Sigh again. The above doesn't work, I get "ambiguous redirect". +# +# Here's another stab: +stap_runtimes_run2 $srcdir/$subdir/$test -c "/bin/sh < /dev/null" +# Sigh, sigh, sigh. the above doesn't work either - FAIL. Curious. Do these loads work for normal kernel-mode stap? I wonder if your space before /dev/null is confusing something? Another problem is that because of the redirects, stapio would usually back off and run these indirectly as sh -c '...'. Even the second sh load would do this to get the redirect. I tried to do this for stapdyn too, not sure how well, but since we aren't doing multiprocess yet, we'll *only* attach to that outer "sh -c" process. Maybe that's ok for what you're doing, but worth noting. > - Since end probes aren't currently working with the dyninst runtime, the test > scripts themselves are failing (since the map_hash.exp testcase uses the auto > end variable printing feature). Ok, well let's just focus on test that don't need "end" for now. :/ > Comments on the general approach would still be appreciated. I worry a little about how often "stap -V" will run for every dyninst_p call - can that memoize itself in a global? Or maybe be set at configure time through a HAVE_DYNINST replacement? (In reply to comment #3) > I've also been looking at modifying testsuite/systemtap.pass1-4/buildok.exp. > We've got 2 main problems here: > > - Several of the test files buildok.exp operates on in testsuite/buildok/ > aren't really systemtap scripts but shell scripts (i.e. they start with "#! > /bin/sh"). I don't see any way of passing in '--runtime=dyninst' to those > testcases. The affected test files are: > > testsuite/buildok/cmdline01.stp:#!/bin/sh > testsuite/buildok/fortytwo.stp:#! /bin/sh > testsuite/buildok/oldlocals01.stp:#! /bin/sh > testsuite/buildok/scsi-detailed.stp:#! /bin/sh > testsuite/buildok/thirtythree.stp:#! /bin/sh > testsuite/buildok/thirtytwo.stp:#! /bin/sh > testsuite/buildok/two.stp:#! /bin/sh They'd have to be modified appropriately, but can't they just use their parameters as $1 or even "$@" in the script? Also, how do you propose filtering kernel-specific buildok tests out? > - The builok.exp testcase uses 'stap_run_batch' (from > testsuite/lib/systemtap.exp). You can pass as many arguments to that function > as you like, but it only looks at the first arg (as a filename). So, passing > in the extra "--runtime=dyninst" argument will mean some changes to > 'stap_run_batch'. (This might be a problem for systemtap.server/client.exp, > since it tries to pass extra args to 'stap_run_batch'.) Do you mean that client.exp is broken now, since its extra args are ignored? Sounds like fixing stap_run_batch is a good idea though.
(In reply to comment #4) > (In reply to comment #2) > > Created attachment 6640 [details] > > Alpha (non-working) patch. > > > > Here's a patch that implements a 'dyninst_p' function and a function that > > returns the list of supported runtimes. > > > > However, the testcase that has been modified to use the new functions doesn't > > work, for several reasons (some listed in comments in the patch): > > > > - The dyninst runtime requires a target executable. Finding an appropriate one > > is tricky (on 32-bit x86 at least), and I'm having trouble with redirection. > > The latter is probably a tcl problem. > > From your patch: > +# FIXME: For dyninst, we currently have to have a target executable > +# (even though the script doesn't actually need one). A first attempt > +# was something simple like '/bin/true'. However, on x86 that gets > +# you: > +# > +# arch-x86.C[4377]: invalid immediate size 0 in insn > +# symtab.C[1848]: failed to getAnnotations here [main] > +# symtab.C[1881]: failed to getAnnotations here [main] > > These sorts of errors should be fixed now in dyninst.git, and I'll be updating > Fedora packages with a new snapshot soon. (both in official F18/rawhide and in > http://repos.fedorapeople.org/repos/jistone/dyninst/ for F16/17) Good deal, I'll check out the new packages. > +# Sigh. So, we'll use 'stap' instead. Notice we've got to get rid of > +# stap's output, to avoid confusing expect. > +# > +# stap_runtimes_run2 $srcdir/$subdir/$test -c "stap -V > /dev/null 2>&1" > +# > +# Sigh again. The above doesn't work, I get "ambiguous redirect". > +# > +# Here's another stab: > +stap_runtimes_run2 $srcdir/$subdir/$test -c "/bin/sh < /dev/null" > +# Sigh, sigh, sigh. the above doesn't work either - FAIL. > > Curious. Do these loads work for normal kernel-mode stap? I wonder if your > space before /dev/null is confusing something? > > Another problem is that because of the redirects, stapio would usually back off > and run these indirectly as sh -c '...'. Even the second sh load would do this > to get the redirect. I tried to do this for stapdyn too, not sure how well, > but since we aren't doing multiprocess yet, we'll *only* attach to that outer > "sh -c" process. Maybe that's ok for what you're doing, but worth noting. I'm sure the errors I was getting here with my redirection efforts are tcl quoting problems, since redirections like that work fine from the command line. > > - Since end probes aren't currently working with the dyninst runtime, the test > > scripts themselves are failing (since the map_hash.exp testcase uses the auto > > end variable printing feature). > > Ok, well let's just focus on test that don't need "end" for now. :/ > > > Comments on the general approach would still be appreciated. > > I worry a little about how often "stap -V" will run for every dyninst_p call - > can that memoize itself in a global? Or maybe be set at configure time through > a HAVE_DYNINST replacement? Running 'stap -V' too often could slow things down a bit. We could cache the fact whether dyninst is enabled in setup_systemtap_environment() (which is run once I believe) in an environment variable, then check that environment variable in dyninst_p(). > (In reply to comment #3) > > I've also been looking at modifying testsuite/systemtap.pass1-4/buildok.exp. > > We've got 2 main problems here: > > > > - Several of the test files buildok.exp operates on in testsuite/buildok/ > > aren't really systemtap scripts but shell scripts (i.e. they start with "#! > > /bin/sh"). I don't see any way of passing in '--runtime=dyninst' to those > > testcases. The affected test files are: > > > > testsuite/buildok/cmdline01.stp:#!/bin/sh > > testsuite/buildok/fortytwo.stp:#! /bin/sh > > testsuite/buildok/oldlocals01.stp:#! /bin/sh > > testsuite/buildok/scsi-detailed.stp:#! /bin/sh > > testsuite/buildok/thirtythree.stp:#! /bin/sh > > testsuite/buildok/thirtytwo.stp:#! /bin/sh > > testsuite/buildok/two.stp:#! /bin/sh > > They'd have to be modified appropriately, but can't they just use their > parameters as $1 or even "$@" in the script? Hmm, I hadn't thought of that I'll give it a shot. > Also, how do you propose filtering kernel-specific buildok tests out? I'm about to post a new version of buildok.exp. > > - The builok.exp testcase uses 'stap_run_batch' (from > > testsuite/lib/systemtap.exp). You can pass as many arguments to that function > > as you like, but it only looks at the first arg (as a filename). So, passing > > in the extra "--runtime=dyninst" argument will mean some changes to > > 'stap_run_batch'. (This might be a problem for systemtap.server/client.exp, > > since it tries to pass extra args to 'stap_run_batch'.) > > Do you mean that client.exp is broken now, since its extra args are ignored? > Sounds like fixing stap_run_batch is a good idea though. It is possible that client.exp is broken. I think I've fixed stap_run_batch, and I'll see what that does to client.exp.
Created attachment 6643 [details] buildok testsuite patch Here's a patch that modifies testsuite/systemtap.pass1-4/buildok.exp to work for the default (linux) runtime and the dyninst runtime. However, now that I've done it, I don't really like it. There are about 18 lines of shared tcl code, and the rest (225 lines) is runtime-specific tcl code. This runtime-specific tcl code sets up kfails for each runtime. I think it might make more sense to leave buildok.exp alone, and have a separate dyninst-buildok.exp testcase. Obviously splitting the testcase makes it much easier to just test one specific runtime. Comments welcome.
Created attachment 6646 [details] buildok dyninst test Here's a buildok testcase that just tests the dyninst runtime individually. I'm liking it better than the combined linux and dyninst buildok testcase in the last patch.
Commit 8c94efa modifies the systemtap.pass1-4/*.exp tests to also include dyninst. Most of the cases are modified like this: ==== foreach runtime [get_runtime_list] { if {$runtime != ""} { # run test, add '--runtime=$runtime' } else { # run test } } === Each testcase (where it makes sense) will need to be modified to add dyninst support. Hopefully if we get more runtimes the testsuite will accommodate them much easier. There are 2 remaining obstacles to getting the testsuite working well with dyninst: 1) Dyninst outputs some debug messages that interferes with the testsuite output matching. Lines like: Setting bs state to 1 found target /usr/lib/libpthread-2.16.so 0x7074 found target /usr/lib/libpthread-2.16.so 0x7985 found target /usr/lib/libpthread-2.16.so 0x7a3d found target /usr/lib/libpthread-2.16.so 0x6aed 2) Dyninst requires a test executable. Now that probing /bin/true works, this one isn't a "must have", but it would be nice to fix this one.
(In reply to comment #8) > Commit 8c94efa modifies the systemtap.pass1-4/*.exp tests to also include > dyninst. > > Most of the cases are modified like this: > > ==== > foreach runtime [get_runtime_list] { > if {$runtime != ""} { > # run test, add '--runtime=$runtime' > } else { > # run test > } > } > === Must we have the if-else duplicated in every test? Why can't get_runtime_list return the full option already prepared? If it helps, we could be explicit about the default - instead of "" go ahead and spell out "--runtime=kernel". Side question: in tapset/ and runtime/ subdirectories I chose linux/ for our traditional kernel mode. Do you think the --runtime option should do the same? Or perhaps both --runtime=kernel and --runtime=linux should be accepted as aliases? > Each testcase (where it makes sense) will need to be modified to add dyninst > support. Hopefully if we get more runtimes the testsuite will accommodate them > much easier. > > There are 2 remaining obstacles to getting the testsuite working well with > dyninst: > > 1) Dyninst outputs some debug messages that interferes with the testsuite > output matching. Lines like: > > Setting bs state to 1 This line is indeed from dyninst. It's already been removed upstream, but that was after I made my latest rpm snapshot. > found target /usr/lib/libpthread-2.16.so 0x7074 > found target /usr/lib/libpthread-2.16.so 0x7985 > found target /usr/lib/libpthread-2.16.so 0x7a3d > found target /usr/lib/libpthread-2.16.so 0x6aed These are from stapdyn. I really need to clean these up and hide them behind -v flags. If they're causing testing trouble, that's more reason for me to do this sooner than later. > 2) Dyninst requires a test executable. Now that probing /bin/true works, this > one isn't a "must have", but it would be nice to fix this one. Part of the metadata rework I'm doing involves dlopen'ing the module in stapdyn. With that loaded, we could easily call the init/exit functions directly when there's no target. In fact, once we get multiprocess and shared memory going, we'll probably always call init/exit this way. So while targetless stapdyn has no practical use in general, the sort of toy scripts used in testing are important enough that we should make it work.
(In reply to comment #9) > (In reply to comment #8) > > Commit 8c94efa modifies the systemtap.pass1-4/*.exp tests to also include > > dyninst. > > > > Most of the cases are modified like this: > > > > ==== > > foreach runtime [get_runtime_list] { > > if {$runtime != ""} { > > # run test, add '--runtime=$runtime' > > } else { > > # run test > > } > > } > > === > > Must we have the if-else duplicated in every test? Why can't get_runtime_list > return the full option already prepared? If it helps, we could be explicit > about the default - instead of "" go ahead and spell out "--runtime=kernel". Originally I had something like the following: ==== foreach runtime [get_runtime_list] { if {$runtime != ""} { args = "--runtime=$runtime" } else { args = "" } # run test, passing $args } === But, I had problems with passing the empty arg. Stap actually got an empty arg, which confused it. I went back and forth on whether 'get_runtime_list' should return the full option. I went the current solution because I append the runtime name to the test name. That means you get passes/fails that look like this: === FAIL: buildok/string-embedded.stp (dyninst) FAIL: buildok/system-embedded.stp (dyninst) === That output looked a bit better to me, but I could be argued the other way. As far as doing something like "--runtime=linux", I'm unsure. One possible problem here would be with comparing old test results (since we'd be changing the all the test names). > Side question: in tapset/ and runtime/ subdirectories I chose linux/ for our > traditional kernel mode. Do you think the --runtime option should do the same? > Or perhaps both --runtime=kernel and --runtime=linux should be accepted as > aliases? > > > Each testcase (where it makes sense) will need to be modified to add dyninst > > support. Hopefully if we get more runtimes the testsuite will accommodate them > > much easier. > > > > There are 2 remaining obstacles to getting the testsuite working well with > > dyninst: > > > > 1) Dyninst outputs some debug messages that interferes with the testsuite > > output matching. Lines like: > > > > Setting bs state to 1 > > This line is indeed from dyninst. It's already been removed upstream, but that > was after I made my latest rpm snapshot. > > > found target /usr/lib/libpthread-2.16.so 0x7074 > > found target /usr/lib/libpthread-2.16.so 0x7985 > > found target /usr/lib/libpthread-2.16.so 0x7a3d > > found target /usr/lib/libpthread-2.16.so 0x6aed > > These are from stapdyn. I really need to clean these up and hide them behind > -v flags. If they're causing testing trouble, that's more reason for me to do > this sooner than later. Glad to hear this is being worked on. > > 2) Dyninst requires a test executable. Now that probing /bin/true works, this > > one isn't a "must have", but it would be nice to fix this one. > > Part of the metadata rework I'm doing involves dlopen'ing the module in > stapdyn. With that loaded, we could easily call the init/exit functions > directly when there's no target. In fact, once we get multiprocess and shared > memory going, we'll probably always call init/exit this way. > > So while targetless stapdyn has no practical use in general, the sort of toy > scripts used in testing are important enough that we should make it work. Glad to hear this is being worked on also.
(In reply to comment #10) > > So while targetless stapdyn has no practical use in general, the sort of toy > > scripts used in testing are important enough that we should make it work. > > Glad to hear this is being worked on also. Commit fe3f046 should let basic begin/end/error scripts work without a command.
(In reply to comment #10) > > > 1) Dyninst outputs some debug messages that interferes with the testsuite > > > output matching. Lines like: > > > > > > Setting bs state to 1 > > > > This line is indeed from dyninst. It's already been removed upstream, but that > > was after I made my latest rpm snapshot. As mentioned, this is fixed in dyninst.git HEAD, and Fedora's 0.26 snapshot should now have this too. > > > found target /usr/lib/libpthread-2.16.so 0x7074 > > > found target /usr/lib/libpthread-2.16.so 0x7985 > > > found target /usr/lib/libpthread-2.16.so 0x7a3d > > > found target /usr/lib/libpthread-2.16.so 0x6aed > > > > These are from stapdyn. I really need to clean these up and hide them behind > > -v flags. If they're causing testing trouble, that's more reason for me to do > > this sooner than later. > > Glad to hear this is being worked on. stap commit 41300e6d cleaned this up.