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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Nice! There are enough comments below that I won't hit "accept" just yet, but none of the comments are blocking and you can talk me out of all of them if you disagree with the suggestions.



================
Comment at: llvm/utils/gn/build/BUILD.gn:138
 
+config("crt_code") {
+  include_dirs = [ "//compiler-rt/lib" ]
----------------
"crt" means "c runtime library" in my head. I guess "compiler_rt_code" is too long for you?


================
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)",
----------------
Maybe

```
android_aarch64_toolchain = "//llvm/utils/gn/build/toolchain:stage2_android_aarch64"
deps += [
  ""//compiler-rt/lib/hwasan:hwasan_shared($android_aarch64_toolchain)"
  ...
```

?


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn:4
+runtime_output_dir = "$root_build_dir/lib/clang/$llvm_version/lib/linux"
+runtime_target = "aarch64-android"
+
----------------
assert(current_os == "android") or something like that up here (ldflags etc assume this)


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn:31
+
+source_set("sources") {
+  sources = [
----------------
This is the first source_set :-) I went with static_library almost everywhere because

- llvm-config expects the presence of many of the llvm libs
- the chromium build used source_sets pervasively at first but that was bad for binary size and link time, so many of that had to be undone

If you think source_set is the thing to do here it's fine to keep this as-is, just wanted to mention it.


================
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" ]
+}
----------------
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.


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn:79
+
+  complete_static_lib = true
+}
----------------
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)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56628