Horton

Thread

Commits

  1. Remove configure-time thread safety checking (thread_test.c).

  1. Configure with thread sanitizer fails the thread test

    Mikhail Matrosov <mikhail.matrosov@gmail.com> — 2021-07-23T14:27:36Z

    This is a reply to an old thread with the same name:
https://www.postgresql.org/message-id/1513971675.5870501.1439797066345.JavaMail.yahoo@mail.yahoo.com
I was not able to do a proper reply since I cannot download the raw
message: https://postgrespro.com/list/thread-id/2483942

Anyway, the problem with thread sanitizer is still present. If I try to
build libpq v13.3 with thread sanitizer, I get a configuration error like
this:

configure:18852: checking thread safety of required library functions
configure:18875: /usr/bin/clang-12 -o conftest -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -m64 -O3 -fsanitize=thread -pthread
-D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DIN_CONFIGURE
-I/home/mmatrosov/.conan/data/zlib/1.2.11/_/_/package/98c3afaf7dd035538c92258b227714d9d4a19852/include
-DNDEBUG -D_GNU_SOURCE  -m64
-L/home/mmatrosov/.conan/data/zlib/1.2.11/_/_/package/98c3afaf7dd035538c92258b227714d9d4a19852/lib
 conftest.c -lz -lm -lz  >&5
configure:18875: $? = 0
configure:18875: ./conftest
==================
WARNING: ThreadSanitizer: data race (pid=3413987)
  Write of size 4 at 0x000000f1744c by thread T2:
    #0 func_call_2 <null> (conftest+0x4b5e51)

  Previous read of size 4 at 0x000000f1744c by thread T1:
    #0 func_call_1 <null> (conftest+0x4b5d12)

  Location is global 'errno2_set' of size 4 at 0x000000f1744c
(conftest+0x000000f1744c)

  Thread T2 (tid=3413990, running) created by main thread at:
    #0 pthread_create <null> (conftest+0x424b3b)
    #1 main <null> (conftest+0x4b5b49)

  Thread T1 (tid=3413989, running) created by main thread at:
    #0 pthread_create <null> (conftest+0x424b3b)
    #1 main <null> (conftest+0x4b5b2e)

...

configure:18879: result: no
configure:18881: error: thread test program failed
This platform is not thread-safe.  Check the file 'config.log' or compile
and run src/test/thread/thread_test for the exact reason.
Use --disable-thread-safety to disable thread safety.


I am not sure what is the proper fix for the issue, but I at least suggest
to disable this test when building with thread sanitizer:
https://github.com/conan-io/conan-center-index/pull/6472/files#diff-b8639f81e30f36c8ba29a0878f1ef4d9f1552293bc9098ebb9b429ddb1f0935f

-----
Best regards, Mikhail Matrosov

  2. Re: Configure with thread sanitizer fails the thread test

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-07-23T17:35:22Z

    On 2021-Jul-23, Mikhail Matrosov wrote:

> I am not sure what is the proper fix for the issue, but I at least suggest
> to disable this test when building with thread sanitizer:
> https://github.com/conan-io/conan-center-index/pull/6472/files#diff-b8639f81e30f36c8ba29a0878f1ef4d9f1552293bc9098ebb9b429ddb1f0935f

Here's the proposed patch.  Patches posted to the mailing list by their
authors proposed for inclusion are considered to be under the PostgreSQL
license, but this patch hasn't been posted by the author so let's assume
they're not authorizing them to use it.  (Otherwise, why wouldn't they
just post it here instead of doing the absurdly convoluted dance of a
github PR?)

diff --git a/src/test/thread/thread_test.c b/src/test/thread/thread_test.c
index e1bec01..e4ffd78 100644
--- a/src/test/thread/thread_test.c
+++ b/src/test/thread/thread_test.c
@@ -61,6 +61,14 @@ main(int argc, char *argv[])
 	fprintf(stderr, "Perhaps rerun 'configure' using '--enable-thread-safety'.\n");
 	return 1;
 }
+
+#elif __has_feature(thread_sanitizer)
+int
+main(int argc, char *argv[])
+{
+	printf("Thread safety check is skipped since it does not work with thread sanitizer.\n");
+	return 0;
+}
 #else
 
 /* This must be down here because this is the code that uses threads. */

-- 
Álvaro Herrera

  3. Re: Configure with thread sanitizer fails the thread test

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-07-23T17:36:54Z

    On 2021-Jul-23, Alvaro Herrera wrote:

> On 2021-Jul-23, Mikhail Matrosov wrote:
> 
> > I am not sure what is the proper fix for the issue, but I at least suggest
> > to disable this test when building with thread sanitizer:
> > https://github.com/conan-io/conan-center-index/pull/6472/files#diff-b8639f81e30f36c8ba29a0878f1ef4d9f1552293bc9098ebb9b429ddb1f0935f
> 
> Here's the proposed patch.  Patches posted to the mailing list by their
> authors proposed for inclusion are considered to be under the PostgreSQL
> license, but this patch hasn't been posted by the author so let's assume
> they're not authorizing them to use it.  (Otherwise, why wouldn't they
> just post it here instead of doing the absurdly convoluted dance of a
> github PR?)

