[PATCH] D153262: [llvm-objcopy] --set-section-flags: allow "large" to add SHF_X86_64_LARGE

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:141
+ `debug`, `code`, `data`, `rom`, `share`, `contents`, `merge`, `strings`, and
+ `large`. Not all flags are meaningful for all object file formats.
 
----------------
Perhaps worth extending this to say "all object file formats or target architectures"?


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:155
    section.
+ - `large` = add the `SHF_X86_64_LARGE` on x86_64, it is an error if the target
+   architecture is not x86_64.
----------------
aeubanks wrote:
> 
I found "this" somewhat awkward in the comment (the semi-colon is still needed though). How about "rejected if target architecture is not x86_64"?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags-large-multiarch.test:10
+# RUN: llvm-objcopy -O elf64-tradbigmips --set-section-flags=.foo=alloc --set-section-flags=.bar=alloc %t %t.mips
+# RUN: llvm-readobj --sections %t.mips | FileCheck %s --check-prefixes=CHECK,ALLOC,WRITE,REINTERPRET-GPREL
+
----------------
As you don't have many different variant test cases which have different expected outputs, you don't need so many check-prefixes. I suggest reducing them down to just the REINTERPRET* prefixes and one other (e.g. CHECK).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags-large-multiarch.test:21
+
+# BAD-LARGE: section flag SHF_X86_64_LARGE can only be used with x86_64 architecture
+
----------------
As this is only used in one case, I recommend moving it to immediately after that case's RUN line. Also, I suggest including the "error: " prefix, as it helps ensure we've captured the whole message.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags-large-multiarch.test:30-32
+  - Name:   .foo
+    Type:   SHT_PROGBITS
+    Flags:  [ ]
----------------
Is it worth having a version of this section (i.e. one without SHF_X86_64_LARGE) that isn't operated on by --set-section-flags, just to complete the test matrix? (Possibly not)


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

https://reviews.llvm.org/D153262