[PATCH] D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 4 inline comments as done.
rupprecht added a comment.

In https://reviews.llvm.org/D49870#1181835, @jhenderson wrote:

> The code looks correct to me (barring a couple of nits) for what you are trying to do, but I do have a couple of outstanding concerns, that I'd like to discuss further:
>
> I'm really worried about clearing the TLS flag, and to a lesser extent, the INFO_LINK flag. Clearing the TLS flag would almost certainly break the file, and I have no idea what the repercussions of clearing INFO_LINK might be. I'd be tempted to break with GNU compatibility here and not clear either of them. If we really don't want to do that, I'd recommend including a "tls" input option. I'm not sure what to do with INFO_LINK, because it affects other fields in the section header. Are you sure that the INFO_LINK and TLS flags are being cleared by the renaming and not by something else (e.g. implicit meaning of section names)? What happens if you rename e.g. .tdata.foo to .tdata.bar (thus maintaining the standard TLS naming scheme)?


I'm still not able to get `SHF_TLS` propagated, even with a real object file (not just something artificially created w/ yaml2obj):

  $ cat /tmp/t.c
  static __thread char tls = 5;
  char touch() { return ++tls; }
  $ clang -c /tmp/t.c -o /tmp/t.o && bin/llvm-readobj -sections /tmp/t.o
    ...
    Section {
      Index: 4
      Name: .tdata (87)
      Type: SHT_PROGBITS (0x1)
      Flags [ (0x403)
        SHF_ALLOC (0x2)
        SHF_TLS (0x400)
        SHF_WRITE (0x1)
      ]
    ...
  $ objcopy --rename-section .tdata=.tdata1,alloc /tmp/t.o /tmp/t2.o && bin/llvm-readobj -sections /tmp/t2.o
    ...
    Section {
      Index: 3
      Name: .tdata1 (38)
      Type: SHT_NOBITS (0x8)
      Flags [ (0x3)
        SHF_ALLOC (0x2)
        SHF_WRITE (0x1)
      ]
    ...

Although note that PROGBITS also changed to NOBITS. Maybe we should just not support renaming sections that having SHF_TLS, SHF_INFO_LINK, or other special flags? This could be reaching into unsupported objcopy territory which gnu objcopy is silently allowing.

I don't really have an immediate preference, so I just added those two to the preserve mask -- let me know if I should add others.

> I've been thinking about this a bit more, and I honestly feel like it doesn't make sense to publish the somewhat arcane options that GNU objcopy supports, except for legacy support. As such, whilst I believe we should support things like "contents", "debug" etc, in order to maintain compatibility, I also feel like they shouldn't be in the help text, to try to "persuade" people to adopt a more sensible and meaningful usage going forwards. Going on from this, how would you feel about extending the syntax to make more sense for ELF, i.e. to have a flag name for each modifiable individual ELF flag (i.e. alloc, write, execinstr, tls, ...)? I don't think this needs to happen in this change, but I do feel like we shouldn't be satisfied with just GNU compatibility.

Yep, I was thinking something like `objcopy --rename-section .foo=.bar,shf_alloc,shf_execinstr,noshf_write...` might be a good way to build that into this.

BTW, the use case I'm currently trying to fix is `objcopy -I binary -Bi386:x86-64 -Oelf64-x86-64 --rename-section .data=.rodata,alloc,load,readonly,data,contents`, mentioned here: http://lists.llvm.org/pipermail/llvm-dev/2017-June/113655.html. It would make my life (and I think many others' lives) a lot easier if we could at least support those options for drop-in compatibility, but agree that we don't need to go full-on bug for bug compatibility.



================
Comment at: test/tools/llvm-objcopy/rename-section-flag-preserved.test:1
+# RUN: yaml2obj %s > %t
+
----------------
jhenderson wrote:
> Since this uses an OS-specific flag, will this work without x86_64 target support? Or do we need a REQUIRES?
No, it appears this works cross platform; I've separated all flags into a separate test case for clarity. (And it works fine even though the machine I'm testing on is not mips/arm/hex).


Repository:
  rL LLVM

https://reviews.llvm.org/D49870