[PATCH] D80591: Patch up issues with GN builds (pthread / libz)

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 2 inline comments as done.
hctim added inline comments.


================
Comment at: llvm/utils/gn/build/libs/pthread/BUILD.gn:10
+  if (llvm_enable_threads && current_os != "win") {
+    all_dependent_configs = [ ":pthread_config" ]
   }
----------------
hctim wrote:
> thakis wrote:
> > hctim wrote:
> > > thakis wrote:
> > > > `all_dependent_configs` is an anti pattern and we shouldn't use it. Why is this part of the change needed?
> > > 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`.
> > 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?
> ```
> 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.
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` :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80591/new/

https://reviews.llvm.org/D80591