GN(android) Fix build all with is_asan

build/config/sanitizers/BUILD.gn

Index: build/config/sanitizers/BUILD.gn
diff --git a/build/config/sanitizers/BUILD.gn b/build/config/sanitizers/BUILD.gn
index 193f19a60e8592f061feff3ad2d368faed39b881..439e3a699f98601fe60ce743e6a3599387a5ba28 100644
--- a/build/config/sanitizers/BUILD.gn
+++ b/build/config/sanitizers/BUILD.gn
@@ -10,7 +10,14 @@ import("//build/config/sanitizers/sanitizers.gni")
 # |is_asan|, |is_lsan|, |is_tsan|, |is_msan| and |use_custom_libcxx| are false.
 group("deps") {
   if (using_sanitizer) {
-    public_configs = [ ":sanitizer_options_link_helper" ]
+    public_configs = [
+      ":sanitizer_options_link_helper",
+
+      # Even when a target removes default_sanitizer_flags, it may be depending
+      # on a library that did not remove default_sanitizer_flags. Thus, we need
+      # to add the ldflags here as well as in default_sanitizer_flags.
+      ":default_sanitizer_ldflags",

[brettw, 2016/01/19 20:39:42]

Doesn't this mean that if a target removes the default sanitizer flags, this
will add it back? That seems... undesirable. It seems like a benefit if you
remove the sanitizer config explicitly but it can't be removed, that you get an
error rather than having it silently be added back for you. Or maybe I
misunderstand some aspect of this.

[agrieve, 2016/02/02 19:22:31]

That's correct, but I think it's desirable and how it works already. 

If you list this as a dep, then you get the ldflags regardless of whether your
specific target removes the default_sanitizer_flags config (which mainly sets
ccflags)

If you don't want any sanitizer business in your app, then you should remove the
config *and* not depend on this target.

[brettw, 2016/02/02 22:31:17]

Okay, you convinced me.

+    ]
     deps = [
       ":options_sources",
     ]
@@ -26,24 +33,6 @@ group("deps") {
 
 config("sanitizer_options_link_helper") {
   ldflags = [ "-Wl,-u_sanitizer_options_link_helper" ]
-  if (is_asan) {
-    ldflags += [ "-fsanitize=address" ]
-  }
-  if (is_lsan) {
-    ldflags += [ "-fsanitize=leak" ]
-  }
-  if (is_tsan) {
-    ldflags += [ "-fsanitize=thread" ]
-  }
-  if (is_msan) {
-    ldflags += [ "-fsanitize=memory" ]
-  }
-  if (is_ubsan) {
-    ldflags += [ "-fsanitize=undefined" ]
-  }
-  if (is_ubsan_vptr) {
-    ldflags += [ "-fsanitize=vptr" ]
-  }
 }
 
 source_set("options_sources") {
@@ -74,6 +63,29 @@ source_set("options_sources") {
   }
 }
 
+config("default_sanitizer_ldflags") {
+  if (is_posix) {
+    if (is_asan) {
+      ldflags = [ "-fsanitize=address" ]
+    }
+    if (is_lsan) {
+      ldflags = [ "-fsanitize=leak" ]
+    }
+    if (is_tsan) {
+      ldflags = [ "-fsanitize=thread" ]
+    }
+    if (is_msan) {
+      ldflags = [ "-fsanitize=memory" ]
+    }
+    if (is_ubsan) {
+      ldflags = [ "-fsanitize=undefined" ]
+    }
+    if (is_ubsan_vptr) {
+      ldflags = [ "-fsanitize=vptr" ]
+    }
+  }
+}
+
 # This config is applied by default to all targets. It sets the compiler flags
 # for sanitizer usage, or, if no sanitizer is set, does nothing.
 #
@@ -84,6 +96,7 @@ config("default_sanitizer_flags") {
   cflags_cc = []
   ldflags = []
   defines = []
+  configs = [ ":default_sanitizer_ldflags" ]

[brettw, 2016/01/19 20:39:43]

This will cause these ldflags to get duplicated in the targets (since
sub-configs and regular configs aren't uniquified. I think we should avoid this.

[agrieve, 2016/02/02 19:22:31]

Didn't know that, and you obviously know about this way better than I do, but I
just did:
    ninja -n -v chrome_apk > build.log

And it looks like -fsanitize=address appears only once for each
gcc_solink_helper.py invokation.

[brettw, 2016/02/02 22:31:17]

Oh actually this is fine. I misunderstood this and thought this was duplicating
a config in BUILDCONFIG.

 
   # Only works on Posix-like platforms.
   # FIXME: this is not true, remove the conditional.