lld + objcopy/strip breaks relro

[ngg]

NDK version: r18b
Architecture: tested on aarch64 (probably others as well)
Host OS: tested on Linux (probably others as well)

This bug is most probably caused by https://sourceware.org/bugzilla/show_bug.cgi?id=22829

To reproduce, compile an executable (or shared library) with lld and relro enabled then strip or objcopy it. This last step will disable relro by deleting the GNU_RELRO segment.

Possible solutions:

• update objcopy and strip to binutils >= 2.31
• cherry-pick the fix from https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f2731e0c374e5323ce4cdae2bcc7b7fe22da1a6f
• do not use objcopy and strip at all (provide llvm-objcopy and llvm-strip instead which do not have this problem)

Example file and command to reproduce:

// hello.c
#include <stdio.h>
int main(void)
{
    puts("Hello world!");
    return 0;
}

nggpc% $NDK/toolchains/llvm/prebuilt/linux-x86_64/bin/clang -target aarch64-none-linux-android --sysroot $NDK/platforms/android-21/arch-arm64 -L$NDK/sources/cxx-stl/llvm-libc++/libs/arm64-v8a -isystem $NDK/sysroot/usr/include -isystem $NDK/sysroot/usr/include/aarch64-linux-android -gcc-toolchain $NDK/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64 -fuse-ld=lld -o original hello.c                                                                                                                                    
nggpc% $NDK/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/aarch64-linux-android/bin/objcopy original copied
nggpc% cp original stripped
nggpc% $NDK/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/aarch64-linux-android/bin/strip stripped
nggpc% readelf -We original |grep RELRO
  GNU_RELRO      0x030000 0x0000000000030000 0x0000000000030000 0x000200 0x001000 R   0x1
nggpc% readelf -We copied |grep RELRO
[1]    15327 done       readelf -We copied | 
       15328 exit 1     grep --color=auto RELRO
nggpc% readelf -We stripped |grep RELRO
[1]    15406 done       readelf -We stripped | 
       15407 exit 1     grep --color=auto RELRO

[DanAlbert]

Thanks for the diagnosis!

@stephenhines should we be moving to llvm-objcopy/llvm-strip? Are they command line compatible? I don't think we can outright remove binutils strip in a timely manner since the gradle plugin expects it to exist at its current location and to have its current interface, but if we can install llvm-strip in its place and it Just Works then that's probably our best option.

[enh]

i know strip -o caused some trouble. @chih-hung has been switching AOSP over, so he'll know how much of this (if any) is already fixed...

[stephenhines]

Weird, I only just got the email notification of Dan's mention. I do think we should be asking users to switch. I don't believe these tools are fully command-line compatible, but we should be able to fix any egregious missing parts. Just like switching to Clang, this is going to involve some transition time, but it's probably better to start early.

[DanAlbert]

Sounds like it's reasonable for us to start moving. I'm not brave enough to clobber the binutils tools with the LLVM tools in r19 since beta 1 has shipped, but we'll note the upcoming transition in the changelog at the very least and I'll think about moving ndk-build/cmake in beta 2, but probably r20.

[chih-hung]

The issue I found was in https://b.corp.google.com/issues/80093681

…

On Mon, Nov 12, 2018 at 12:49 PM Elliott Hughes ***@***.***> wrote: i know strip -o caused some trouble. @chih-hung <https://github.com/chih-hung> has been switching AOSP over, so he'll know how much of this (if any) is already fixed... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#843 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQ5CvAfDgEcx_Wq_YSHn0E7Nq-GwQ1HPks5uud7WgaJpZM4YaMHS> .

[ngg]

Unfortunately llvm-objcopy is not fully command-line compatible with objcopy.
Chromium had this issue when they switched to lld last year, they switched to eu-strip instead of strip. https://chromium-review.googlesource.com/c/chromium/src/+/644908

[ngg]

It would be great to include llvm-objcopy and llvm-strip in r19 (and leave objcopy and strip there as well), and encourage users to use that if they choose to use lld (at least the ones who use custom build systems like us can start to transition)

[DanAlbert]

Unfortunately llvm-objcopy is not fully command-line compatible with objcopy.

