start to replace dependence of test suite on gdb with LD_PRELOAD shims

start to replace dependence of test suite on gdb with LD_PRELOAD shims

[David Bremner]

These patches are somewhere in between WIP and proposed for
merging. I'm sure there are some shell scripting tweaks needed, and
it's a potentially disruptive enough change that I want to wait until
after 0.29.  On the other hand, it works for me, and the API seems
like an improvement on what it replaces.  I'm particularly interested
in the portability of this approach.

[PATCH 1/2] test: provide machinery to make and use test_shims

[David Bremner]

These can be used e.g. to override return values for functions, in
place of the existing scripting of gdb.
---
 test/test-lib.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index ff18fae6..a423b7f4 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -95,6 +95,8 @@ TEST_EMACSCLIENT=${TEST_EMACSCLIENT:-emacsclient}
 TEST_GDB=${TEST_GDB:-gdb}
 TEST_CC=${TEST_CC:-cc}
 TEST_CFLAGS=${TEST_CFLAGS:-"-g -O0"}
+TEST_SHIM_CFLAGS=${TEST_SHIM_CFLAGS:-"-fpic -shared"}
+TEST_SHIM_LDFLAGS=${TEST_SHIM_LDFLAGS:-"-ldl"}
 
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
@@ -1032,6 +1034,20 @@ test_C () {
     notmuch_dir_sanitize OUTPUT.stdout OUTPUT.stderr > OUTPUT
 }
 
+make_shim () {
+    base_name="$1"
+    test_file="${base_name}.c"
+    shim_file="${base_name}.so"
+    cat > ${test_file}
+    ${TEST_CC} ${TEST_CFLAGS} ${TEST_SHIM_CFLAGS} -I${NOTMUCH_SRCDIR}/test -I${NOTMUCH_SRCDIR}/lib -o ${shim_file} ${test_file} ${TEST_SHIM_LDFLAGS}
+}
+
+notmuch_with_shim () {
+    base_name="$1"
+    shift
+    shim_file="${base_name}.so"
+    LD_PRELOAD=./${shim_file} notmuch-shared "$@"
+}
 
 # Creates a script that counts how much time it is executed and calls
 # notmuch.  $notmuch_counter_command is set to the path to the
-- 
2.20.1

Re: [PATCH 1/2] test: provide machinery to make and use test_shims

[Tomi Ollila]

On Sun, May 26 2019, David Bremner wrote:

> These can be used e.g. to override return values for functions, in
> place of the existing scripting of gdb.
> ---
>  test/test-lib.sh | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index ff18fae6..a423b7f4 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -95,6 +95,8 @@ TEST_EMACSCLIENT=${TEST_EMACSCLIENT:-emacsclient}
>  TEST_GDB=${TEST_GDB:-gdb}
>  TEST_CC=${TEST_CC:-cc}
>  TEST_CFLAGS=${TEST_CFLAGS:-"-g -O0"}
> +TEST_SHIM_CFLAGS=${TEST_SHIM_CFLAGS:-"-fpic -shared"}
> +TEST_SHIM_LDFLAGS=${TEST_SHIM_LDFLAGS:-"-ldl"}
>  
>  # Protect ourselves from common misconfiguration to export
>  # CDPATH into the environment
> @@ -1032,6 +1034,20 @@ test_C () {
>      notmuch_dir_sanitize OUTPUT.stdout OUTPUT.stderr > OUTPUT
>  }
>  
> +make_shim () {
> +    base_name="$1"
> +    test_file="${base_name}.c"
> +    shim_file="${base_name}.so"
> +    cat > ${test_file}
> +    ${TEST_CC} ${TEST_CFLAGS} ${TEST_SHIM_CFLAGS} -I${NOTMUCH_SRCDIR}/test -I${NOTMUCH_SRCDIR}/lib -o ${shim_file} ${test_file} ${TEST_SHIM_LDFLAGS}
> +}
> +
> +notmuch_with_shim () {
> +    base_name="$1"
> +    shift
> +    shim_file="${base_name}.so"
> +    LD_PRELOAD=./${shim_file} notmuch-shared "$@"

LD_PRELOAD=./${shim_file}${LD_PRELOAD:+:$LD_PRELOAD} notmuch-shared "$@"

So that if LD_PRELOAD is already set and is non-empty then ':' and its
old value is appended to new value of LD_PRELOAD before this
notmuch-shared invocation.

Also I'm "insisting" that your patch 2/2 is updated based on
id:8736kipe9f.fsf@fifthhorseman.net -- dkg had good opening how to 
improve robustness in `eval` executions, but since there are quite
a few similar problems in code (i.e. and there are quite a few ways
to improve it) IMO maintaining status quo we can this feature improved
incrementally and choose how to fix *all* related problems...

> +}


