This is an archive of the discontinued LLVM Phabricator instance.

Patch up pthread issues with GN build
Needs ReviewPublic

Authored by hctim on May 26 2020, 3:05 PM.

Details

Summary

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

Diff Detail

Event Timeline

hctim created this revision.May 26 2020, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 3:05 PM
hctim updated this revision to Diff 266344.May 26 2020, 3:07 PM

Move dependencies inside the group.

hctim updated this revision to Diff 266345.May 26 2020, 3:07 PM

Rebased incorrectly.

pcc added inline comments.May 26 2020, 5:27 PM
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.

Harbormaster completed remote builds in B57944: Diff 266341.
hctim updated this revision to Diff 266580.May 27 2020, 9:39 AM
hctim marked 3 inline comments as done.
  • Address Peter's comments.
hctim updated this revision to Diff 266581.May 27 2020, 9:40 AM
  • remove redundant comment about Android.
Harbormaster completed remote builds in B58073: Diff 266580.
hctim added a comment.Jun 2 2020, 1:35 PM

Friendly ping

thakis added a comment.Jun 2 2020, 3:42 PM

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 marked 3 inline comments as done.Jun 2 2020, 4:35 PM
hctim added inline comments.
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 added inline comments.Jun 2 2020, 4:50 PM
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 marked 4 inline comments as done.Jun 2 2020, 5:54 PM
hctim added inline comments.
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 marked 2 inline comments as done.Jun 2 2020, 6:29 PM
hctim added inline comments.
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 updated this revision to Diff 269270.Jun 8 2020, 9:35 AM
  • Rebase against master now that the library link order is fixed by D81035.
hctim retitled this revision from Patch up issues with GN builds (pthread / libz) to Patch up pthread issues with GN build.Jun 8 2020, 9:35 AM
hctim edited the summary of this revision. (Show Details)
hctim added a comment.Jun 8 2020, 9:40 AM

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 :)

This revision was not accepted when it landed; it landed in state Needs Review.Jun 9 2020, 1:15 PM
This revision was automatically updated to reflect the committed changes.
hctim reopened this revision.Jun 9 2020, 1:34 PM

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)