[PATCH] D69188: [llvm-objcopy] Preserve .ARM.attributes section when stripping files

Fri Oct 18 14:23:46 PDT 2019

rupprecht added a comment.

In D69188#1715050 <https://reviews.llvm.org/D69188#1715050>, @thieta wrote:

> In D69188#1715040 <https://reviews.llvm.org/D69188#1715040>, @abrachet wrote:
>
> > Seems like a hack, no? What about using `--keep-section` when you need it? You say its not strictly needed on modern systems and the problem is with the `dlopen` implementation of these older libcs, and not `llvm-objcopy`, so I would be hesitant to add something like this especially given we have a solution with `--keep-section` <https://llvm.org/docs/CommandGuide/llvm-objcopy.html#cmdoption-llvm-objcopy-keep-section>
>
>
> Keep-sections totally works. But since this produces unusable binaries on systems that require this section and lld already makes this consession I think it's worth adding this very low maintenance hack.

I agree, we generally want `--strip-all` to be the aggressive option and `--strip-all-gnu` to be the option when more compatibility is needed (and `--keep-section` when things seriously go wrong), but I think we should probably not have `--strip-all` be completely unusable for default modes.

OTOH, Alex has a point, we don't want to make binaries larger just so buggy systems are compatible. How large are arm attribute sections anyway? Is there any significant binary bloat to worry about? Maybe this just warrants a more explicit comment in the source that we should revisit this in a few years -- and if anyone is still using an old system, they can stay pinned to an old llvm-strip.

> The most common usecase here is to replace GNU strip with llvm-strip, to pass keep-section in this case you would need a wrapper or patch build systems (painful with automake for example).
> 
> Reading the launchpad report linked from Peter's post it seems like this might actually still be a problem which would mean that even a new Ubuntu install on arm wouldn't be able to run these binaries.

For more context, I dug up the patch where we added the `.gnu.warning` exemption above, and tracked it down to where `--strip-all-gnu` was created in rL319071 <https://reviews.llvm.org/rL319071>. The patch description there says that the more aggressive `--strip-all` was inspired by `eu-strip`. Have you tried out that tool to see if it has the same issue? It would be interesting to see if they exempt that & if they've run into this issue before.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-preserve-arm-attributes.test:2
+# RUN: yaml2obj %s > %t
+# RUN: cp %t %t3
+# RUN: llvm-objcopy --strip-all %t %t2
----------------
Copying the input file is only required for tests that plan to check that the input file wasn't accidentally modified (i.e. there would be a check for `cmp %t %t3` later). I think that's not necessary for this test, and you can remove this line.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-preserve-arm-attributes.test:3
+# RUN: cp %t %t3
+# RUN: llvm-objcopy --strip-all %t %t2
+# RUN: llvm-readobj --file-headers --sections %t2 | FileCheck %s
----------------
I think `--strip-all-gnu` should work too. Can you add another check that it also works?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69188