Re: [PATCH] Fix handling of null stap semaphores

[Simon Marchi <simark@simark.ca>]

On 2019-12-30 11:39 p.m., George Barrett wrote:
> On Mon, Dec 30, 2019 at 12:59:30PM -0500, Simon Marchi wrote:
>> Yeah, I know writing a test case is not the most fun part, but it pays in
>> the long run.  I can help if you are not familiar enough with
>> tcl/expect/dejagnu.
> 
> I can stumble through enough to get something ready for review, but there is
> a bit of a hiccup I wanted some advice on: the testing strategy you outlined
> hinges on prepare_for_testing producing PIC executables, but AFAICS this is
> neither done by default nor can I find a facility in the test suite utilities
> to achieve this. Is adding -pie/-fPIC to additional_flags acceptable in this
> instance?

If nothing else is specified, the testsuite produces executables with the default
settings of the toolchain.  If, for a test, you need to force creating a PIE or
a non-PIE, you can pass these flags to gdb_compile & al:

   - pie: Force creation of PIE executables.
   - nopie: Prevent creation of PIE executables.

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/testsuite/lib/gdb.exp;h=8b1ec62c9c21570f3d3cb4c9d3fa0bdce016876e;hb=HEAD#l3686

If there are some differences in how PIE and non-PIE are handled, it can be good
to run the test with both.  This can be achieved relatively easily with a:

  foreach_with_prefix pie { "nopie" "pie" } {
    ...
  }

Here's an example:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/testsuite/gdb.base/break-idempotent.exp;h=96f91c50f906955b289b7ee7dfd65dc7c375e79a;hb=HEAD#l149

> On a related note, I was a bit surprised to discover that the test case
> doesn't actually ever define USE_PROBES since the argument provided to
> stap_test(_no_debuginfo) is `-DUSE_PROBES' instead of
> `additional_flags=-DUSE_PROBES'. This seems like a trivial enough fix to be
> rolled into a single commit, but I was thinking that the fix wouldn't be
> particularly evident from the diff if the -pie flags were added in the same
> patch. Would this be worth splitting into an independent patch?

Yes, totally, a separate patch (ideally before, in the same series) would be perfect.
This shows that the test is written in a poor way, it does not appear to actually
test anything related to semaphores.  At least, I think it should parse the output
of "info probes" and expect an hexadecimal number for the semaphore address.

Good catch, btw.  Also, shouldn't it be called USE_SEMAPHORES and not USE_PROBES?

Thanks for looking into this.

Simon