Good to know. This isn't as much of a problem as it is for strip since it isn't used by gradle (it's used indirectly via ndk-build, but we can fix our use of it easily enough). Given that it's a less frequently used tool (at least in my experience), I'm hoping this won't be a big issue. Depending on the differences, we may be able to fix them.

[DanAlbert]

It would be great to include llvm-objcopy and llvm-strip in r19

Looks like this is already done, btw. It's in my build of master anyway and I'm pretty sure that's still the same version of the Clang tools as we have in r19 beta 1.

[DanAlbert]

But it also looks like llvm-strip does not do as well at actually producing small binaries. At least with --strip-unneeded (which is what ndk-build uses by default), binaries stripped by llvm-strip are larger than those stripped by binutils strip. For lld linked binaries, which have much larger unstripped intermediates, this is significant (an executable with an empty main is ~150KB instead of 12KB).

Definitely something that will need to be resolved before llvm-strip is viable.

[chih-hung]

On Tue, Nov 13, 2018 at 4:25 PM Dan Albert ***@***.***> wrote: But it also looks like llvm-strip does not do as well at actually producing small binaries. At least with --strip-unneeded (which is what ndk-build uses by default), binaries stripped by llvm-strip are larger than those stripped by binutils strip. For lld linked binaries, which have *much* larger unstripped intermediates, this is significant (an executable with an empty main is ~150KB instead of 12KB).

Dan, do you have some examples to show that lld produced much larger binaries? Maybe we used different flags, last time I checked lld outputs have similar size. I have counted some AOSP files at go/USE_CLANG_LLD.

…

-- chh Definitely something that will need to be resolved before llvm-strip is

viable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#843 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQ5CvKzD8zvWM4hFcyGtLE-nmbBpAs9Rks5uu2LwgaJpZM4YaMHS> .

[rprichard]

The platform passes this flag to lld for arm64, but it doesn't happen automatically with the NDK:

	arm64Lldflags = append(ClangFilterUnknownLldflags(arm64Ldflags),
		"-Wl,-z,max-page-size=4096")

[DanAlbert]

@chih-hung: I'd filed http://b/119522392, which I just duped into http://b/79162664 (apparently this was known and actually had been assigned to me for feedback and I missed the email).

The platform passes this flag to lld for arm64, but it doesn't happen automatically with the NDK

That's the problem then. Adding these "defaults" to the platcform build system does nothing to help the NDK. The NDK doesn't have control over all the build systems that use it, so this needs to be fixed either in the Clang driver or LLD itself.

I can add a note to our build system maintainers doc for now, but this will need to be fixed before it becomes the NDK's default.

[DanAlbert]

The lld fix mentioned above is in the compiler that we'll have for r21, so I need to re-investigate the other issues with strip.

[DanAlbert]

LLD and llvm-strip are looking good for r21. Uploaded https://android-review.googlesource.com/c/platform/ndk/+/1115434 to move us to llvm-strip.

... but Gradle will need to be fixed separately since it performs its own stripping. AGP version 3.6+ (presumably, I sent the patch but it isn't submitted yet) will be needed to fix this for most users.

[DanAlbert]

The NDK side of this is fixed, so moving out of the r21 bucket.

[DanAlbert]

I'm seeing incomplete stripping happening again. Stripping a binary with llvm-strip --strip-unneeded and then strip --strip-unneeded afterward yields the following with bloaty:

    FILE SIZE        VM SIZE
 --------------  --------------
  [NEW] +49.7Ki  [ = ]       0    .debug_loc
  [NEW] +18.7Ki  [ = ]       0    .debug_info
  [NEW] +11.7Ki  [ = ]       0    .debug_line
  [NEW] +7.15Ki  [ = ]       0    .debug_abbrev
  [NEW] +4.36Ki  [ = ]       0    .debug_str
  [NEW] +4.23Ki  [ = ]       0    .debug_ranges
  [NEW] +1.19Ki  [ = ]       0    .debug_aranges
   +32%    +104  [ = ]       0    .shstrtab
  [NEW]     +65  [ = ]       0    .debug_macinfo
  +0.2%      +4  [ = ]       0    [Unmapped]
   +31% +97.2Ki  [ = ]       0    TOTAL

About 100kb larger when llvm-strip is used (a 30% increase compared to GNU strip).

[DanAlbert]

However, it looks like --strip-all with llvm-strip yields the same binary as GNU strip. --strip-unneeded has the same --help description in both, so it seems the behavior is wrong.

[DanAlbert]

Also note that --strip-all has different behavior with llvm-strip than with GNU strip:

From llvm-strip:

--strip-all-gnu Compatible with GNU strip's --strip-all
--strip-all Remove non-allocated sections outside segments. .gnu.warning* sections are not removed

[DanAlbert]

Gradle side of this is submitted now. It'll be in 4.0.

[DanAlbert]

The ndk-build/CMake change for this got reverted due to #1083, and unfortunately I forgot to revert the revert before r21 was cut. Changing those tools out so late in the release cycle will effectively mean we need to restart the beta process, and I don't think we should be delaying r21 that much longer.

ndk-build and CMake will be fixed in r22, and llvm-strip is at least fixed to actually strip as much as binutils strip is now.

I should also mention that the NDK side of this change doesn't affect Gradle builds. Those will need AGP 4.0 regardless. The lack of this fix in r21 only affects ndk-build and CMake users that are not using Gradle.

[DanAlbert]

Fix for r22: https://android-review.googlesource.com/c/platform/ndk/+/1182837