[llvm-objcopy] Consider section flags when adding section

[alfonsosanchezbeato]

Summary

The --set-section-flags option was being ignored when adding a new
section. Take it into account if present.

Fixes https://llvm.org/PR51244

[jhenderson]

Could you please add a test case to the existing llvm-objcopy tests (llvm/test/tools/llvm-objcopy) that shows this fix works.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

232

Rather than modify flagsToCharacteristics, could you just move this loop above the SetSectionFlags block above?

[alfonsosanchezbeato]

Test added.

[alfonsosanchezbeato]

Test added

[alfonsosanchezbeato]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

232

You need to change the characteristics before adding the section so addSection() takes them into account and can set VirtualSize, VirtualAddress, etc. Otherwise the flags would be set, but those parts of the section header would not get the right values.

[jhenderson]

@mstorsjo, could you take a look, please? My COFF knowledge isn't really sufficient to see if there's a better way of doing things. My initial thought is whether a separate later pass should be the one that sets virtual addresses, rather than addSection, but it may not make sense.

[mstorsjo]

In D106942#2921637, @jhenderson wrote:

@mstorsjo, could you take a look, please? My COFF knowledge isn't really sufficient to see if there's a better way of doing things. My initial thought is whether a separate later pass should be the one that sets virtual addresses, rather than addSection, but it may not make sense.

I think approach is pragmatically sensible here.

We don't recalculate and set virtual addresses for all sections, only added ones. (We generally can't change the virtual memory layout of the linked image as there are lots of relative addresses encoded anywhere in the section contents without any relocation marker, as those relocations were fixed at link time. So we can remove sections from the end and add new ones at the end, but we generally can't remove ones in the middle leaving a gap.) So if we split out setting addresses to a separate pass, it would only run over the sections that were added, not all of them. So keeping it in addSection and just checking the flags to set like this patch does, probably is fine.

llvm/test/tools/llvm-objcopy/COFF/set-flags-on-setion-added.test

14 ↗