--- above comment for this message -- below more on the test robustness
--- part -- I was not supposed to write so long (and should have gone
--- sleep an hour ago -- anyway if you read it all shout your opinion.


One option, which is relatively easy to start using (when needed) while
writing code is to use ${parameter@Q} transformation; 

from bash (4.4+) documentation:

    Q      The  expansion is a string that is the value of parameter
           quoted in a format that can be reused as input.

( that transforms e.g.  foo'bar to 'foo'\''bar' )

Like hinted above, this would require that minimum bash version supported
is 4.4. I did some linux distribution comparison and took gmime 3 along
with it.

Debian 9 (Stretch) has bash 4.4 -- and gmime 3.x

Debian 8 (Jessie) as bash 4.3 -- but (only) gmime 2.6 (and if I used
packages.debian.org correctly there is no backports either...)

Ubuntu 18.04 (bionic) has bash 4.4 -- and gmime 3.x

likewise, Ubuntu 16.04 (xenial) has neither (not even in backports)

SO, in above cases, to compile current notmuch, gmime 3 has to be compiled
separately. Then such user could also build newer bash... (and if there are
3rd party package repositories those can provide both...)

I'd guess the situation is pretty much the same with other long-term
supported distributions (centos 7 has bash 4.2, gmime 2.6 -- rhel/centos8
probably gmime30 3.2 and bash 4.4)

Latest short-term, like Fedora -- and rolling release distributions, if
those provide gmime 3.x those most probably have at least bash 4.4, if
not 5.0, provided).

I personally would like to use this feature; it is easy to add into scripts
and pretty lean learning curve. I tried to do the same with bash syntax
${param//\'/\'\\\'\'}, but that looks more complicated -- and even more 
complicated to get it working right on bash 4.1 where the feature is a bit
buggy when included inside double quotes ;(

The downside could be that we get less testing due to some users not wanting
to build newer bash (have to test whether it is hard so cannot say whether
the time is right...


BTW: tested ${var:q} on decade-old zsh (4.3.12 -- 2009-06-01), does the
same. According to zsh changes file perhaps even version from 2001-06-01
has this feature.... And from https://mywiki.wooledge.org/BashFAQ/061
(which seems pretty good page to dig for features):

${var@spec} / 4.4 (2016) / mksh (2012) for the syntax, zsh (1990s) for the feature

the feature could have been there even when I start using that shell ;D


Tomi

Re: [PATCH 1/2] test: provide machinery to make and use test_shims

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

On Wed 2019-06-26 00:05:09 +0300, Tomi Ollila wrote:
> LD_PRELOAD=./${shim_file}${LD_PRELOAD:+:$LD_PRELOAD} notmuch-shared "$@"
>
> So that if LD_PRELOAD is already set and is non-empty then ':' and its
> old value is appended to new value of LD_PRELOAD before this
> notmuch-shared invocation.

agreed, good catch, Tomi.

> Also I'm "insisting" that your patch 2/2 is updated based on
> id:8736kipe9f.fsf@fifthhorseman.net -- dkg had good opening how to 
> improve robustness in `eval` executions, but since there are quite
> a few similar problems in code (i.e. and there are quite a few ways
> to improve it) IMO maintaining status quo we can this feature improved
> incrementally and choose how to fix *all* related problems...

sounds fine to me, as long as we've got gen_insert_msg in the right
place.

> One option, which is relatively easy to start using (when needed) while
> writing code is to use ${parameter@Q} transformation; 
>
> from bash (4.4+) documentation:
>
>     Q      The  expansion is a string that is the value of parameter
>            quoted in a format that can be reused as input.
>
> ( that transforms e.g.  foo'bar to 'foo'\''bar' )

nice find!  "reused as input" sounds pretty weird to me.  does it mean
"reused as a single shell token" or something like that?  because:

    x='foo bar'
    head -v $x

is a legitimate form of "reusing" $x as input, though it will read the
two files "foo" and "bar" instead of the single file "foo bar".

I suppose "help printf" itself says:

        %q	quote the argument in a way that can be reused as shell input

so it probably means the same thing :)

> I personally would like to use this feature; it is easy to add into scripts
> and pretty lean learning curve. I tried to do the same with bash syntax
> ${param//\'/\'\\\'\'}, but that looks more complicated -- and even more 
> complicated to get it working right on bash 4.1 where the feature is a bit
> buggy when included inside double quotes ;(

ugh, this is not only more visually complicated, it's also much harder
to tell if it's correct at a glance.

> […]
> likewise, Ubuntu 16.04 (xenial) has neither (not even in backports)

Thanks for doing the research about where bash 4.4 is supported.  I
don't personally mind requiring bash 4.4 for running the test suite.

I can add older bash to the xenial PPA for travis if we need to (or if
someone else wants to to it, i'm happy to add you as an authorized
uploader for that repo).

      --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[David Bremner]

This removes the dependency of this test script on gdb, and
considerably speeds up the running of the tests.
---
 test/T070-insert.sh | 50 ++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 48165caa..ab26ecd4 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -2,8 +2,6 @@
 test_description='"notmuch insert"'
 . $(dirname "$0")/test-lib.sh || exit 1
 
-test_require_external_prereq gdb
-
 # subtests about file permissions assume that we're working with umask
 # 022 by default.
 umask 022
@@ -246,50 +244,38 @@ test_expect_code 1 "notmuch insert $gen_msg_filename 2>&1"
 notmuch config set new.tags $OLDCONFIG
 
 # DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
+gen_insert_msg
 
-for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
-    READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
-cat <<EOF > index-file-$code.gdb
-set breakpoint pending on
-set logging file index-file-$code.log
-set logging on
-break notmuch_database_index_file
-commands
-return NOTMUCH_STATUS_$code
-continue
-end
-run
+# pregenerate all of the test shims
+for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR OUT_OF_MEMORY XAPIAN_EXCEPTION; do
+    make_shim shim-$code <<EOF
+#include <notmuch.h>
+#include <stdio.h>
+notmuch_status_t
+notmuch_database_index_file (notmuch_database_t *notmuch,
+                             const char *filename,
+                             notmuch_indexopts_t *indexopts,
+                             notmuch_message_t **message_ret)
+{
+  return NOTMUCH_STATUS_$code;
+}
 EOF
 done
 
-gen_insert_msg
-
 for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
     test_begin_subtest "EXIT_FAILURE when index_file returns $code"
-    test_expect_code 1 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 1 "notmuch_with_shim shim-$code insert < \"$gen_msg_filename\""
 
     test_begin_subtest "success exit with --keep when index_file returns $code"
-    test_expect_code 0 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert --keep < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "notmuch_with_shim shim-$code insert --keep < \"$gen_msg_filename\""
 done
 
 for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
     test_begin_subtest "EX_TEMPFAIL when index_file returns $code"
-    test_expect_code 75 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 75 "notmuch_with_shim shim-$code insert < \"$gen_msg_filename\""
 
     test_begin_subtest "success exit with --keep when index_file returns $code"
-    test_expect_code 0 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert --keep < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "notmuch_with_shim shim-$code insert --keep < \"$gen_msg_filename\""
 done
 
 test_done
-- 
2.20.1

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]

On Sun 2019-05-26 10:08:54 -0300, David Bremner wrote:
> This removes the dependency of this test script on gdb, and
> considerably speeds up the running of the tests.

This series looks good to me.  I've tested it with moreutils parallel
installed, and it reduces total CPU time for the parallel test suite
from ~76 seconds of CPU to ~56 seconds, a > %25 reduction in CPU cost
for the whole test suite, despite touching only one test.  This is
awesome work.

I also like the elegance of the proposed change.  It would be great to
see it extended to the rest of our test suite's dependency on GDB if we
can do it. (T050, T060, and T380 i think)

Details on my profiling:

On the current master (bc396c967c7cd8e7a109858e428d7bf97173f7a7),
without these changes applied, the whole test suite consumes this much
CPU on my 4-core intel i5-2540M (2.60GHz):

real	0m33.106s
user	1m1.150s
sys	0m14.998s

On the same machine, with the pair of patches applied, the whole test
suite consumes this:

real	0m27.557s
user	0m44.172s
sys	0m12.018s

Caveat: i don't know how well LD_PRELOAD works on non-GNU/Linux
platforms, and we're not using LD_PRELOAD elsewhere in the test suite
yet.  Perhaps Ralph Seichter (explicitly cc'ed above) could comment on
how it'll affect homebrew?  Do we need to consider a variant for
platforms that can't do LD_PRELOAD?

One nit-pick below:

> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index 48165caa..ab26ecd4 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -2,8 +2,6 @@
>  test_description='"notmuch insert"'
>  . $(dirname "$0")/test-lib.sh || exit 1
>  
> -test_require_external_prereq gdb
> -
>  # subtests about file permissions assume that we're working with umask
>  # 022 by default.
>  umask 022
> @@ -246,50 +244,38 @@ test_expect_code 1 "notmuch insert $gen_msg_filename 2>&1"
>  notmuch config set new.tags $OLDCONFIG
>  
>  # DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
> +gen_insert_msg
>  
> -for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
> -    READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
> -cat <<EOF > index-file-$code.gdb
> -set breakpoint pending on
> -set logging file index-file-$code.log
> -set logging on
> -break notmuch_database_index_file
> -commands
> -return NOTMUCH_STATUS_$code
> -continue
> -end
> -run
> +# pregenerate all of the test shims
> +for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR OUT_OF_MEMORY XAPIAN_EXCEPTION; do
> +    make_shim shim-$code <<EOF
> +#include <notmuch.h>
> +#include <stdio.h>
> +notmuch_status_t
> +notmuch_database_index_file (notmuch_database_t *notmuch,
> +                             const char *filename,
> +                             notmuch_indexopts_t *indexopts,
> +                             notmuch_message_t **message_ret)
> +{
> +  return NOTMUCH_STATUS_$code;
> +}
>  EOF
>  done
>  
> -gen_insert_msg
> -

Why put the shim generation below gen_insert_msg?  If it were above, it
would make the comment about DUPLICATE_MESSAGE_ID less confusing, and
the changeset could be shorter.

          --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Ralph Seichter]

* Daniel Kahn Gillmor:

> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how
> it'll affect homebrew?

MacPorts, actually. ;-) I have not yet been able to look into this patch
series, but I hope to be able to do so soonish.

-Ralph

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[David Bremner]

Ralph Seichter <abbot@monksofcool.net> writes:

> * Daniel Kahn Gillmor:
>
>> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how
>> it'll affect homebrew?
>
> MacPorts, actually. ;-) I have not yet been able to look into this patch
> series, but I hope to be able to do so soonish.
>
> -Ralph

Supposedly DYLD_INSERT_LIBRARIES does (did?) the same job as LD_PRELOAD,
but there seems to some macOS specific complications.

d

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

On Fri 2019-06-14 08:16:14 -0300, David Bremner wrote:
> Ralph Seichter <abbot@monksofcool.net> writes:
>
>> * Daniel Kahn Gillmor:
>>
>>> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how
>>> it'll affect homebrew?
>>
>> MacPorts, actually. ;-) I have not yet been able to look into this patch
>> series, but I hope to be able to do so soonish.
>>
>> -Ralph
>
> Supposedly DYLD_INSERT_LIBRARIES does (did?) the same job as LD_PRELOAD,
> but there seems to some macOS specific complications.

Do you think we could go ahead and apply these patches on master now
anyway, and fix them up subsequently to make sure they apply to MacOS?
I don't know what the "MacOS specific complications" are.  Can they be
spelled out in more detail?  I note that
https://stackoverflow.com/questions/34114587/dyld-library-path-dyld-insert-libraries-not-working
suggests that DYLD_INSERT_LIBRARIES won't work with signed binaries, but
i don't think the binaries tested during the test suite are signed
binaries, are they?

Alternately if DYLD_INSERT_LIBRARIES doesn't work, should we just skip
this test on MacOS?  or have it fall back to gdb?

I'd really like to see this test suite speedup merged.

    --dkg

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Tomi Ollila]

On Mon, Jun 24 2019, Daniel Kahn Gillmor wrote:

> On Fri 2019-06-14 08:16:14 -0300, David Bremner wrote:
>> Ralph Seichter <abbot@monksofcool.net> writes:
>>
>>> * Daniel Kahn Gillmor:
>>>
>>>> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how
>>>> it'll affect homebrew?
>>>
>>> MacPorts, actually. ;-) I have not yet been able to look into this patch
>>> series, but I hope to be able to do so soonish.
>>>
>>> -Ralph
>>
>> Supposedly DYLD_INSERT_LIBRARIES does (did?) the same job as LD_PRELOAD,
>> but there seems to some macOS specific complications.
>
> Do you think we could go ahead and apply these patches on master now
> anyway, and fix them up subsequently to make sure they apply to MacOS?
> I don't know what the "MacOS specific complications" are.  Can they be
> spelled out in more detail?  I note that
> https://stackoverflow.com/questions/34114587/dyld-library-path-dyld-insert-libraries-not-working
> suggests that DYLD_INSERT_LIBRARIES won't work with signed binaries, but
> i don't think the binaries tested during the test suite are signed
> binaries, are they?
>
> Alternately if DYLD_INSERT_LIBRARIES doesn't work, should we just skip
> this test on MacOS?  or have it fall back to gdb?
>
> I'd really like to see this test suite speedup merged.

