Make GN Android build link executables

build/config/compiler/BUILD.gn

Index: build/config/compiler/BUILD.gn
diff --git a/build/config/compiler/BUILD.gn b/build/config/compiler/BUILD.gn
index c492356ce6bfafb24a056e2f7a89e1e0a7fdff48..253f6eb0c4e48196b1740e7f7b1c47d5de36d792 100644
--- a/build/config/compiler/BUILD.gn
+++ b/build/config/compiler/BUILD.gn
@@ -7,6 +7,12 @@ if (cpu_arch == "arm") {
   import("//build/config/arm.gni")
 }
 
+declare_args() {
+  # Normally, Android builds are lightly optimized, even for debug builds, to
+  # keep binary size down. Setting this flag to true disables such optimization
+  android_full_debug = false
+}
+
 # compiler ---------------------------------------------------------------------
 #
 # Base compiler configuration.
@@ -59,6 +65,12 @@ config("compiler") {
     } else if (is_linux) {
       cflags += [ "-fstack-protector", "--param=ssp-buffer-size=4" ]
     }
+
+    # Linker warnings.
+    if (!is_chromeos || cpu_arch != "arm") {

[awong, 2014/05/12 19:23:02]

de morgan's?

!(is_chromeos && cpu_arch == "arm")

Feel easier to read.

+      # TODO(jochen): Enable this on chromeos on arm. http://crbug.com/356580
+      ldflags += [ "-Wl,--fatal-warnings" ]
+    }
   }
 
   # Mac-specific compiler flags setup.
@@ -131,16 +143,26 @@ config("compiler") {
     }
   }
 
