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

[Jordan Rupprecht via Phabricator via llvm-commits]

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

> What is the behaviour of --rename-section when it touches a section with existing flags that don't conflict with the specified value? For example alloc when the section has SHF_EXECINSTR already? Does it preserve those flags?

It looks like SHF_EXECINSTR is not preserved in that case, e.g.

  $ cat /tmp/a.c 
  char foo[] = "hello";
  $ readelf -S /tmp/a.o | grep -A 1 .text
    [ 2] .text             PROGBITS         0000000000000000  00000040
         0000000000000000  0000000000000000  AX       0     0     4
  $ objcopy --rename-section=.text=.foo,alloc /tmp/a.o /tmp/a-gnu.o; readelf -S /tmp/a-gnu.o | grep -A 1 .foo
    [ 1] .foo              NOBITS           0000000000000000  00000040
         0000000000000000  0000000000000000  WA       0     0     4

> Can multiple modes be specified for the same section (e.g. code + alloc)? I don't think the tests really show these.

Yes, it's comma-delimited. I added a couple tests to show that.



================
Comment at: test/tools/llvm-objcopy/rename-section-flag.test:40
+
+# ALLOC: Name: .bar
+# ALLOC-NEXT: Type: SHT_PROGBITS
----------------
jhenderson wrote:
> You could probably make this test a little more explicit, and reduce some of the duplication by using --check-prefixes:
> 
> ```--rename-section=.foo=.bar,alloc | FileCheck %s --check-prefixes=CHECK,ALLOC,WRITE
> ... --rename-section=.foo=.bar,load | FileCheck %s --check-prefixes=CHECK,WRITE
> 
> # CHECK: Name: .bar
> # CHECK-NEXT: Type: SHT_PROGBITS
> # CHECK-NEXT: Flags [
> # ALLOC-NEXT: SHF_ALLOC (0x2)
> # WRITE-NEXT: SHF_WRITE (0x1)
> # CHECK-NEXT: ]
> ```
Thanks, the test is much more readable now. I wasn't aware of multiple check-prefixes.


================
Comment at: test/tools/llvm-objcopy/rename-section-flag.test:49
+# LOAD-NEXT: Flags [ (0x1)
+# LOAD-NEXT: SHF_WRITE (0x1)
+# LOAD-NEXT: ]
----------------
jhenderson wrote:
> I'm a bit surprised by this one. I haven't checked, but I'd expect load to cause SHF_ALLOC.
That's what I thought as well. However,
```
$ readelf -S /tmp/a.o | grep -A 1 .text
  [ 2] .text             PROGBITS         0000000000000000  00000040
       0000000000000000  0000000000000000  AX       0     0     4
$ objcopy --rename-section=.text=.foo,load /tmp/a.o /tmp/a-gnu.o; readelf -S /tmp/a-gnu.o | grep -A 1 .foo
  [ 1] .foo              PROGBITS         0000000000000000  00000040
       0000000000000000  0000000000000000   W       0     0     4

```


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:196
 
+enum SectionFlag {
+  SecNone = 0,
----------------
alexshap wrote:
> rupprecht wrote:
> > alexshap wrote:
> > > I think in any case it should not be in the global namespace,
> > > but more importantly, probably it would be better to use ELF* constants directly rather than introducing this enum.
> > > I think in any case it should not be in the global namespace,
> > Moved to the anonymous namespace above (w/ config classes)
> > 
> > > but more importantly, probably it would be better to use ELF* constants directly rather than introducing this enum.
> > 
> > This isn't meant to replace the ELF SHF_* constants, it's meant to map to allowed command-line flags.
> > For instance, "readonly" maps to "unset SHF_WRITE"
> yea, it's not a replacement, but why do we need this double indirection - 
> i mean now there is a switch (lines 215 - 228) and there is a chain of "if" statements (lines 255 - 265) -
> it looks like the latter would be kinda redundant if ParseSectionName returned the flag (instead of the corresponding enum value) - am i missing smth ?
So, I had it written w/o this, and it was pretty ugly. I added a separate type here, and I think it's better for a couple reasons:
 - It logically separates command line flags (which are really bfd names) against ELF section types.
 - It's not a simple map to ELF flag values. Most of them, sure -- for "alloc", just return SHF_ALLOC and have the caller |= the flag value. But there is no "readonly" flag; instead, there's SHF_WRITE, which is set _unless_ "readonly" is set. You could special case that one -- it's the only outlier here -- but I think it's easier to follow the code when written this way.
 - I've just started incrementally working on this, and I'm not convinced that what I have is complete w.r.t. gnu objcopy compatibility (or at least as far as we care about matching it). These might do things besides just SHF_* flags, so having separate types makes that more transparent.


Repository:
  rL LLVM

https://reviews.llvm.org/D49870