If this suite speedup is merged, I'd suggest using original David's patch
2/2 due to consistency reasons -- $gen_test_filename is used likewise in
other test_expect_code cases. and, unless $TEST_DIRECTORY contains '"'s, 
'$'s or '`'s it work (in our current cases $gen_test_name does not contain
the above characters (gen_test_filename=$TEST_DIRECTORY/$gen_test_name))

I've looked this quite a lot lately (mostly for fun). I'll send email in
near future some suggestions (ten (10) or so) how we could improve the
situation here, which then could be applied everywhere...

>
>     --dkg

Tomi

PS: actually, there is e.g.

T070-insert.sh:test_expect_code 1 "notmuch insert --folder=nonesuch < $gen_msg_filename"

(and that is not the only one)

In this case also ' ', \t and \n in $gen_msg_filename would make tests
break (e.g. someone runs tests in $PWD that has spaces in it).

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Mon 2019-06-24 21:44:13 +0300, Tomi Ollila wrote:
> If this suite speedup is merged, I'd suggest using original David's patch
> 2/2 due to consistency reasons -- $gen_test_filename is used likewise in
> other test_expect_code cases. and, unless $TEST_DIRECTORY contains '"'s, 
> '$'s or '`'s it work (in our current cases $gen_test_name does not contain
> the above characters (gen_test_filename=$TEST_DIRECTORY/$gen_test_name))
>
> I've looked this quite a lot lately (mostly for fun). I'll send email in
> near future some suggestions (ten (10) or so) how we could improve the
> situation here, which then could be applied everywhere...

