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

[Thomas Köppe via Phabricator via llvm-commits]

tkoeppe marked an inline comment as done.
tkoeppe added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags-large-multiarch.test:30-32
+  - Name:   .foo
+    Type:   SHT_PROGBITS
+    Flags:  [ ]
----------------
jhenderson wrote:
> tkoeppe wrote:
> > jhenderson wrote:
> > > 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)
> > Yes, because there's a difference in behaviour: if you don't operate on the section, the flag is reinterpreserved when you change architectures, but if you do operate on it, then it might get dropped. I initially had a separate objcopy run for this, but maskray@ suggested just rolling that into a section that's not operated on.
> I think you misunderstood my comment. At the moment, you have the following three cases (one case per section):
> 
> section without flag, specified by --set-section-flags (.foo)
> section with flag, specified by --set-section-flags (.bar)
> section with flag, not specified by --set-section-flags (.untouched)
> 
> I'm suggesting you add the missing case:
> section without flag, not specified by --set-section-flags
> 
> Hopefully that case is trivial, but given you've got the rest of the test matrix, I think having coverage for that final combination would make sense.
Ah yes, of course, I did indeed misread your message. Sorry! Adding the test.


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

https://reviews.llvm.org/D153262