This is an archive of the discontinued LLVM Phabricator instance.

gn build: Change scudo's list of supported platforms to a whitelist.
ClosedPublic

Authored by pcc on Dec 6 2019, 10:22 AM.

Details

Summary

Scudo only supports building for android/linux/fuchsia, so require target_os to
be one of linux/fuchsia to do a stage2_unix scudo build. Android is already
covered by the stage2_android* toolchains below.

Diff Detail

Event Timeline

pcc created this revision.Dec 6 2019, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2019, 10:22 AM

Build result: pass - 60563 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

thakis accepted this revision.Dec 6 2019, 11:10 AM

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.

This revision is now accepted and ready to land.Dec 6 2019, 11:10 AM
pcc added a comment.Dec 6 2019, 1:39 PM

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.

thakis added a comment.Dec 6 2019, 2:21 PM

Sounds fine to me too, as long as all the compiler-rt subdirs do the same thing.

This revision was automatically updated to reflect the committed changes.