[PATCH] D56628: gn build: Add build files for compiler-rt/lib/{hwasan, interception, sanitizer_common, ubsan}.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/utils/gn/build/BUILD.gn:138
 
+config("crt_code") {
+  include_dirs = [ "//compiler-rt/lib" ]
----------------
thakis wrote:
> "crt" means "c runtime library" in my head. I guess "compiler_rt_code" is too long for you?
I have a mild preference for `crt_` as I was going to name variables and such `crt_foo` in upcoming patches, and I wanted the prefix to be short and consistent. But if you think `crt_` is  too confusing, let me know.


================
Comment at: llvm/utils/gn/secondary/BUILD.gn:15
     # depended on from here.
-    deps += [ "//llvm/tools/llvm-symbolizer(//llvm/utils/gn/build/toolchain:stage2_android_aarch64)" ]
+    deps += [
+      "//compiler-rt/lib/hwasan:hwasan_shared(//llvm/utils/gn/build/toolchain:stage2_android_aarch64)",
----------------
thakis wrote:
> Maybe
> 
> ```
> android_aarch64_toolchain = "//llvm/utils/gn/build/toolchain:stage2_android_aarch64"
> deps += [
>   ""//compiler-rt/lib/hwasan:hwasan_shared($android_aarch64_toolchain)"
>   ...
> ```
> 
> ?
Sure, that'll do for now, although I intend to replace this whole block with a reference to a test target (without the toolchain stuff since it will be refactored inside the test target) in an upcoming patch.


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn:53
+  configs -= [ "//llvm/utils/gn/build:llvm_code" ]
+  configs += [ "//llvm/utils/gn/build:crt_code" ]
+}
----------------
thakis wrote:
> In all the other build files, I kept these alphabetical (config, deps, sources) (except that I put public_configs next to configs). I thought I saw a recommended order somewhere once but couldn't find it. If this is the recommended order, can you point me to where the recommendation is? Then I'll change all the other build files. Else, if I'm making up the memory of there being a public recommendation, it's probably good to make the order here consistent with the order in all the existing build files.
I've done this for the library targets (and removed empty lines and moved `output_*` to the top since that seems to be the style too). I couldn't find a consistent style for action so I left as is.


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn:79
+
+  complete_static_lib = true
+}
----------------
thakis wrote:
> I think we currently unconditionally pass T to ar; probably need some mechanism to not do this for static libaries you want to be "really" complete. (cf https://cs.chromium.org/chromium/src/build/config/compiler/BUILD.gn?q=complete_static_lib+file:build/config&sq=package:chromium&g=0&l=1679 too)
We'll probably need something like that if/when we start creating installable packages. This will do for now because we can just assume that the object files will be present on disk.


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/ubsan/BUILD.gn:1
+source_set("sources") {
+  sources = [
----------------
phosek wrote:
> How closely do we want to follow CMake target names? For example, in CMake build this target is called `RTUbsan` (`add_compiler_rt_object_libraries` also adds `.<arch>` but that's an implementation detail).
I don't think it's worth copying the target names here. CMake is subject to other constraints (e.g. the target name has to be unique throughout the build) and the name I've chosen makes it a little clearer that this is a source set and therefore behaves as a source set when linked into other targets.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56628