(On Diff #363300)

I don't see any check here that actually verifies that the newly added sections get the right flags here?

[mstorsjo]

Oh, also, @alfonsosanchezbeato, please include extra context in the diff when you upload (by doing e.g. git diff -U999) to make it easier to review here.

[alfonsosanchezbeato]

Test updated as requested by review.

[mstorsjo]

The test looks good to me now, and I've given my opinion on the implementation form, so +1 on this from me now, but I'll let @jhenderson approve if he's ok with it.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/set-flags-on-setion-added.test

7 ↗

(On Diff #365182)

There's only one test case in this file, so there's no need for the --check-prefixes argument. Just use the default CHECK for your check prefix throughout the file.

9–24 ↗

(On Diff #365182)

Let's clean up the spacing so that the values all line up nicely.

13 ↗

(On Diff #365182)

Is there a reason you don't check the full set of Characteristics here (i.e. including the title etc)? Something like the following:

# CHECK:       Characteristics [
# CHECK-NEXT:   IMAGE_SCN_CNT_CODE (0x20)
# CHECK-NEXT:   IMAGE_SCN_MEM_EXECUTE (0x20000000)
# CHECK-NEXT:   IMAGE_SCN_MEM_READ (0x40000000)
# CHECK-NEXT:   IMAGE_SCN_MEM_WRITE (0x80000000)
# CHECK-NEXT: ]

This will ensure no spurious flags have been set too.

Same applies for the second section.

[alfonsosanchezbeato]

Thanks for the comments. I have improved the test as suggested.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/set-flags-on-setion-added.test

13 ↗

(On Diff #366840)

I'd omit the (0x...) bit from this line and the similar line below. This will a) make maintaining the test easier in the future (if the flag set changes), and more importantly b) will ensure this matches the Characteristics block for this section as opposed to some future section.

[alfonsosanchezbeato]

Ok, I've removed the hex bits from the comparison.

[jhenderson]

In D106942#2948903, @alfonsosanchezbeato wrote:

Ok, I've removed the hex bits from the comparison.

FWIW, I was referring specifically to the Characteristics line, but I don't think it matters whether they are there or not for the other lines. LGTM.

[MaskRay]

Consider renaming the test to match ELF/add-section-and-set-flags.test

[mstorsjo]

@ alfonsosanchezbeato - This is approved but not pushed yet - I guess you don't have push access? If you provide your preferred git author line, "Real Name <email@address>", I can push it for you. (Also note @MaskRay 's request for renaming the test, but I guess I can do that before pushing too.)

[alfonsosanchezbeato]

In D106942#2962195, @mstorsjo wrote:

@ alfonsosanchezbeato - This is approved but not pushed yet - I guess you don't have push access? If you provide your preferred git author line, "Real Name <email@address>", I can push it for you. (Also note @MaskRay 's request for renaming the test, but I guess I can do that before pushing too.)

I've renamed the test now. Right, I do not have push access, so I would appreciate if you can do that for me. The author line would be:

Author: Alfonso Sánchez-Beato <[e-mail]>

Thanks,
Alfonso

llvm/test/tools/llvm-objcopy/COFF/add-section-and-set-flags.test

+# RUN: yaml2obj %s -o %t
+
+## Test that llvm-objcopy can add sections and set its flags in the same
+## call.
+# RUN: echo DEADBEEF > %t.sec
+# RUN: llvm-objcopy --set-section-flags=.test.section1=code --add-section=.test.section1=%t.sec --set-section-flags=.test.section2=data --add-section=.test.section2=%t.sec %t %t1
+# RUN: llvm-readobj --sections --section-data %t1 | FileCheck %s
+
+# CHECK:       Name: .test.section1
+# CHECK-NEXT:  VirtualSize: 0x9
+# CHECK-NEXT:  VirtualAddress: 0x0
+# CHECK-NEXT:  RawDataSize: 9
+# CHECK:       Characteristics [
+# CHECK-NEXT:      IMAGE_SCN_CNT_CODE
+# CHECK-NEXT:      IMAGE_SCN_MEM_EXECUTE
+# CHECK-NEXT:      IMAGE_SCN_MEM_READ
+# CHECK-NEXT:      IMAGE_SCN_MEM_WRITE
+# CHECK-NEXT:  ]
+
+# CHECK:       Name: .test.section2
+# CHECK-NEXT:  VirtualSize: 0x9
+# CHECK-NEXT:  VirtualAddress: 0x9
+# CHECK-NEXT:  RawDataSize: 9
+# CHECK:       Characteristics [
+# CHECK-NEXT:    IMAGE_SCN_CNT_INITIALIZED_DATA
+# CHECK-NEXT:    IMAGE_SCN_MEM_READ
+# CHECK-NEXT:    IMAGE_SCN_MEM_WRITE
+# CHECK-NEXT:  ]
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ ]
+sections:
+symbols:
+...

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

static Error addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
 
   addSection(Obj, ".gnu_debuglink", *Contents,
              IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
                  IMAGE_SCN_MEM_DISCARDABLE);
 
   return Error::success();
 }
 
-static void setSectionFlags(Section &Sec, SectionFlag AllFlags) {
+static uint32_t flagsToCharacteristics(SectionFlag AllFlags, uint32_t OldChar) {
   // Need to preserve alignment flags.
   const uint32_t PreserveMask =
       IMAGE_SCN_ALIGN_1BYTES | IMAGE_SCN_ALIGN_2BYTES | IMAGE_SCN_ALIGN_4BYTES |
       IMAGE_SCN_ALIGN_8BYTES | IMAGE_SCN_ALIGN_16BYTES |
       IMAGE_SCN_ALIGN_32BYTES | IMAGE_SCN_ALIGN_64BYTES |
       IMAGE_SCN_ALIGN_128BYTES | IMAGE_SCN_ALIGN_256BYTES |
       IMAGE_SCN_ALIGN_512BYTES | IMAGE_SCN_ALIGN_1024BYTES |
       IMAGE_SCN_ALIGN_2048BYTES | IMAGE_SCN_ALIGN_4096BYTES |
       IMAGE_SCN_ALIGN_8192BYTES;
 
   // Setup new section characteristics based on the flags provided in command
   // line.
-  uint32_t NewCharacteristics =
-      (Sec.Header.Characteristics & PreserveMask) | IMAGE_SCN_MEM_READ;
+  uint32_t NewCharacteristics = (OldChar & PreserveMask) | IMAGE_SCN_MEM_READ;
 
   if ((AllFlags & SectionFlag::SecAlloc) && !(AllFlags & SectionFlag::SecLoad))
     NewCharacteristics |= IMAGE_SCN_CNT_UNINITIALIZED_DATA;
   if (AllFlags & SectionFlag::SecNoload)
     NewCharacteristics |= IMAGE_SCN_LNK_REMOVE;
   if (!(AllFlags & SectionFlag::SecReadonly))
     NewCharacteristics |= IMAGE_SCN_MEM_WRITE;
   if (AllFlags & SectionFlag::SecDebug)
     NewCharacteristics |=
         IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_DISCARDABLE;
   if (AllFlags & SectionFlag::SecCode)
     NewCharacteristics |= IMAGE_SCN_CNT_CODE | IMAGE_SCN_MEM_EXECUTE;
   if (AllFlags & SectionFlag::SecData)
     NewCharacteristics |= IMAGE_SCN_CNT_INITIALIZED_DATA;
   if (AllFlags & SectionFlag::SecShare)
     NewCharacteristics |= IMAGE_SCN_MEM_SHARED;
   if (AllFlags & SectionFlag::SecExclude)
     NewCharacteristics |= IMAGE_SCN_LNK_REMOVE;
 
-  Sec.Header.Characteristics = NewCharacteristics;
+  return NewCharacteristics;
 }
 
 static Error handleArgs(const CommonConfig &Config, Object &Obj) {
   // Perform the actual section removals.
   Obj.removeSections([&Config](const Section &Sec) {
     // Contrary to --only-keep-debug, --only-section fully removes sections that
     // aren't mentioned.
     if (!Config.OnlySection.empty() && !Config.OnlySection.matches(Sec.Name))
static Error handleArgs(const CommonConfig &Config, Object &Obj) {
   // Actually do removals of symbols.
   if (Error Err = Obj.removeSymbols(ToRemove))
     return Err;
 
   if (!Config.SetSectionFlags.empty())
     for (Section &Sec : Obj.getMutableSections()) {
       const auto It = Config.SetSectionFlags.find(Sec.Name);
       if (It != Config.SetSectionFlags.end())
-        setSectionFlags(Sec, It->second.NewFlags);
+        Sec.Header.Characteristics = flagsToCharacteristics(
+            It->second.NewFlags, Sec.Header.Characteristics);
     }
 
   for (const auto &Flag : Config.AddSection) {
     StringRef SecName, FileName;
     std::tie(SecName, FileName) = Flag.split("=");
 
     auto BufOrErr = MemoryBuffer::getFile(FileName);
     if (!BufOrErr)
       return createFileError(FileName, errorCodeToError(BufOrErr.getError()));
     auto Buf = std::move(*BufOrErr);
 
+    uint32_t Characteristics;
+    const auto It = Config.SetSectionFlags.find(SecName);
+    if (It != Config.SetSectionFlags.end())
+      Characteristics = flagsToCharacteristics(It->second.NewFlags, 0);
+    else
+      Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_ALIGN_1BYTES;
+
     addSection(
         Obj, SecName,
         makeArrayRef(reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
                      Buf->getBufferSize()),
-        IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_ALIGN_1BYTES);
+        Characteristics);
   }
 
   if (!Config.AddGnuDebugLink.empty())
     if (Error E = addGnuDebugLink(Obj, Config.AddGnuDebugLink))
       return E;
 
   return Error::success();
 }