[PATCH] D71131: gn build: Change scudo's list of supported platforms to a whitelist.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D71131#1773161 <https://reviews.llvm.org/D71131#1773161>, @thakis wrote:

> Thanks! lg.
>
> I'd probably find it easier to understand, or at least more self-consistent, if this file had an assert(is_linux || is_fuchsia), and llvm/utils/gn/secondary/BUILD.gn had just the dep on //compiler-rt it already had, and the supported_toolchain stuff in this file wasn't here and instead relied on secondary/compiler-rt/BUILD.gn, and secondary/compiler-rt/lib/BUILD.gn would add a dep to scudo only if target_os == linux || fuchsia.


I'm not sure if we should go in that direction. I often use `ninja X` (where X = scudo, hwasan, etc) to build that specific runtime on all supported (i.e. supported in the current build) platforms. (This doesn't work for profile or builtins right now, but I've been able to get by for now with the `compiler-rt` target.) And in that layout `check-X` would depend on `X`. That suggests splitting on toolchains before splitting on runtimes, i.e. something like this in compiler-rt/BUILD.gn:

  group("compiler-rt") {
    deps = [ ":scudo", ":hwasan", ":builtins", ":profile" ]
  }
  
  android_supported_toolchains = []
  if (android_ndk_path != "") {
    android_supported_toolchains += [
      "//llvm/utils/gn/build/toolchain:stage2_android_aarch64",
      "//llvm/utils/gn/build/toolchain:stage2_android_arm",
    ]
  }
  
  scudo_supported_toolchains = android_supported_toolchains
  if (target_os == "linux" || target_os == "fuchsia") {
    scudo_supported_toolchains += [ "stage2_unix" ]
  }
  group("scudo") {
    deps = [ "lib/scudo($toolchain)" for toolchain in scudo_supported_toolchains ]
  }
  
  # etc for other runtimes

I guess a side benefit of this sort of layout is that all of the "is this toolchain supported" logic would be in fewer places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71131