... that said, I wonder why would we do this in the thread_test program
rather than in configure itself.  Wouldn't it make more sense for the
configure test to be skipped altogether (marking the result as
thread-safe) when running under thread sanitizer, if there's a way to
detect that?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

  4. Re: Configure with thread sanitizer fails the thread test

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-23T17:42:41Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> ... that said, I wonder why would we do this in the thread_test program
> rather than in configure itself.  Wouldn't it make more sense for the
> configure test to be skipped altogether (marking the result as
> thread-safe) when running under thread sanitizer, if there's a way to
> detect that?

TBH, I wonder why we don't just nuke thread_test.c altogether.
Is it still useful in 2021?  Machines that still need
--disable-thread-safety can doubtless be counted without running
out of fingers, and I think their owners can be expected to know
that they need that.

			regards, tom lane

  5. Re: Configure with thread sanitizer fails the thread test

    Bruce Momjian <bruce@momjian.us> — 2021-07-23T18:01:24Z

    On Fri, Jul 23, 2021 at 01:42:41PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > ... that said, I wonder why would we do this in the thread_test program
> > rather than in configure itself.  Wouldn't it make more sense for the
> > configure test to be skipped altogether (marking the result as
> > thread-safe) when running under thread sanitizer, if there's a way to
> > detect that?
> 
> TBH, I wonder why we don't just nuke thread_test.c altogether.
> Is it still useful in 2021?  Machines that still need
> --disable-thread-safety can doubtless be counted without running
> out of fingers, and I think their owners can be expected to know
> that they need that.

I think it is fine to remove it.  It was really designed to just try a
lot of flags to see what the compiler required, but things have become
much more stable since it was written.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  6. Re: Configure with thread sanitizer fails the thread test

    Andres Freund <andres@anarazel.de> — 2021-07-23T19:57:41Z

    Hi,

On 2021-07-23 13:42:41 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > ... that said, I wonder why would we do this in the thread_test program
> > rather than in configure itself.  Wouldn't it make more sense for the
> > configure test to be skipped altogether (marking the result as
> > thread-safe) when running under thread sanitizer, if there's a way to
> > detect that?
> 
> TBH, I wonder why we don't just nuke thread_test.c altogether.
> Is it still useful in 2021?  Machines that still need
> --disable-thread-safety can doubtless be counted without running
> out of fingers, and I think their owners can be expected to know
> that they need that.

+1. And before long it might be time to remove support for systems
without threads...

Greetings,

Andres Freund

  7. Re: Configure with thread sanitizer fails the thread test

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-23T21:18:37Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-07-23 13:42:41 -0400, Tom Lane wrote:
>> TBH, I wonder why we don't just nuke thread_test.c altogether.

> +1. And before long it might be time to remove support for systems
> without threads...

I'm not prepared to go that far just yet; but certainly we can stop
expending configure cycles on the case.

Should we back-patch this, or just do it in HEAD?  Maybe HEAD+v14?

			regards, tom lane

  8. Re: Configure with thread sanitizer fails the thread test

    Mikhail Matrosov <mikhail.matrosov@gmail.com> — 2021-07-23T21:43:32Z

    Hi, Alvaro and all,

> this patch hasn't been posted by the author so let's assume
> they're not authorizing them to use it.

Not sure what you mean. I am the author and I authorize anyone to do
whatever they want with it.

> Otherwise, why wouldn't they just post it here instead of doing the
absurdly convoluted dance of a github PR?

Well, for me submitting a github PR is a well-established and easy-to-do
procedure which is the same for all libraries. Posting to a new mailing
list definitely is not. I've spent around 15 minutes trying to do a proper
reply to the thread and did not succeed.

Another reason to post a PR is that we consume libpq via conan, and
releasing a new revision of the conan recipe for the same library version
is a relatively fast and well-defined process. While waiting for a new
version of a library with a patch depends heavily on a particular library.
I am not aware of the release cadence of libpq.

Anyway, I am very glad there is swift feedback from you guys and I am
looking forward to your comments on the proper way to fix the issue!

-----
Best regards, Mikhail Matrosov

  9. Re: Configure with thread sanitizer fails the thread test

    Andres Freund <andres@anarazel.de> — 2021-07-23T21:51:42Z

    Hi,

On 2021-07-23 17:18:37 -0400, Tom Lane wrote:
> I'm not prepared to go that far just yet; but certainly we can stop
> expending configure cycles on the case.
> 
> Should we back-patch this, or just do it in HEAD?  Maybe HEAD+v14?

I'm ok with all, with a preference for HEAD+v14, followed by HEAD, and
all branches.

Greetings,

Andres Freund

  10. Re: Configure with thread sanitizer fails the thread test

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-23T22:05:46Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-07-23 17:18:37 -0400, Tom Lane wrote:
>> Should we back-patch this, or just do it in HEAD?  Maybe HEAD+v14?

> I'm ok with all, with a preference for HEAD+v14, followed by HEAD, and
> all branches.

After a bit more thought, HEAD+v14 is also my preference.  I'm not
eager to change this in stable branches, but it doesn't seem too
late for v14.

			regards, tom lane

  11. Re: Configure with thread sanitizer fails the thread test

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-24T16:20:23Z

    I wrote:
> After a bit more thought, HEAD+v14 is also my preference.  I'm not
> eager to change this in stable branches, but it doesn't seem too
> late for v14.

Perusing the commit log, I realized that there's another reason why
v14 is a good cutoff: thread_test.c was in a different place with
an allegedly different raison d'etre before 8a2121185.  So done
that way.

			regards, tom lane