Patch up pthread issues with GN build

[hctim]

Summary

Ensures that the correct ldflag -pthread is provided, not just linking the library.

[hctim]

Move dependencies inside the group.

[hctim]

Rebased incorrectly.

[pcc]

llvm/utils/gn/build/libs/pthread/BUILD.gn

5

We shouldn't need both -pthread and -lpthread. Can you remove this one?

15

The && current_os != "android" part should now be unnecessary with the change above assuming that -pthread does nothing in the Android driver.

llvm/utils/gn/build/toolchain/BUILD.gn

91 ↗

(On Diff #266345)

Nit: you could keep {{libs}} and {{inputs}} in the same order as on Mac.

[hctim]

• Address Peter's comments.

[hctim]

• remove redundant comment about Android.

[hctim]

@pcc - LGTY?

[hctim]

Friendly ping

[thakis]

Thanks for the patch! Look great, except for the all_dependent_libs bit.

llvm/utils/gn/build/libs/pthread/BUILD.gn

17

all_dependent_configs is an anti pattern and we shouldn't use it. Why is this part of the change needed?

llvm/utils/gn/build/toolchain/BUILD.gn

91 ↗

(On Diff #266345)

D81035 just puts {{libs}} after the inputs group. Is that sufficient?

[hctim]

llvm/utils/gn/build/libs/pthread/BUILD.gn

17

If a transitive dependency needs pthreads, we need to supply it at link time.

all_dependent_configs seems like the right knob here - as otherwise anybody that transitively depends on pthreads must be included ONLY via. public_deps.

e.g. there's a current chain from
hwasan_shared -> sanitizer_common/sources -> pthread

if we were to opt not to use all_dependent_configs, then:

1. sanitizer_common/sources needs to use public_deps = [ "path/to/pthread" ] (reasonable at this point)
2. hwasan_shared needs to use public_deps = [ "sanitizer_common/sources" ] or public_deps = [ "path/to/pthread" ] (yuck!), and same with anybody who depends on hwasan_shared.

llvm/utils/gn/build/toolchain/BUILD.gn

91 ↗

(On Diff #266345)

sure - i'll leave it here for now and resolve the merge conflict once that patch lands

[thakis]

llvm/utils/gn/build/libs/pthread/BUILD.gn

17

ldflags should propagate until the first linkable (ie executable or shared library) but no further. If sanitizer_common/sources is a static lib or a source set, the ldflag it gets from its config should make it to the link of hwasan_shared as far as I understand. Is that not what's happening?

And if you have something that depends on hwasan_shared, you wouldn't want _that_ to link to pthreads unless it actively asks for it, right?

[hctim]

llvm/utils/gn/build/libs/pthread/BUILD.gn

17

deps: Private linked dependencies.
  A list of target labels.

  Specifies private dependencies of a target. Private dependencies are
  propagated up the dependency tree and linked to dependant targets, but
  do not grant the ability to include headers from the dependency.
  ***Public configs are not forwarded.***

(emphasis mine)

And if you have something that depends on hwasan_shared, you wouldn't want _that_ to link to pthreads unless it actively asks for it, right?

Yes. Probably a bad example on my part, but it would be nice to stop the dependency chain at shared objects.

[hctim]

llvm/utils/gn/build/libs/pthread/BUILD.gn

17

Looking through the GN docs for a bit I couldn't seem to find an incantation that would allow us to be able to have ldflags bubble up to a DSO or executable (whichever came first) - hence the shotgun approach of all_dependent_configs :(

[hctim]

• Rebase against master now that the library link order is fixed by D81035.

[hctim]

Hey Nico - do you by any chance know the magic sauce that would forward ldflags += -pthread up to the nearest parent DSO or executable so we don't have to all_dependent_configs shotgun it? Thanks :)

[hctim]

Looks like D80599 didn't squash properly before being upstreamed, this commit is still unreviewed and for all intents and purposes unsubmitted (and still needs review).

"committed" in [9e9142cbb90901cd0e96a73d14382721250a4069](https://github.com/llvm/llvm-project/commit/9e9142cbb90901cd0e96a73d14382721250a4069)
... and was reverted in [e26b25f8b1fdbc088abdfab933d909960989c64b](https://github.com/llvm/llvm-project/commit/e26b25f8b1fdbc088abdfab933d909960989c64b#diff-99847aafe752faba5f0cfefbe9ca9708)

llvm/utils/gn/build/libs/pthread/BUILD.gn

 import("//llvm/utils/gn/build/libs/pthread/enable.gni")
 
 config("pthread_config") {
   visibility = [ ":pthread" ]
   libs = [ "pthread" ]
 }
 
+config("pthread_link_time_config") {
+  visibility = [ ":pthread" ]
+  ldflags = [ "-pthread" ]
+}
+
 group("pthread") {
   # On Android, bionic has built-in support for pthreads.
   if (llvm_enable_threads && current_os != "win" && current_os != "android") {
     public_configs = [ ":pthread_config" ]
+    all_dependent_configs = [ ":pthread_link_time_config" ]
   }
 }