Bug 239038 - devel/protobuf: Missing -pthread linker flag after 3.8.0 upgrade on 11.x breaks consumers (eg: net/mosh)
Summary: devel/protobuf: Missing -pthread linker flag after 3.8.0 upgrade on 11.x brea...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Sunpoet Po-Chuan Hsieh
URL:
Keywords: needs-qa, regression
: 239379 (view as bug list)
Depends on: 239528
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-07 19:59 UTC by Alarig Le Lay
Modified: 2019-09-03 17:31 UTC (History)
7 users (show)

See Also:
bugzilla: maintainer-feedback? (sunpoet)


Attachments
/usr/local/etc/poudriere.d/make.conf (162 bytes, text/plain)
2019-07-08 07:03 UTC, Alarig Le Lay
no flags Details
pkg version -v (18.83 KB, text/plain)
2019-07-08 07:06 UTC, Alarig Le Lay
no flags Details
full build log of mosh (39.13 KB, text/plain)
2019-07-08 07:12 UTC, Alarig Le Lay
no flags Details
patch with fix (304 bytes, patch)
2019-07-28 10:20 UTC, Michael Bueker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alarig Le Lay 2019-07-07 19:59:45 UTC
Hi,

The port mush has been upgraded on my machine this night, since then, I can’t log into it any more.

~ % mosh mew.swordarmor.fr                           
Enter passphrase for key '/home/alarig/.ssh/id_rsa_perso': 
[libprotobuf FATAL google/protobuf/generated_message_util.cc:809] CHECK failed: (scc->visit_status.load(std::memory_order_relaxed)) == (SCCInfoBase::kRunning): 
Connection to mew.swordarmor.fr closed.
/usr/bin/mosh: Did not find mosh server startup message. (Have you installed mosh on your server?)
zsh: exit 10    LANG=fr_FR.UTF-8 mosh mew.swordarmor.fr

root@mew:~ # pkg info mosh
mosh-1.3.2_9
Name           : mosh
Version        : 1.3.2_9
Installed on   : Sun Jul  7 00:01:16 2019 CEST
Origin         : net/mosh
Architecture   : FreeBSD:11:amd64
Prefix         : /usr/local
Categories     : net
Licenses       : GPLv3
Maintainer     : zi@FreeBSD.org
WWW            : https://mosh.org/
Comment        : Mobile terminal that supports intermittent connectivity
Shared Libs required:
        libprotobuf.so.19
Annotations    :
        FreeBSD_version: 1102000
        repo_type      : binary
        repository     : poudriere
Flat size      : 654KiB
Description    :
Remote terminal application that allows roaming, supports intermittent
connectivity, and provides intelligent local echo and line editing of
user keystrokes.

Mosh is a replacement for SSH. It's more robust and responsive,
especially over Wi-Fi, cellular, and long-distance links.

WWW: https://mosh.org/
root@mew:~ # uname -a
FreeBSD mew.swordarmor.fr 11.2-RELEASE-p9 FreeBSD 11.2-RELEASE-p9 #0: Tue Feb  5 15:30:36 UTC 2019     root@amd64-builder.daemonology.net:/usr/obj/usr/src/sys/GENERIC  amd64
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-07-08 03:03:17 UTC
@Alarig Can you please provide:

- /etc/make.conf contents (as an attachment)
- pkg version -v output (as an attachment)
- a full build log of mosh (as an attachment)

1.3.2_9 was a portrevision bump in ports r505909 to account for a devel/protobuf update to 3.8.0 (cc committer)

See Also: 

https://github.com/protocolbuffers/protobuf/issues/5923
https://github.com/protocolbuffers/protobuf/issues/6074
https://stackoverflow.com/questions/52890529/fatalexception-thrown-by-protobuf-library-on-instantiation-of-message
Comment 2 Alarig Le Lay 2019-07-08 07:03:24 UTC
Created attachment 205577 [details]
/usr/local/etc/poudriere.d/make.conf

The make.conf file from my poudriere
Comment 3 Alarig Le Lay 2019-07-08 07:06:53 UTC
Created attachment 205578 [details]
pkg version -v

Ran on the VM running mosh
Comment 4 Alarig Le Lay 2019-07-08 07:12:22 UTC
Created attachment 205579 [details]
full build log of mosh

Building mosh on the poudriere
Comment 5 Alarig Le Lay 2019-07-08 07:13:39 UTC
Hi Kubilay,

