[PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[Kevin Boulain]

The test fails quite reliably for me:
  T810-tsan: Testing run code with TSan enabled against the library
   PASS   create
   PASS   query
   FAIL   sexp query
          --- T810-tsan.3.EXPECTED        2023-04-03 19:53:04.400771102 +0000
          +++ T810-tsan.3.OUTPUT  2023-04-03 19:53:04.402771109 +0000
          @@ -1,2 +1,44 @@
           == stdout ==
           == stderr ==
          +==================
          +WARNING: ThreadSanitizer: data race (pid=21372)
          +  Read of size 4 at 0x7b1000000188 by thread T2:
          +    #0 Xapian::Internal::intrusive_ptr<Xapian::Query::Internal>::intrusive_ptr(Xapian::Internal::intrusive_ptr<Xapian::Query::Internal> const&) .../xapian/intrusive_ptr.h:107 (libnotmuch.so.5+0x3017b)
          +    #1 Xapian::Query::Query(Xapian::Query const&) .../xapian/query.h:328 (libnotmuch.so.5+0x2fcbe)
          +    #2 _sexp_to_xapian_query lib/parse-sexp.cc:707 (libnotmuch.so.5+0x43f1f)
          +    #3 _notmuch_sexp_string_to_xapian_query(_notmuch_database*, char const*, Xapian::Query&) lib/parse-sexp.cc:729 (libnotmuch.so.5+0x44207)
          +    #4 _notmuch_query_ensure_parsed_sexpr lib/query.cc:240 (libnotmuch.so.5+0x2c59e)
          +    #5 _notmuch_query_ensure_parsed lib/query.cc:258 (libnotmuch.so.5+0x2c646)
          +    #6 _notmuch_query_search_documents lib/query.cc:362 (libnotmuch.so.5+0x2cc0e)
          +    #7 notmuch_query_search_messages lib/query.cc:350 (libnotmuch.so.5+0x2cb76)
          +    #8 thread CWD/test2.c:17 (test2+0x4012f4)
          +
          +  Previous write of size 4 at 0x7b1000000188 by thread T1:
          +    #0 Xapian::Internal::intrusive_ptr<Xapian::Query::Internal>::intrusive_ptr(Xapian::Internal::intrusive_ptr<Xapian::Query::Internal> const&) .../xapian/intrusive_ptr.h:107 (libnotmuch.so.5+0x3018e)
          +    #1 Xapian::Query::Query(Xapian::Query const&) .../xapian/query.h:328 (libnotmuch.so.5+0x2fcbe)
          +    #2 _sexp_to_xapian_query lib/parse-sexp.cc:707 (libnotmuch.so.5+0x43f1f)
          +    #3 _notmuch_sexp_string_to_xapian_query(_notmuch_database*, char const*, Xapian::Query&) lib/parse-sexp.cc:729 (libnotmuch.so.5+0x44207)
          +    #4 _notmuch_query_ensure_parsed_sexpr lib/query.cc:240 (libnotmuch.so.5+0x2c59e)
          +    #5 _notmuch_query_ensure_parsed lib/query.cc:258 (libnotmuch.so.5+0x2c646)
          +    #6 _notmuch_query_search_documents lib/query.cc:362 (libnotmuch.so.5+0x2cc0e)
          +    #7 notmuch_query_search_messages lib/query.cc:350 (libnotmuch.so.5+0x2cb76)
          +    #8 thread CWD/test2.c:17 (test2+0x4012f4)
          +
          +  Location is heap block of size 56 at 0x7b1000000180 allocated by main thread:
          +    #0 operator new(unsigned long) <null> (libtsan.so.2+0x8ba83)
          +    #1 Xapian::Query::Query(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int, unsigned int) <null> (libxapian.so.30+0x9f200)
          +    #2 __static_initialization_and_destruction_0(int, int) <null> (libxapian.so.30+0xa27ac)
          +    #3 _GLOBAL__sub_I_query.cc <null> (libxapian.so.30+0xa286d)
          +    #4 call_init <null> (ld-linux-x86-64.so.2+0x5e1d)
          +
          +  Thread T2 (tid=21375, running) created by main thread at:
          +    #0 pthread_create <null> (libtsan.so.2+0x62de6)
          +    #1 main CWD/test2.c:24 (test2+0x4013ba)
          +
          +  Thread T1 (tid=21374, running) created by main thread at:
          +    #0 pthread_create <null> (libtsan.so.2+0x62de6)
          +    #1 main CWD/test2.c:23 (test2+0x401380)
          +
          +SUMMARY: ThreadSanitizer: data race .../xapian/intrusive_ptr.h:107 in Xapian::Internal::intrusive_ptr<Xapian::Query::Internal>::intrusive_ptr(Xapian::Internal::intrusive_ptr<Xapian::Query::Internal> const&)
          +==================
          +ThreadSanitizer: reported 1 warnings
---
 test/T810-tsan.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/test/T810-tsan.sh b/test/T810-tsan.sh
index 7e877b27..c9008c6b 100755
--- a/test/T810-tsan.sh
+++ b/test/T810-tsan.sh
@@ -89,4 +89,44 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+if [ $NOTMUCH_HAVE_SFSEXP -eq 1 ]; then
+    test_begin_subtest "sexp query"
+    test_subtest_known_broken
+    test_C ${MAIL_DIR} ${MAIL_DIR}-2 <<EOF
+#include <notmuch-test.h>
+#include <pthread.h>
+
+void *thread (void *arg) {
+  char *mail_dir = arg;
+  notmuch_database_t *db;
+  /*
+   * Query generation from s-expression used the tread-unsafe
+   * Xapian::Query::MatchAll.
+   */
+  EXPECT0(notmuch_database_open_with_config (mail_dir,
+                                             NOTMUCH_DATABASE_MODE_READ_ONLY,
+                                             NULL, NULL, &db, NULL));
+  notmuch_query_t *query;
+  EXPECT0(notmuch_query_create_with_syntax (db, "(from *)", NOTMUCH_QUERY_SYNTAX_SEXP, &query));
+  notmuch_messages_t *messages;
+  EXPECT0(notmuch_query_search_messages (query, &messages));
+  return NULL;
+}
+
+int main (int argc, char **argv) {
+  pthread_t t1, t2;
+  EXPECT0(pthread_create (&t1, NULL, thread, argv[1]));
+  EXPECT0(pthread_create (&t2, NULL, thread, argv[2]));
+  EXPECT0(pthread_join (t1, NULL));
+  EXPECT0(pthread_join (t2, NULL));
+  return 0;
+}
+EOF
+    cat <<EOF > EXPECTED
+== stdout ==
+== stderr ==
+EOF
+    test_expect_equal_file EXPECTED OUTPUT
+fi
+
 test_done

[PATCH 2/2] lib: thread-safe s-expression query parser

[Kevin Boulain]

Follow-up of 6273966d, now that sfsexp 1.4.1 doesn't rely on globals
anymore by default (https://github.com/mjsottile/sfsexp/issues/21).

This simply defers the initial query generation to use the thread-safe
helper (xapian_query_match_all) instead of Xapian::Query::MatchAll.
---
 lib/parse-sexp.cc | 85 +++++++++++++++++++++++++++++------------------
 test/T810-tsan.sh |  1 -
 2 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/lib/parse-sexp.cc b/lib/parse-sexp.cc
index 9cadbc13..825bb9f9 100644
--- a/lib/parse-sexp.cc
+++ b/lib/parse-sexp.cc
@@ -3,6 +3,7 @@
 #if HAVE_SFSEXP
 #include "sexp.h"
 #include "unicode-util.h"
+#include "xapian-extra.h"
 
 /* _sexp is used for file scope symbols to avoid clashing with
  * definitions from sexp.h */
@@ -53,68 +54,85 @@ operator& (_sexp_flag_t a, _sexp_flag_t b)
 	static_cast<unsigned>(a) & static_cast<unsigned>(b));
 }
 