If you insist, i'm ok to wait on your cleanup for pathnames that have
unusual characters in them, though i'd also be happy to see the cleanup
applied to make *inconsistent* things consistent, rather than enforcing
a broken-yet-uniform consistency and then applying the cleanup.

But we should not use Bremner's patch 2/2 directly, because it moves
shim generation below gen_insert_msg, as mentioned in
id:8736kipe9f.fsf@fifthhorseman.net.

                --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Ralph Seichter]

* Daniel Kahn Gillmor:

> Do you think we could go ahead and apply these patches on master now
> anyway, and fix them up subsequently to make sure they apply to MacOS?

I still have not been able to set up any macOS based tests, because
contractual work has kept me busy. I suggest you go ahead with the
changes, and should I run into any unexpected issues in future builds,
I'll speak up. I don't want to slow you folks down.

-Ralph

Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

One more nit-pick:

On Sun 2019-05-26 10:08:54 -0300, David Bremner wrote:
> +    test_expect_code 0 "notmuch_with_shim shim-$code insert --keep < \"$gen_msg_filename\""

This kind of business breaks obscurely if $gen_msg_filename happens to
have U+0022 QUOTATION MARK in it.  That's a pretty perverse situation,
but i'd generally prefer to use bash's builtin printf's %q for
robustness.

