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

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 3 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" ]
   }
----------------
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`.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:91
       } else {
-        command = "$ld {{ldflags}} -o $outfile {{libs}} -Wl,--start-group {{inputs}} -Wl,--end-group"
+        command = "$ld {{ldflags}} -o $outfile -Wl,--start-group {{inputs}} {{libs}} -Wl,--end-group"
       }
----------------
thakis wrote:
> pcc wrote:
> > Nit: you could keep `{{libs}}` and `{{inputs}}` in the same order as on Mac.
> D81035 just puts {{libs}} after the inputs group. Is that sufficient?
sure - i'll leave it here for now and resolve the merge conflict once that patch lands


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80591