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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D153262#4518480 <https://reviews.llvm.org/D153262#4518480>, @tkoeppe wrote:

> I cannot figure out why clang-format rejects `llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp`: when I run the formatter script (`clang/tools/clang-format/git-clang-format llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp`) it doesn't make any changes.

Is it possible there's a difference in versions between the clang-format being run by the pre-merge checks and the version you're using? Anyway, see my inline edit. Hopefully that solves the pre-merge check issue.



================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:96-97
 
-static uint64_t getSectionFlagsPreserveMask(uint64_t OldFlags,
-                                            uint64_t NewFlags) {
+static uint64_t getSectionFlagsPreserveMask(
+    uint64_t OldFlags, uint64_t NewFlags, uint16_t EMachine) {
   // Preserve some flags which should not be dropped when setting flags.
----------------
When I run my local clang-format on this code, I get the inline edit.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags-large-multiarch.test:30-32
+  - Name:   .foo
+    Type:   SHT_PROGBITS
+    Flags:  [ ]
----------------
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.


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

https://reviews.llvm.org/D153262