A revised patch will follow shortly that fixes both of my nitpicks.

  --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[PATCH v2 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

From: David Bremner <david@tethera.net>

This removes the dependency of this test script on gdb, and
considerably speeds up the running of the tests.

---

dkg cleaned up the placement of gen_insert_msg, and used printf's %q
to handle the perverse case where $gen_msg_filename happens to have a
U+0022 QUOTATION MARK ('"') in it.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/T070-insert.sh | 48 ++++++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 31 deletions(-)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 48165caa..017124fc 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -2,8 +2,6 @@
 test_description='"notmuch insert"'
 . $(dirname "$0")/test-lib.sh || exit 1
 
-test_require_external_prereq gdb
-
 # subtests about file permissions assume that we're working with umask
 # 022 by default.
 umask 022
@@ -246,19 +244,19 @@ test_expect_code 1 "notmuch insert $gen_msg_filename 2>&1"
 notmuch config set new.tags $OLDCONFIG
 
 # DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
-
-for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
-    READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
-cat <<EOF > index-file-$code.gdb
-set breakpoint pending on
-set logging file index-file-$code.log
-set logging on
-break notmuch_database_index_file
-commands
-return NOTMUCH_STATUS_$code
-continue
-end
-run
+# pregenerate all of the test shims
+for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR OUT_OF_MEMORY XAPIAN_EXCEPTION; do
+    make_shim shim-$code <<EOF
+#include <notmuch.h>
+#include <stdio.h>
+notmuch_status_t
+notmuch_database_index_file (notmuch_database_t *notmuch,
+                             const char *filename,
+                             notmuch_indexopts_t *indexopts,
+                             notmuch_message_t **message_ret)
+{
+  return NOTMUCH_STATUS_$code;
+}
 EOF
 done
 
@@ -266,30 +264,18 @@ gen_insert_msg
 
 for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
     test_begin_subtest "EXIT_FAILURE when index_file returns $code"
-    test_expect_code 1 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 1 "$(printf "notmuch_with_shim shim-%q insert < %q" "$code" "$gen_msg_filename")"
 
     test_begin_subtest "success exit with --keep when index_file returns $code"
-    test_expect_code 0 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert --keep < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "$(printf "notmuch_with_shim shim-%q insert --keep < %q" "$code" "$gen_msg_filename")"
 done
 
 for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
     test_begin_subtest "EX_TEMPFAIL when index_file returns $code"
-    test_expect_code 75 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 75 "$(printf "notmuch_with_shim shim-%q insert < %q" "$code" "$gen_msg_filename")"
 
     test_begin_subtest "success exit with --keep when index_file returns $code"
-    test_expect_code 0 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert --keep < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "$(printf "notmuch_with_shim shim-%q insert --keep < %q" "$code" "$gen_msg_filename")"
 done
 
 test_done
-- 
2.20.1

Re: [PATCH v2 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Tomi Ollila]

On Mon, Jun 10 2019, Daniel Kahn Gillmor wrote:

> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index 48165caa..017124fc 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -266,30 +264,18 @@ gen_insert_msg
>  
>  for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
>      test_begin_subtest "EXIT_FAILURE when index_file returns $code"
> -    test_expect_code 1 \
> -         "${TEST_GDB} --batch-silent --return-child-result \
> -	     -ex 'set args insert < $gen_msg_filename' \
> -	     -x index-file-$code.gdb notmuch"
> +    test_expect_code 1 "$(printf "notmuch_with_shim shim-%q insert < %q" "$code" "$gen_msg_filename")"

does   test_expect_code 1 'notmuch_with_shim shim-$code insert < "$gen_msg_filename"'

i.e $code and "$gen_msg_filename" are evaluated by eval instead when given
as an argument to test_expect_code

to me it look like is what get executed in test_run_ (called by
test_expect_code) and neither of the 2 variables are clobbered
in the call path :D

    eval >&3 2>&4 'notmuch_with_shim shim-$code insert < "$gen_msg_filename"'


Tomi

Re: [PATCH v2 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Sun 2019-06-16 14:35:53 +0300, Tomi Ollila wrote:
> On Mon, Jun 10 2019, Daniel Kahn Gillmor wrote:
>> +    test_expect_code 1 "$(printf "notmuch_with_shim shim-%q insert < %q" "$code" "$gen_msg_filename")"
>
> does   test_expect_code 1 'notmuch_with_shim shim-$code insert < "$gen_msg_filename"'

hm, i think your proposal would do the right thing, but if someone was
to "clobber those variables in the call path" as you put it -- or if it
ended up getting evaluated by a subshell that didn't have those
variables exported, it would fail.

Furthermore, when test_expect_code fails, at least one of the failure
paths prints out the literal string that it received as the "$2"
argument, so it's nice to have the literal string fully-expanded before
it gets passed to test_expect_code.

So for both of those reasons (fragility of variables in the callpath;
and clearer test failure reporting) i prefer the way i've done it even
if it is a bit harder to read.

I won't fight too hard about this though, i've got other things on my
plate with a higher priority.  So if you want to offer a third variant
of bremner's patches with your preferred approach, i'll probably be ok
with it.

    --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

v3 of test speedup by replacing gdb with LD_PRELOAD

[Daniel Kahn Gillmor]

Here are the two patches from this series, revised according to the
on-list discussion.

In particular, the revision from Bremner's original series are:

 * LD_PRELOAD is now prepended to, not clobbered (thanks, Tomi!)
 * gen_insert_msg is not moved

We still don't deal with platforms that don't support LD_PRELOAD (like
MacOS), but i'd encourage us to take Ralph at his word and go ahead
with these changes now, cleaning them up on those platforms later.

     --dkg

[PATCH v3 1/2] test: provide machinery to make and use test_shims

[Daniel Kahn Gillmor]

From: David Bremner <david@tethera.net>

These can be used e.g. to override return values for functions, in
place of the existing scripting of gdb.

This prepends to LD_PRELOAD rather than clobbering it, thanks to a
suggestion from Tomi Ollila.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/test-lib.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 616cb674..7f8a3a4d 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -95,6 +95,8 @@ TEST_EMACSCLIENT=${TEST_EMACSCLIENT:-emacsclient}
 TEST_GDB=${TEST_GDB:-gdb}
 TEST_CC=${TEST_CC:-cc}
 TEST_CFLAGS=${TEST_CFLAGS:-"-g -O0"}
+TEST_SHIM_CFLAGS=${TEST_SHIM_CFLAGS:-"-fpic -shared"}
+TEST_SHIM_LDFLAGS=${TEST_SHIM_LDFLAGS:-"-ldl"}
 
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
@@ -1056,6 +1058,20 @@ test_C () {
     notmuch_dir_sanitize OUTPUT.stdout OUTPUT.stderr > OUTPUT
 }
 
+make_shim () {
+    base_name="$1"
+    test_file="${base_name}.c"
+    shim_file="${base_name}.so"
+    cat > ${test_file}
+    ${TEST_CC} ${TEST_CFLAGS} ${TEST_SHIM_CFLAGS} -I${NOTMUCH_SRCDIR}/test -I${NOTMUCH_SRCDIR}/lib -o ${shim_file} ${test_file} ${TEST_SHIM_LDFLAGS}
+}
+
+notmuch_with_shim () {
+    base_name="$1"
+    shift
+    shim_file="${base_name}.so"
+    LD_PRELOAD=./${shim_file}${LD_PRELOAD:+:$LD_PRELOAD} notmuch-shared "$@"
+}
 
 # Creates a script that counts how much time it is executed and calls
 # notmuch.  $notmuch_counter_command is set to the path to the
-- 
2.20.1

[PATCH v3 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh

[Daniel Kahn Gillmor]

From: David Bremner <david@tethera.net>

This removes the dependency of this test script on gdb, and
considerably speeds up the running of the tests.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/T070-insert.sh | 48 ++++++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 31 deletions(-)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 48165caa..c8161e1e 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -2,8 +2,6 @@
 test_description='"notmuch insert"'
 . $(dirname "$0")/test-lib.sh || exit 1
 
-test_require_external_prereq gdb
-
 # subtests about file permissions assume that we're working with umask
 # 022 by default.
 umask 022
@@ -246,19 +244,19 @@ test_expect_code 1 "notmuch insert $gen_msg_filename 2>&1"
 notmuch config set new.tags $OLDCONFIG
 
 # DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
-
-for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
-    READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
-cat <<EOF > index-file-$code.gdb
-set breakpoint pending on
-set logging file index-file-$code.log
-set logging on
-break notmuch_database_index_file
-commands
-return NOTMUCH_STATUS_$code
-continue
-end
-run
+# pregenerate all of the test shims
+for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR OUT_OF_MEMORY XAPIAN_EXCEPTION; do
+    make_shim shim-$code <<EOF
+#include <notmuch.h>
+#include <stdio.h>
+notmuch_status_t
+notmuch_database_index_file (notmuch_database_t *notmuch,
+                             const char *filename,
+                             notmuch_indexopts_t *indexopts,
+                             notmuch_message_t **message_ret)
+{
+  return NOTMUCH_STATUS_$code;
+}
 EOF
 done
 
@@ -266,30 +264,18 @@ gen_insert_msg
 
 for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
     test_begin_subtest "EXIT_FAILURE when index_file returns $code"
-    test_expect_code 1 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 1 "notmuch_with_shim shim-$code insert < \"$gen_msg_filename\""
 
     test_begin_subtest "success exit with --keep when index_file returns $code"
-    test_expect_code 0 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert --keep < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "notmuch_with_shim shim-$code insert --keep < \"$gen_msg_filename\""
 done
 
 for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
     test_begin_subtest "EX_TEMPFAIL when index_file returns $code"
-    test_expect_code 75 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 75 "notmuch_with_shim shim-$code insert < \"$gen_msg_filename\""
 
     test_begin_subtest "success exit with --keep when index_file returns $code"
-    test_expect_code 0 \
-         "${TEST_GDB} --batch-silent --return-child-result \
-	     -ex 'set args insert --keep < $gen_msg_filename' \
-	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "notmuch_with_shim shim-$code insert --keep < \"$gen_msg_filename\""
 done
 
 test_done
-- 
2.20.1

Re: v3 of test speedup by replacing gdb with LD_PRELOAD

[Tomi Ollila]

On Wed, Jun 26 2019, Daniel Kahn Gillmor wrote:

> Here are the two patches from this series, revised according to the
> on-list discussion.
>
> In particular, the revision from Bremner's original series are:
>
>  * LD_PRELOAD is now prepended to, not clobbered (thanks, Tomi!)
>  * gen_insert_msg is not moved
>
> We still don't deal with platforms that don't support LD_PRELOAD (like
> MacOS), but i'd encourage us to take Ralph at his word and go ahead
> with these changes now, cleaning them up on those platforms later.

Agreed. Series LGTM.

>
>      --dkg

Tomi

Re: v3 of test speedup by replacing gdb with LD_PRELOAD

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Wed, Jun 26 2019, Daniel Kahn Gillmor wrote:
>
>> Here are the two patches from this series, revised according to the
>> on-list discussion.
>>
>> In particular, the revision from Bremner's original series are:
>>
>>  * LD_PRELOAD is now prepended to, not clobbered (thanks, Tomi!)
>>  * gen_insert_msg is not moved
>>
>> We still don't deal with platforms that don't support LD_PRELOAD (like
>> MacOS), but i'd encourage us to take Ralph at his word and go ahead
>> with these changes now, cleaning them up on those platforms later.
>
> Agreed. Series LGTM.
>
>>
>>      --dkg

Pushed to master. Thanks to both of you for polishing that up.

d