Please find the requested outputs as attachments.
Comment 6 Michael Bueker 2019-07-08 11:37:20 UTC
I can confirm this bug (as a user of portmaster) for versions:
- devel/protobuf-3.8.0,1
- net/mosh-1.3.2_9
Comment 7 Michael Bueker 2019-07-22 10:28:01 UTC
As per the discussion in https://stackoverflow.com/questions/52890529/fatalexception-thrown-by-protobuf-library-on-instantiation-of-message, this might be related to the (non-?)inclusion of pthread when compiling protobuf. So, this might actually be a protobuf compilation bug.
Comment 8 clive 2019-07-22 15:22:36 UTC
Hi,

(I'm redirected here by protobuf MAINTAINER to join the discussion)

This only happens on FreeBSD 11.2 and 11.3, and the root cause is configure script of protobuf, but only exposed after protobuf 3.8 upgrade, in this protobuf commit:

https://github.com/protocolbuffers/protobuf/commit/4b389fad575f830fee4a39287ebfd53a6f0862b2#diff-e0c3a6028601b4402de2b802df8542e2

Removing PTHREAD_CFLAGS from Libs is absolutely right. The question is, why PTHREAD_LIBS is empty? I think around line 250 of this file is why:

https://github.com/protocolbuffers/protobuf/blob/d9ccd0c0e6bbda9bf4476088eeb46b02d7dcd327/m4/ax_pthread.m4

The second note at line 258, which is true on macOS, FreeBSD 12, FreeBSD 13, but unfortunately not on FreeBSD 11.

I propose a simple one line patch to fix protobuf. This is only tested on FreeBSD 11, though. I would like to test this on FreeBSD 12 and 13, though my resource and time is quite limited. Really appreciated for anyone could help.

--- configure~  2019-07-22 15:39:35.886057000 +0800
+++ configure   2019-07-22 15:40:41.618362000 +0800
@@ -20122,7 +20122,7 @@
        # ignore this macro, third-party headers might not.)

        PTHREAD_CFLAGS="-pthread"
-       PTHREAD_LIBS=
+       PTHREAD_LIBS="-pthread"

        ax_pthread_ok=yes
Comment 9 Michael Bueker 2019-07-23 22:08:09 UTC
As per clive's request, I have tested and confirmed that:

- on FreeBSD 11, the proposed patch solves the problem

- on FreeBSD 12, the proposed patch causes no problem for protobuf compilation and mosh functionality

- on FreeBSD 13, the proposed patch causes no problem for protobuf compilation and mosh functionality

When the patch is committed, it will close this bug and also bug #239379.
Comment 10 Michael Bueker 2019-07-28 10:20:09 UTC
Created attachment 206110 [details]
patch with fix

patch with bugfix (tested on FreeBSD 11, 12, 13) by clive
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2019-07-30 10:35:35 UTC
*** Bug 239379 has been marked as a duplicate of this bug. ***
Comment 12 Kubilay Kocak freebsd_committer freebsd_triage 2019-07-30 10:38:17 UTC
(In reply to Michael Bueker from comment #10)

Could you test to see if adding CONFIGURE_ENV+=PTHREAD_LIBS="-pthread" to devel/protobuf/Makefile also works, as this will preclude the need to carry a patch to WRKSRC/configure, which is preferable.

We should probably report this upstream so that developers can resolve the root cause correctly/permanently there
Comment 13 Michael Bueker 2019-07-30 13:04:07 UTC
(In reply to Kubilay Kocak from comment #12)

Sadly, CONFIGURE_ENV+=PTHREAD_LIBS="-pthread" in the Makefile does not work. It looks like the configure script overrides that environmental variable. Hence, it looks like the patch is needed.
Comment 14 clive 2019-07-30 13:46:06 UTC
(In reply to Kubilay Kocak from comment #12)

Hi,

    It does not work, since the protobuf.pc is generated by configure script (from protobuf.pc.in template), and the pthread_clang part of the configure script does not honor CONFIGURE_ENV for PTHREAD_LIBS. Just did the test in a fresh install 11.3 VM to confirm that.

    FreeBSD quarterly package still contains old version, so mosh still works for pkg fans at this moment, and other packages depend on protobuf.

    Since quarterly package still works, and protobuf 3.9.0 is out, MAINTAINER of protobuf plans to include the configure patch in next update.

    For ports fans who need a working mosh, please try patch below. This is really just a workaround, since now we know the root cause is protobuf, and only happens on FreeBSD 11. 

# /usr/ports/net/mosh % diff -u Makefile~ Makefile
--- Makefile~   2019-07-27 04:46:53.000000000 +0800
+++ Makefile    2019-07-30 21:37:08.520312000 +0800
@@ -19,7 +19,7 @@
 GNU_CONFIGURE= yes
 CONFIGURE_ARGS=        --with-utempter --without-ncurses
 CONFIGURE_ENV+=        CRYPTO_CFLAGS="-I${OPENSSLINC}" CRYPTO_LIBS="-L${OPENSSLLIB} -lssl -lcrypto" \
-               TINFO_CFLAGS="-I/usr/include"
+               TINFO_CFLAGS="-I/usr/include" LIBS="-pthread"
 INSTALL_TARGET=        install-strip
 USES=          compiler:c++11-lang ncurses perl5 pkgconfig ssl
 USE_CXXSTD=    c++11
Comment 15 Sunpoet Po-Chuan Hsieh freebsd_committer 2019-07-30 16:00:43 UTC
FYI, I've included the protobuf patch in bug #239528 (exp-run for 3.9.0 update).
Comment 16 andrew 2019-07-31 01:01:48 UTC
I'm pretty sure this is a base system bug, somewhere between rtld and the libc pthread stubs.

(There is no actual reason why one should link protobuf with -pthread, other than as a workaround for such a bug.)

I think it's most likely an initialization order problem, though I haven't got it down to a simple testcase yet.
Comment 17 andrew 2019-07-31 01:52:23 UTC
Spoke too soon. This is a disagreement between libstdc++'s std::thread and libc pthread_stubs:

std::thread::id is defined as a value that can be either the id of a thread, or a distinguished value that does not correspond to any thread (the implementation uses 0 for this).

The operator== for std::thread::id is just a call to pthread_equal.

But pthread_equal takes pthread_t values as input, which do _not_, per spec, allow for any distinguished "no thread" value. Accordingly, the stub implementation of pthread_equal thinks it's OK to just return "true" unconditionally, because in a non-threaded process there is only one legal pthread_t value, and comparing against illegal values is undefined.

Clearly this is wrong, and either the stub pthread_equal should actually do the comparison (easiest fix) or std::thread::id should avoid calling pthread_equal if one of the arguments is 0 (since doing so violates the pthread spec).

I don't think the std::thread issue is specific to 11.x; the fact that the mosh port works on 12+ is probably just down to the fact that it pulls in libthr.so from somewhere else (openssl most likely).
Comment 18 commit-hook freebsd_committer 2019-07-31 14:18:23 UTC
A commit references this bug:

Author: sunpoet
Date: Wed Jul 31 14:17:31 UTC 2019
New revision: 507713
URL: https://svnweb.freebsd.org/changeset/ports/507713

Log:
  Set PTHREAD_LIBS to -pthread after PTHREAD_CFLAGS removal from Libs in protobuf.pc.in

  - Bump PORTREVISION for package change

  PR:		239038
  Reported by:	Alarig Le Lay <alarig@swordarmor.fr>, Clive <clive@tongi.org>

Changes:
  head/devel/protobuf/Makefile
  head/devel/protobuf/files/patch-configure
Comment 19 Sunpoet Po-Chuan Hsieh freebsd_committer 2019-07-31 16:40:40 UTC
Committed. Thanks!
Comment 20 andrew 2019-07-31 17:50:23 UTC
This fix looks like it's just telling all of protobuf's consumers to link with -pthread, rather than linking libprotobuf.so _itself_ with -pthread.

I think the latter is preferable (though I'm open to arguments otherwise).
Comment 21 clive 2019-07-31 19:42:52 UTC
(In reply to andrew from comment #20)

// Ticket closed anyway, my response below is for the record only.

Hi,

    Thanks for previous comments. The real problem seems to be libprotobuf itself not linking against threading library. On FreeBSD 12 and 13, I see at least libssl.so and libcrypto.so have libthr in their dependency list. That should be why mosh works on 12 and 13.

    To link libpthread.so itself with default threading library, change configure script like this just works.

# /usr/ports/devel/protobuf % cat files/patch-configure
--- configure.orig      2019-07-12 16:18:37 UTC
+++ configure
@@ -20165,7 +20165,7 @@ if test "x$ax_pthread_clang" = "xyes"; t
        # ignore this macro, third-party headers might not.)

        PTHREAD_CFLAGS="-pthread"
-       PTHREAD_LIBS=
+       PTHREAD_LIBS="-lthr"

        ax_pthread_ok=yes

    It looks like clang and ld need different flag for threading library. Now I do not have enough knowledge for correct-and-elegant fix. Maybe someone knows more about LLVM and FreeBSD internals, can eventually fix this in protobuf upstream.
Comment 22 clive 2019-07-31 19:50:21 UTC
(In reply to clive from comment #21)

"To link libpthread.so itself..." should be

"To link libprotobuf.so itself..."

Sorry for the autocorrect typo.
Comment 23 andrew 2019-07-31 21:17:17 UTC
(In reply to clive from comment #21)

That change doesn't affect the linking of libprotobuf.so itself, it only affects the contents of the .pc file which is used by consumers of the library.

As the port currently stands (i.e. after the "fix"), I see this:

% ldd /usr/local/lib/libprotobuf.so                   
/usr/local/lib/libprotobuf.so:
        libz.so.6 => /lib/libz.so.6 (0x8014ff000)
        libc++.so.1 => /usr/lib/libc++.so.1 (0x801717000)
        libcxxrt.so.1 => /lib/libcxxrt.so.1 (0x8019e6000)
        libm.so.5 => /lib/libm.so.5 (0x801c05000)
        libc.so.7 => /lib/libc.so.7 (0x800825000)
        libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x801e34000)

Note that libthr is not referenced here, so this is (in my opinion) the wrong fix.
Comment 24 commit-hook freebsd_committer 2019-08-20 17:39:57 UTC
A commit references this bug:

Author: dim
Date: Tue Aug 20 17:39:34 UTC 2019
New revision: 351253
URL: https://svnweb.freebsd.org/changeset/base/351253

Log:
  Pull in r368867 from upstream libc++ trunk (by Marshall Clow):

    Rework recursive_timed_mutex so that it uses __thread_id instead of
    using the lower-level __libcpp_thread_id. This is prep for fixing
    PR42918. Reviewed as https://reviews.llvm.org/D65895

  Pull in r368916 from upstream libc++ trunk (by Marshall Clow):

    Fix thread comparison by making sure we never pass our special 'not a
    thread' value to the underlying implementation. Fixes PR#42918.

  This should fix std::thread::id::operator==() attempting to call
  pthread_equal(3) with zero values.

  Reported by:	andrew@tao11.riddles.org.uk
  PR:		239038, 239550
  MFC after:	3 days

Changes:
  head/contrib/libc++/include/__threading_support
  head/contrib/libc++/include/mutex
  head/contrib/libc++/include/thread
  head/contrib/libc++/src/mutex.cpp
Comment 25 commit-hook freebsd_committer 2019-09-03 17:31:17 UTC
A commit references this bug:

Author: dim
Date: Tue Sep  3 17:31:14 UTC 2019
New revision: 351767
URL: https://svnweb.freebsd.org/changeset/base/351767

Log:
  MFC r351253:

  Pull in r368867 from upstream libc++ trunk (by Marshall Clow):

    Rework recursive_timed_mutex so that it uses __thread_id instead of
    using the lower-level __libcpp_thread_id. This is prep for fixing
    PR42918. Reviewed as https://reviews.llvm.org/D65895

  Pull in r368916 from upstream libc++ trunk (by Marshall Clow):

    Fix thread comparison by making sure we never pass our special 'not a
    thread' value to the underlying implementation. Fixes PR#42918.

  This should fix std::thread::id::operator==() attempting to call
  pthread_equal(3) with zero values.

  Reported by:	andrew@tao11.riddles.org.uk
  PR:		239038, 239550

Changes:
_U  stable/11/
  stable/11/contrib/libc++/include/__threading_support
  stable/11/contrib/libc++/include/mutex
  stable/11/contrib/libc++/include/thread
  stable/11/contrib/libc++/src/mutex.cpp
_U  stable/12/
  stable/12/contrib/libc++/include/__threading_support
  stable/12/contrib/libc++/include/mutex
  stable/12/contrib/libc++/include/thread
  stable/12/contrib/libc++/src/mutex.cpp