+typedef enum {
+    SEXP_INITIAL_MATCH_ALL,
+    SEXP_INITIAL_MATCH_NOTHING,
+} _sexp_initial_t;
+
+inline Xapian::Query
+_sexp_initial_query (_sexp_initial_t initial)
+{
+    switch (initial) {
+    case SEXP_INITIAL_MATCH_ALL:
+	return xapian_query_match_all ();
+    case SEXP_INITIAL_MATCH_NOTHING:
+	return Xapian::Query::MatchNothing;
+    }
+    assert (! "unreachable");
+}
+
 typedef struct  {
     const char *name;
     Xapian::Query::op xapian_op;
-    Xapian::Query initial;
+    _sexp_initial_t initial;
     _sexp_flag_t flags;
 } _sexp_prefix_t;
 
 static _sexp_prefix_t prefixes[] =
 {
-    { "and",            Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "and",            Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_NONE },
-    { "attachment",     Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "attachment",     Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-    { "body",           Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "body",           Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD },
-    { "date",           Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "date",           Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_RANGE },
-    { "from",           Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "from",           Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "folder",         Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "folder",         Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND |
       SEXP_FLAG_PATHNAME },
-    { "id",             Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "id",             Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX },
-    { "infix",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "infix",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-    { "is",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "is",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "lastmod",           Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "lastmod",           Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_RANGE },
-    { "matching",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "matching",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_DO_EXPAND },
-    { "mid",            Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "mid",            Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX },
-    { "mimetype",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "mimetype",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-    { "not",            Xapian::Query::OP_AND_NOT,      Xapian::Query::MatchAll,
+    { "not",            Xapian::Query::OP_AND_NOT,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_NONE },
-    { "of",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "of",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_DO_EXPAND },
-    { "or",             Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "or",             Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_NONE },
-    { "path",           Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "path",           Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX |
       SEXP_FLAG_PATHNAME },
-    { "property",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "property",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "query",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchNothing,
+    { "query",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-    { "regex",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "regex",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_DO_REGEX },
-    { "rx",             Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "rx",             Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_DO_REGEX },
-    { "starts-with",    Xapian::Query::OP_WILDCARD,     Xapian::Query::MatchAll,
+    { "starts-with",    Xapian::Query::OP_WILDCARD,     SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE },
-    { "subject",        Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "subject",        Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "tag",            Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "tag",            Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "thread",         Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "thread",         Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "to",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "to",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
     { }
 };
@@ -318,7 +336,8 @@ _sexp_expand_query (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_BAD_QUERY_SYNTAX;
     }
 
-    status = _sexp_combine_query (notmuch, NULL, NULL, prefix->xapian_op, prefix->initial, sx,
+    status = _sexp_combine_query (notmuch, NULL, NULL, prefix->xapian_op,
+				  _sexp_initial_query (prefix->initial), sx,
 				  subquery);
     if (status)
 	return status;
@@ -370,7 +389,8 @@ _sexp_parse_header (notmuch_database_t *notmuch, const _sexp_prefix_t *parent,
 
     parent = &user_prefix;
 
-    return _sexp_combine_query (notmuch, parent, env, Xapian::Query::OP_AND, Xapian::Query::MatchAll,
+    return _sexp_combine_query (notmuch, parent, env, Xapian::Query::OP_AND,
+				xapian_query_match_all (),
 				sx->list->next, output);
 }
 
@@ -520,7 +540,7 @@ _sexp_parse_range (notmuch_database_t *notmuch,  const _sexp_prefix_t *prefix,
 
     /* empty range matches everything */
     if (! sx) {
-	output = Xapian::Query::MatchAll;
+	output = xapian_query_match_all ();
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
@@ -628,7 +648,7 @@ _sexp_to_xapian_query (notmuch_database_t *notmuch, const _sexp_prefix_t *parent
 
     /* Empty list */
     if (! sx->list) {
-	output = Xapian::Query::MatchAll;
+	output = xapian_query_match_all ();
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
@@ -704,7 +724,8 @@ _sexp_to_xapian_query (notmuch_database_t *notmuch, const _sexp_prefix_t *parent
 		return _sexp_expand_query (notmuch, prefix, parent, env, sx->list->next, output);
 	    }
 
-	    return _sexp_combine_query (notmuch, parent, env, prefix->xapian_op, prefix->initial,
+	    return _sexp_combine_query (notmuch, parent, env, prefix->xapian_op,
+					_sexp_initial_query (prefix->initial),
 					sx->list->next, output);
 	}
     }
diff --git a/test/T810-tsan.sh b/test/T810-tsan.sh
index c9008c6b..e876306c 100755
--- a/test/T810-tsan.sh
+++ b/test/T810-tsan.sh
@@ -91,7 +91,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 if [ $NOTMUCH_HAVE_SFSEXP -eq 1 ]; then
     test_begin_subtest "sexp query"
-    test_subtest_known_broken
     test_C ${MAIL_DIR} ${MAIL_DIR}-2 <<EOF
 #include <notmuch-test.h>
 #include <pthread.h>

Re: [PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> The test fails quite reliably for me:
>   T810-tsan: Testing run code with TSan enabled against the library
>    PASS   create
>    PASS   query
>    FAIL   sexp query

I can't get this test to fail at all. I'm not sure what to conclude from
that. This is on a 20 core system running linux.

d

Re: [PATCH 2/2] lib: thread-safe s-expression query parser

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> +
> +inline Xapian::Query
> +_sexp_initial_query (_sexp_initial_t initial)

My first thought was that this should be static, but maybe it doesn't
matter in C++; I see the other inline functions in that file are not
declared static.

> +    }
> +    assert (! "unreachable");

I stumbled over the logic this code. I would prefer not to use assert
for this. In a few other places we call INTERNAL_ERROR from a default:
case.

Other than those quibbles, the change looks reasonable. I'm unsure how
to test it though.

Re: [PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[Michael J Gruber]

Am Sa., 22. Juli 2023 um 20:48 Uhr schrieb David Bremner <david@tethera.net>:
>
> Kevin Boulain <kevin@boula.in> writes:
>
> > The test fails quite reliably for me:
> >   T810-tsan: Testing run code with TSan enabled against the library
> >    PASS   create
> >    PASS   query
> >    FAIL   sexp query
>
> I can't get this test to fail at all. I'm not sure what to conclude from
> that. This is on a 20 core system running linux.

This might be completely unrelated, but I recall a strange problem I
had with asan (not tsan): We build without asan, but if asan is
present for whatever reason then the tests are run nonetheless. So we
had to disable asan tests explicitly. Is your sexp compiled
accordingly?

Michael

Re: [PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[David Bremner]

Michael J Gruber <michaeljgruber+grubix+git@gmail.com> writes:

> This might be completely unrelated, but I recall a strange problem I
> had with asan (not tsan): We build without asan, but if asan is
> present for whatever reason then the tests are run nonetheless. So we
> had to disable asan tests explicitly. Is your sexp compiled
> accordingly?

Only the test program is compile with -fsanitize=thread; that seemed to
be enough for the other tests in T810-tsan.sh.

Re: [PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[Kevin Boulain]

On 2023-07-22 at 15:48 -03, David Bremner <david@tethera.net> wrote:
>
> I can't get this test to fail at all. I'm not sure what to conclude from
> that. This is on a 20 core system running linux.

I can't really remember what I did at the time but I believe only the
test is instrumented and not the Notmuch library.
The test fails reliably when compiled like this:
  ./configure
  make -j CFLAGS=-fsanitize=thread LDFLAGS=-fsanitize=thread
  ./test/T810-tsan.sh
  # ...
  # WARNING: ThreadSanitizer: data race (pid=35836)
  # ...

I would say it would be best to run all the tests with the different
instrumentations enabled (for the best coverage) but I don't know if
there's a CI. Or if out-of-tree compilation is supported, I guess the
ASan and TSan tests could recompile the library with the instrumentation
enabled... What do you think?

Re: [PATCH 1/2] test: showcase thread-unsafe s-expression query parser

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> I would say it would be best to run all the tests with the different
> instrumentations enabled (for the best coverage) but I don't know if
> there's a CI. Or if out-of-tree compilation is supported, I guess the
> ASan and TSan tests could recompile the library with the
> instrumentation enabled... What do you think?

The notmuch test-suite is optimized/intended for interactive use, so I
don't know if the slowdown from a complete rebuild in several of the
tests would be tolerable. Someone (TM) could check that. In principle
out of tree builds should work, although they tend to bitrot,

Running the whole test suite with instrumentation enabled is already an
option (basically what you already did, but with more tests run), but
then tests which depend on that instumentation should disable themselves
if it is not present. I'm not sure if instrumentation can be reliably
detected by introspection, or if there needs to be one or more
configuration variables set with the configure script, and used by both
build and test scripts.

d

[PATCH v2 1/2] test: showcase thread-unsafe s-expression query parser

[Kevin Boulain]

The test fails reliably when Notmuch is compiled as such:
  ./configure
  make CFLAGS=-fsanitize=thread LDFLAGS=-fsanitize=thread
It's still unclear how the test suite could be run with the correct
compilation flags.

Output:
  T810-tsan: Testing run code with TSan enabled against the library
   PASS   create
   PASS   query
   FAIL   sexp query
          --- T810-tsan.3.EXPECTED        2023-04-03 19:53:04.400771102 +0000
          +++ T810-tsan.3.OUTPUT  2023-04-03 19:53:04.402771109 +0000
          @@ -1,2 +1,44 @@
           == stdout ==
           == stderr ==
          +==================
          +WARNING: ThreadSanitizer: data race (pid=21372)
          +  Read of size 4 at 0x7b1000000188 by thread T2:
          +    #0 Xapian::Internal::intrusive_ptr<Xapian::Query::Internal>::intrusive_ptr(Xapian::Internal::intrusive_ptr<Xapian::Query::Internal> const&) .../xapian/intrusive_ptr.h:107 (libnotmuch.so.5+0x3017b)
          +    #1 Xapian::Query::Query(Xapian::Query const&) .../xapian/query.h:328 (libnotmuch.so.5+0x2fcbe)
          +    #2 _sexp_to_xapian_query lib/parse-sexp.cc:707 (libnotmuch.so.5+0x43f1f)
          +    #3 _notmuch_sexp_string_to_xapian_query(_notmuch_database*, char const*, Xapian::Query&) lib/parse-sexp.cc:729 (libnotmuch.so.5+0x44207)
          +    #4 _notmuch_query_ensure_parsed_sexpr lib/query.cc:240 (libnotmuch.so.5+0x2c59e)
          +    #5 _notmuch_query_ensure_parsed lib/query.cc:258 (libnotmuch.so.5+0x2c646)
          +    #6 _notmuch_query_search_documents lib/query.cc:362 (libnotmuch.so.5+0x2cc0e)
          +    #7 notmuch_query_search_messages lib/query.cc:350 (libnotmuch.so.5+0x2cb76)
          +    #8 thread CWD/test2.c:17 (test2+0x4012f4)
          +
          +  Previous write of size 4 at 0x7b1000000188 by thread T1:
          +    #0 Xapian::Internal::intrusive_ptr<Xapian::Query::Internal>::intrusive_ptr(Xapian::Internal::intrusive_ptr<Xapian::Query::Internal> const&) .../xapian/intrusive_ptr.h:107 (libnotmuch.so.5+0x3018e)
          +    #1 Xapian::Query::Query(Xapian::Query const&) .../xapian/query.h:328 (libnotmuch.so.5+0x2fcbe)
          +    #2 _sexp_to_xapian_query lib/parse-sexp.cc:707 (libnotmuch.so.5+0x43f1f)
          +    #3 _notmuch_sexp_string_to_xapian_query(_notmuch_database*, char const*, Xapian::Query&) lib/parse-sexp.cc:729 (libnotmuch.so.5+0x44207)
          +    #4 _notmuch_query_ensure_parsed_sexpr lib/query.cc:240 (libnotmuch.so.5+0x2c59e)
          +    #5 _notmuch_query_ensure_parsed lib/query.cc:258 (libnotmuch.so.5+0x2c646)
          +    #6 _notmuch_query_search_documents lib/query.cc:362 (libnotmuch.so.5+0x2cc0e)
          +    #7 notmuch_query_search_messages lib/query.cc:350 (libnotmuch.so.5+0x2cb76)
          +    #8 thread CWD/test2.c:17 (test2+0x4012f4)
          +
          +  Location is heap block of size 56 at 0x7b1000000180 allocated by main thread:
          +    #0 operator new(unsigned long) <null> (libtsan.so.2+0x8ba83)
          +    #1 Xapian::Query::Query(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int, unsigned int) <null> (libxapian.so.30+0x9f200)
          +    #2 __static_initialization_and_destruction_0(int, int) <null> (libxapian.so.30+0xa27ac)
          +    #3 _GLOBAL__sub_I_query.cc <null> (libxapian.so.30+0xa286d)
          +    #4 call_init <null> (ld-linux-x86-64.so.2+0x5e1d)
          +
          +  Thread T2 (tid=21375, running) created by main thread at:
          +    #0 pthread_create <null> (libtsan.so.2+0x62de6)
          +    #1 main CWD/test2.c:24 (test2+0x4013ba)
          +
          +  Thread T1 (tid=21374, running) created by main thread at:
          +    #0 pthread_create <null> (libtsan.so.2+0x62de6)
          +    #1 main CWD/test2.c:23 (test2+0x401380)
          +
          +SUMMARY: ThreadSanitizer: data race .../xapian/intrusive_ptr.h:107 in Xapian::Internal::intrusive_ptr<Xapian::Query::Internal>::intrusive_ptr(Xapian::Internal::intrusive_ptr<Xapian::Query::Internal> const&)
          +==================
          +ThreadSanitizer: reported 1 warnings
---
 test/T810-tsan.sh | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/test/T810-tsan.sh b/test/T810-tsan.sh
index 7e877b27..de98c0a9 100755
--- a/test/T810-tsan.sh
+++ b/test/T810-tsan.sh
@@ -4,7 +4,8 @@ test_directory=$(cd "$(dirname "${BASH_SOURCE[0]}")" > /dev/null && pwd)
 
 test_description='run code with TSan enabled against the library'
 # Note it is hard to ensure race conditions are deterministic so this
-# only provides best effort detection.
+# only provides best effort detection. Compile Notmuch with
+#  make CFLAGS=-fsanitize=thread LDFLAGS=-fsanitize=thread
 
 . "$test_directory"/test-lib.sh || exit 1
 
@@ -89,4 +90,44 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+if [ $NOTMUCH_HAVE_SFSEXP -eq 1 ]; then
+    test_begin_subtest "sexp query"
+    test_subtest_known_broken
+    test_C ${MAIL_DIR} ${MAIL_DIR}-2 <<EOF
+#include <notmuch-test.h>
+#include <pthread.h>
+
+void *thread (void *arg) {
+  char *mail_dir = arg;
+  notmuch_database_t *db;
+  /*
+   * Query generation from s-expression used the tread-unsafe
+   * Xapian::Query::MatchAll.
+   */
+  EXPECT0(notmuch_database_open_with_config (mail_dir,
+                                             NOTMUCH_DATABASE_MODE_READ_ONLY,
+                                             NULL, NULL, &db, NULL));
+  notmuch_query_t *query;
+  EXPECT0(notmuch_query_create_with_syntax (db, "(from *)", NOTMUCH_QUERY_SYNTAX_SEXP, &query));
+  notmuch_messages_t *messages;
+  EXPECT0(notmuch_query_search_messages (query, &messages));
+  return NULL;
+}
+
+int main (int argc, char **argv) {
+  pthread_t t1, t2;
+  EXPECT0(pthread_create (&t1, NULL, thread, argv[1]));
+  EXPECT0(pthread_create (&t2, NULL, thread, argv[2]));
+  EXPECT0(pthread_join (t1, NULL));
+  EXPECT0(pthread_join (t2, NULL));
+  return 0;
+}
+EOF
+    cat <<EOF > EXPECTED
+== stdout ==
+== stderr ==
+EOF
+    test_expect_equal_file EXPECTED OUTPUT
+fi
+
 test_done

[PATCH v2 2/2] lib: thread-safe s-expression query parser

[Kevin Boulain]

Follow-up of 6273966d, now that sfsexp 1.4.1 doesn't rely on globals
anymore by default (https://github.com/mjsottile/sfsexp/issues/21).

This simply defers the initial query generation to use the thread-safe
helper (xapian_query_match_all) instead of Xapian::Query::MatchAll.
---
 lib/parse-sexp.cc | 89 +++++++++++++++++++++++++++++------------------
 test/T810-tsan.sh |  1 -
 2 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/lib/parse-sexp.cc b/lib/parse-sexp.cc
index 9cadbc13..930888e9 100644
--- a/lib/parse-sexp.cc
+++ b/lib/parse-sexp.cc
@@ -3,6 +3,7 @@
 #if HAVE_SFSEXP
 #include "sexp.h"
 #include "unicode-util.h"
+#include "xapian-extra.h"
 
 /* _sexp is used for file scope symbols to avoid clashing with
  * definitions from sexp.h */
@@ -39,82 +40,99 @@ typedef enum {
 /*
  * define bitwise operators to hide casts */
 
-inline _sexp_flag_t
+static inline _sexp_flag_t
 operator| (_sexp_flag_t a, _sexp_flag_t b)
 {
     return static_cast<_sexp_flag_t>(
 	static_cast<unsigned>(a) | static_cast<unsigned>(b));
 }
 
-inline _sexp_flag_t
+static inline _sexp_flag_t
 operator& (_sexp_flag_t a, _sexp_flag_t b)
 {
     return static_cast<_sexp_flag_t>(
 	static_cast<unsigned>(a) & static_cast<unsigned>(b));
 }
 
+typedef enum {
+    SEXP_INITIAL_MATCH_ALL,
+    SEXP_INITIAL_MATCH_NOTHING,
+} _sexp_initial_t;
+
+static inline Xapian::Query
+_sexp_initial_query (_sexp_initial_t initial)
+{
+    switch (initial) {
+    case SEXP_INITIAL_MATCH_ALL:
+	return xapian_query_match_all ();
+    case SEXP_INITIAL_MATCH_NOTHING:
+	return Xapian::Query::MatchNothing;
+    }
+    INTERNAL_ERROR ("invalid initial sexp value %d", initial);
+}
+
 typedef struct  {
     const char *name;
     Xapian::Query::op xapian_op;
-    Xapian::Query initial;
+    _sexp_initial_t initial;
     _sexp_flag_t flags;
 } _sexp_prefix_t;
 
 static _sexp_prefix_t prefixes[] =
 {
-    { "and",            Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "and",            Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_NONE },
-    { "attachment",     Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "attachment",     Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-    { "body",           Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "body",           Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD },
-    { "date",           Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "date",           Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_RANGE },
-    { "from",           Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "from",           Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "folder",         Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "folder",         Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND |
       SEXP_FLAG_PATHNAME },
-    { "id",             Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "id",             Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX },
-    { "infix",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "infix",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-    { "is",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "is",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "lastmod",           Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "lastmod",           Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_RANGE },
-    { "matching",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "matching",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_DO_EXPAND },
-    { "mid",            Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "mid",            Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX },
-    { "mimetype",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "mimetype",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-    { "not",            Xapian::Query::OP_AND_NOT,      Xapian::Query::MatchAll,
+    { "not",            Xapian::Query::OP_AND_NOT,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_NONE },
-    { "of",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "of",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_DO_EXPAND },
-    { "or",             Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "or",             Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_NONE },
-    { "path",           Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "path",           Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX |
       SEXP_FLAG_PATHNAME },
-    { "property",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "property",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "query",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchNothing,
+    { "query",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-    { "regex",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "regex",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_DO_REGEX },
-    { "rx",             Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "rx",             Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_DO_REGEX },
-    { "starts-with",    Xapian::Query::OP_WILDCARD,     Xapian::Query::MatchAll,
+    { "starts-with",    Xapian::Query::OP_WILDCARD,     SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE },
-    { "subject",        Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "subject",        Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "tag",            Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "tag",            Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "thread",         Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "thread",         Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "to",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "to",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
     { }
 };
@@ -318,7 +336,8 @@ _sexp_expand_query (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_BAD_QUERY_SYNTAX;
     }
 
-    status = _sexp_combine_query (notmuch, NULL, NULL, prefix->xapian_op, prefix->initial, sx,
+    status = _sexp_combine_query (notmuch, NULL, NULL, prefix->xapian_op,
+				  _sexp_initial_query (prefix->initial), sx,
 				  subquery);
     if (status)
 	return status;
@@ -370,7 +389,8 @@ _sexp_parse_header (notmuch_database_t *notmuch, const _sexp_prefix_t *parent,
 
     parent = &user_prefix;
 
-    return _sexp_combine_query (notmuch, parent, env, Xapian::Query::OP_AND, Xapian::Query::MatchAll,
+    return _sexp_combine_query (notmuch, parent, env, Xapian::Query::OP_AND,
+				xapian_query_match_all (),
 				sx->list->next, output);
 }
 
@@ -520,7 +540,7 @@ _sexp_parse_range (notmuch_database_t *notmuch,  const _sexp_prefix_t *prefix,
 
     /* empty range matches everything */
     if (! sx) {
-	output = Xapian::Query::MatchAll;
+	output = xapian_query_match_all ();
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
@@ -628,7 +648,7 @@ _sexp_to_xapian_query (notmuch_database_t *notmuch, const _sexp_prefix_t *parent
 
     /* Empty list */
     if (! sx->list) {
-	output = Xapian::Query::MatchAll;
+	output = xapian_query_match_all ();
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
@@ -704,7 +724,8 @@ _sexp_to_xapian_query (notmuch_database_t *notmuch, const _sexp_prefix_t *parent
 		return _sexp_expand_query (notmuch, prefix, parent, env, sx->list->next, output);
 	    }
 
-	    return _sexp_combine_query (notmuch, parent, env, prefix->xapian_op, prefix->initial,
+	    return _sexp_combine_query (notmuch, parent, env, prefix->xapian_op,
+					_sexp_initial_query (prefix->initial),
 					sx->list->next, output);
 	}
     }
diff --git a/test/T810-tsan.sh b/test/T810-tsan.sh
index de98c0a9..d4482998 100755
--- a/test/T810-tsan.sh
+++ b/test/T810-tsan.sh
@@ -92,7 +92,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 if [ $NOTMUCH_HAVE_SFSEXP -eq 1 ]; then
     test_begin_subtest "sexp query"
-    test_subtest_known_broken
     test_C ${MAIL_DIR} ${MAIL_DIR}-2 <<EOF
 #include <notmuch-test.h>
 #include <pthread.h>

Re: [PATCH 2/2] lib: thread-safe s-expression query parser

[Kevin Boulain]

On 2023-07-22 at 16:06 -03, David Bremner <david@tethera.net> wrote:
> My first thought was that this should be static, but maybe it doesn't
> matter in C++; I see the other inline functions in that file are not
> declared static.

True, I should have marked this function static. I did the same for the
others.

> I stumbled over the logic this code. I would prefer not to use assert
> for this. In a few other places we call INTERNAL_ERROR from a default:
> case.

Thanks for the pointer, I removed the assert.

Re: [PATCH v2 1/2] test: showcase thread-unsafe s-expression query parser

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> The test fails reliably when Notmuch is compiled as such:
>   ./configure
>   make CFLAGS=-fsanitize=thread LDFLAGS=-fsanitize=thread
> It's still unclear how the test suite could be run with the correct
> compilation flags.

At the moment I can think of 3 options:

1) have the test file check if the library is built with tsan, and to skip
if not. Something like "nm lib/libnotmuch.a| grep -q __tsan_init" seems
to do the trick here, although I don't know how portable it is.

2) Provide a "--with-tsan" option to set CFLAGS/LDFLAGS and also
   some variable that the tests can check

3) Do (2) by default if TSAN is detected, and provide --without-tsan to
disable it. This is only possible if the ruby bindings build is fixed to
ignore/work-with TSAN; currently it pretty much explodes.

All three options would need some thought as to how to support (if at
all) testing installed notmuch (new in 0.38, used by some CI systems).