-  # Linux-specific compiler flags setup.
-  # ------------------------------------
-  if (is_linux) {
+  # Linux/Android common flags setup.

[scottmg, 2014/05/12 16:33:58]

or any of this

+  # ---------------------------------
+  if (is_linux || is_android) {
     cflags += [
       "-fPIC",

[awong, 2014/05/12 19:23:02]

Huh...do we actually build -fPIC for a static link?  That feels a little odd.
Also, it feels like we should be using fPIE for the final executable link...

http://lfs.osuosl.org/hlfs/view/unstable/glibc/chapter02/pie.html

I think pulling in one of thakis, thestig, and/or mmoss to verify might be good
here.

[brettw, 2014/05/12 21:00:08]

Yes, I verified that this matches the GYP build.

[cjhopman, 2014/05/12 21:04:19]

IDK about linux, but android doesn't build executables (except for a couple rare
tools used for running tests). It seems that we probably do want fpie for
executables, though.

It looks like the gyp build, adds -pie only to the final chrome executable.

[awong, 2014/05/12 21:34:23]

Gotcha. I was worried about -fPIE more for the is_linux variant. I mean, we
could do it for actual pure NDK targets too...but since we don't release any of
those it doesn't matter.

       "-pipe",  # Use pipes for communicating between sub-processes. Faster.
     ]
-    if (!is_android) {
-      cflags += [ "-pthread" ]
-    }
+
+    ldflags += [
+      "-fPIC",
+      "-Wl,-z,noexecstack",
+      "-Wl,-z,now",

[awong, 2014/05/12 19:23:02]

Can we track down why we force immediate binding of symbols on dlopen and add a
comment?

http://www.qnx.com/developers/docs/660/index.jsp?topic=%2Fcom.qnx.doc.neutrin...

My naive expectation is will mostly just cause us to be slower at startup. I
*think* the combination of -z relro and -z now serves to disable an attack on
the GOT

  https://isisblogs.poly.edu/2011/06/01/relro-relocation-read-only/

but I'm not security smart enough to be 100% certain.

Also, if that's true, can we rewrite this as
-Wl,-z,relro,-z,now

so it's more obvious that these two linker flags are doing one conceptual
operation?

[brettw, 2014/05/12 21:00:08]

I actually know very little about Linux dynamic loading (I'm a Windows person)
and I just copied the flags. I actually never looked up what they do. Perhaps
you want to start a thread with relevant people to discuss this kind of thing?

+      "-Wl,-z,relro",
+    ]
+  }
+
+  # Linux-specific compiler flags setup.
+  # ------------------------------------
+  if (is_linux) {
+    cflags += [ "-pthread" ]
 
     if (cpu_arch == "x64") {
       # Use gold for linking on 64-bit Linux only (on 32-bit it runs out of
@@ -170,11 +192,7 @@ config("compiler") {
     }
 
     ldflags += [
-      "-fPIC",
       "-pthread",
-      "-Wl,-z,noexecstack",
-      "-Wl,-z,now",
-      "-Wl,-z,relro",
     ]
   }
 
@@ -225,6 +243,22 @@ config("compiler") {
       defines += [ "HAVE_SYS_UIO_H" ]
     }
 
+    # Use gold for Android for most CPU architectures.
+    if (cpu_arch == "x86" || cpu_arch == "x64" ||
+        (cpu_arch == "arm" && !is_clang)) {
+      # These architectures support gold for linking. Apparently Clang has not
+      # been made to work on x86 yet or we would probably need a -B option like
+      # we do below for Clang-on-arm.

[awong, 2014/05/12 19:23:02]

Do you mean Clang has not been make to work on arm?

This comment overall is slightly confusing. I'm not quite sure if you mean gold
won't work on other architectures, or if you just need to do smarter things with
flags or what.

It's even odder that clang on x86 support -fuse-ld=gold, but on arm it
doesn't...

[brettw, 2014/05/12 21:00:08]

This matches the GYP file, but I changed it to match what I think the GYP file
was going for, which is that we don't pass -fuse-ld to clang. The x86 abdroid
build is new and doesn't support clang yet, so it probably hasn't come up.

+      ldflags += [ "-fuse-ld=gold" ]
+    } else if (cpu_arch == "arm") {
+      # Clang does not support -fuse-ld to invoke the built-in gold linker,
+      # so use the -B option which requires us to specify the path.

[awong, 2014/05/12 19:23:02]

Is this the long-term expected approach? Having flag divergence between gcc and
clang is annoying. Can we confirm with nico?

[brettw, 2014/05/12 21:00:08]

I did some googleing and it seems clang doesn't support this.

+      ldflags += [
+        "-B" + rebase_path("//build/android/arm-linux-androideabi-gold",
+                           root_build_dir)
+      ]
+    }
+
     ldflags += [
       "-Wl,--no-undefined",
       # Don't export symbols from statically linked libraries.
@@ -322,10 +356,10 @@ config("runtime_library") {
     ]
 
     ldflags += [
+      "-Wl,--warn-shared-textrel",
       "-nostdlib",
     ]
 
-
     # NOTE: The stlport header include paths below are specified in cflags
     # rather than include_dirs because they need to come after include_dirs.
     # Think of them like system headers, but don't use '-isystem' because the
@@ -335,8 +369,9 @@ config("runtime_library") {
     if (use_system_stlport) {
       cflags += [
         # For libstdc++/include, which is used by stlport.
-        "-I$android_src/bionic",
-        "-I$android_src/external/stlport/stlport",
+        "-I" + rebase_path("$android_src/bionic", root_build_dir),

[awong, 2014/05/12 19:23:02]

I'm unconvinced this makes sense and the comment about it doesn't explain why
we're doing this.

Do you know what is in the $anrdoi_src/bionic directory and why we are playing
with include orderings in such a manner? This sounds like a recipe for version
skew in the headers.

[brettw, 2014/05/12 21:00:08]

I believe this is only the case on the WebView build. I don't understand this at
all.

[cjhopman, 2014/05/12 21:04:19]

Bionic is the Android c library, stlport is the c++ library. 

The ndk does this same thing for the stlport path, but I didn't see it for the
bionic path, though I only took a quick look (also I don't think that there is
an option for a different c library, so bionic might be set differently than
stlport).

[awong, 2014/05/12 21:34:23]

Yep...so I've been messing a lot with the NDK c++ library build/configure link
in the past few months. In a normal NDK build, you ignore the toolchain c++
runtime and then add include paths for the NDK version.

I don't undestand why we're overriding bionic though. The sysroot should handle
this. I could see that the sysroot doesn't have headers for the libc which would
make this necessary...but the flag order mattering implies that there are
mutiple libcs and multiple c++ standard libraries that are being shadowed in
weird ways. This seems wrong.

Anyways, again out of scope for this CL.

@brettw: Can you add a TODO(ajwong): Reexamine how selection of NDK c++ runtimes
work in its entirely (include headers, build options, linking, crtbegin, etc)

+        "-I" + rebase_path("$android_src/external/stlport/stlport",
+                           root_build_dir),
       ]
       libs += [
         "stlport",
@@ -344,7 +379,9 @@ config("runtime_library") {
     } else {
       android_stlport_root = "$android_ndk_root/sources/cxx-stl/stlport"
 
-      cflags += [ "-I$android_stlport_root/stlport" ]
+      cflags += [
+        "-I" + rebase_path("$android_stlport_root/stlport", root_build_dir)
+      ]
       lib_dirs += [ "$android_stlport_root/libs/$android_app_abi" ]
 
       if (component_mode == "shared_library") {
@@ -353,6 +390,16 @@ config("runtime_library") {
         libs += [ "stlport_static" ]
       }
     }
+
+    libs += [
+      # Manually link the libgcc.a that the cross compiler uses. This is
+      # absolute because the linker will look inside the sysroot if it's not.
+      rebase_path(android_libgcc_file),
+      "c",

[awong, 2014/05/12 19:23:02]

This seems incorrect to me. I think it's being caused by the -nostdlib in line
360.

What is the overall goal of the whole is_android stanza?  The comment in #342
seems to indicate we're just trying to STLPort, which means we're attempting to
avoid linking in the toolchain's libstdc++

If so, then I think nodefaultlib is what you want. That way you will still get
c, dl, crtbegin/end, etc.

[brettw, 2014/05/12 21:00:08]

GYP build does nostdlib and adds these libraries. I don't have any background in
why it works this way.

+      "dl",
+      "m",
+    ]
+
   }
 }
 
@@ -587,23 +634,60 @@ config("wexit_time_destructors") {
 #   configs -= default_optimization_config
 #   configs += [ "//build/config/compiler/optimize_max" ]
 
+# Shared settings for both "optimize" and "optimize_max" configs.
+if (is_win) {
+  common_optimize_on_cflags = [
+    "/o2",
+    "/ob2",  # both explicit and auto inlining.
+    "/oy-",  # disable omitting frame pointers, must be after /o2.
+    "/os",   # favor size over speed.
+  ]
+  common_optimize_on_ldflags = []
+} else {
+  common_optimize_on_cflags = [
+    # Don't emit the GCC version ident directives, they just end up in the
+    # .comment section taking up binary size.
+    "-fno-ident",
+    # Put data and code in their own sections, so that unused symbols
+    # can be removed at link time with --gc-sections.
+    "-fdata-sections",
+    "-ffunction-sections",
+  ]
+
+  common_optimize_on_ldflags = []
+  if (is_mac) {
+    if (symbol_level == 2) {
+      # Mac dead code stripping requires symbols.
+      common_optimize_on_ldflags += [
+        "-Wl,-dead_strip",
+      ]
+    }
+  } else {
+    # Non-Mac Posix linker flags.
+    common_optimize_on_ldflags += [
+      # Specifically tell the linker to perform optimizations.
+      # See http://lwn.net/Articles/192624/ .
+      "-Wl,-O1",
+      "-Wl,--as-needed",
+      "-Wl,--gc-sections",
+    ]
+  }
+
+  if (is_ios) {
+    common_optimize_on_cflags += [ "-os" ]

[scottmg, 2014/05/12 16:33:58]

are these really lower case 'O's on iOS?

[brettw, 2014/05/12 18:06:41]

Nope, I think an errant vi command lower-cased everything.

[awong, 2014/05/12 19:23:02]

Strange...does Android not compile -Os?

+  } else {
+    common_optimize_on_cflags += [ "-o2" ]
+  }
+}
+
 # Default "optimization on" config. On Windows, this favors size over speed.
-#
-# IF YOU CHANGE THIS also consider whether optimize_max should be updated.
 config("optimize") {
+  cflags = common_optimize_on_cflags
+  ldflags = common_optimize_on_ldflags
   if (is_win) {
-    cflags = [
-      "/O2",
-      "/Ob2",  # Both explicit and auto inlining.
-      "/Oy-",  # Disable omitting frame pointers, must be after /O2.
-      "/Os",   # Favor size over speed.
+    cflags += [
+      "/os",   # favor size over speed.

[scottmg, 2014/05/12 16:33:58]

/Os

     ]
-  } else {
-    if (is_ios) {
-      cflags = [ "-Os" ]
-    } else {
-      cflags = [ "-O2" ]
-    }
   }
 }
 
@@ -615,6 +699,20 @@ config("no_optimize") {
       "/Ob0",  # Disable all inlining (on by default).
       "/RTC1",  # Runtime checks for stack frame and uninitialized variables.
     ]
+  } else if (is_android && !android_full_debug) {
+    # On Android we kind of optimize some things that don't affect debugging
+    # much even when optimization is disabled to get the binary size down.
+    cflags = [
+      "-O1",
+      "-fomit-frame-pointer",
+      "-fdata-sections",
+      "-ffunction-sections",
+    ]
+    ldflags = [
+      "-Wl,-O1",

[awong, 2014/05/12 19:23:02]

This blob of linker flags is the same here as in 670. Extract to a
_common_linker_optimizations that you can append to the ldflags in both parts?

[brettw, 2014/05/12 21:00:08]

I didn't do that because I didn't feel like this list should necessarily be the
same. As with the cflags, how we want to optimize the debug build even though
optimization would normally be off, and how we want to optimize the release
build, seem like independent concepts that don't need to stay in sync.

[awong, 2014/05/12 21:34:23]

I think the fact that these 4 flags are used in concert to "optimize" linking is
actually a logical unit that would never get separated. The likilhood of someone
adding new flags to one of these and missing the other seems much higher than
the cost of artificially tying them.

[brettw, 2014/05/13 17:39:21]

Will do this in a followup. I have green tries and this is blocking some
people...

+      "-Wl,--as-needed",
+      "-Wl,--gc-sections",
+    ]
   } else {
     cflags = [ "-O0" ]
   }
@@ -624,31 +722,18 @@ config("no_optimize") {
 # optimization and link-time code generation which is very expensive and should
 # be used sparingly. For non-Windows, this is the same as "optimize".
 config("optimize_max") {
+  cflags = common_optimize_on_cflags
+  ldflags = common_optimize_on_ldflags
   if (is_win) {
-    cflags = [
-      "/O2",
-      "/Ob2",  # Both explicit and auto inlining.
-      "/Oy-",  # Disable omitting frame pointers, must be after /O2.
+    cflags += [
       "/Ot",   # Favor speed over size.
       "/GL",   # Whole program optimization.
     ]
-  } else {
-    if (is_ios) {
-      cflags = [ "-Os" ]
-    } else {
-      cflags = [ "-O2" ]
-    }
   }
 }
 
 # Symbols ----------------------------------------------------------------------
 
-# TODO(brettw) Since this sets ldflags on Windows which is inherited across
-# static library boundaries, if you want to remove the default symbol config
-# and set a different one on a target, you also have to do it for all static
-# libraries that go into that target, which is messed up. Either we need a
-# more flexible system for defining linker flags, or we need to separate this
-# out into a "symbols_linker" config that is only applied to DLLs and EXEs.
 config("symbols") {
   if (is_win) {
     cflags = [ "/Zi" ]  # Produce PDB file, no edit and continue.