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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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? Can multiple modes be specified for the same section (e.g. code + alloc)? I don't think the tests really show these.



================
Comment at: test/tools/llvm-objcopy/rename-section-flag.test:40
+
+# ALLOC: Name: .bar
+# ALLOC-NEXT: Type: SHT_PROGBITS
----------------
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: ]
```


================
Comment at: test/tools/llvm-objcopy/rename-section-flag.test:49
+# LOAD-NEXT: Flags [ (0x1)
+# LOAD-NEXT: SHF_WRITE (0x1)
+# LOAD-NEXT: ]
----------------
I'm a bit surprised by this one. I haven't checked, but I'd expect load to cause SHF_ALLOC.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:214
 
+static SectionFlag ParseSectionName(StringRef SectionName) {
+  return llvm::StringSwitch<SectionFlag>(SectionName)
----------------
This function name is a bit misleading to me, since it doesn't do anything with the section name at all. Please rename it to something like ParseSectionRenameFlags or something equally descriptive.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:231
+
+static SectionRename ParseRenameSectionFlag(StringRef FlagValue) {
+  if (!FlagValue.contains('='))
----------------
I would say this should be called "ParseRenameSectionValue", not Flag, to avoid confusion with the flags portion of the value passed to the option.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:265
+      ElfFlags |= ELF::SHF_STRINGS;
+    SectionConfig.NewFlags = ElfFlags;
+  }
----------------
Why not assign to this directly in the above set of ifs, rather than go via another variable?


Repository:
  rL LLVM

https://reviews.llvm.org/D49870