[llvm-objcopy] Make --set-section-flags work with --add-section

[MaskRay]

Summary

This matches behavior GNU objcopy and can simplify clang-offload-bundler
(which currently works around the issue by invoking llvm-objcopy twice).

[MaskRay]

You may ask what objcopy --add-gnu-debuglink=a --set-section-flags=.gnu_debuglink=alloc does. It does not change the flags of .gnu_debuglink and thus our behavior matches GNU.
It seems like an a combo users won't use and I don't bother adding a test.

[rupprecht]

LGTM. I'll wait for a second person to stamp it.

In D90438#2363471, @MaskRay wrote:

You may ask what objcopy --add-gnu-debuglink=a --set-section-flags=.gnu_debuglink=alloc does. It does not change the flags of .gnu_debuglink and thus our behavior matches GNU.
It seems like an a combo users won't use and I don't bother adding a test.

I wasn't going to, actually :)

Maybe we should allow that, but print a warning that the user is changing flags for a special section, and behavior after that may be unexpected. And apply that warning for other kinds of special sections. That may be beyond the scope of this patch, though.

[grimar]

The change looks good to me.

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

4

GNU objcopy (GNU Binutils for Ubuntu) 2.31.1 doesn't recognize exclude, btw:

objcopy: unrecognized section flag `exclude'
objcopy: supported flags: alloc, load, noload, readonly, debug, code, data, rom, share, contents, merge, strings

[MaskRay]

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

4

Right. I added 'exclude' to GNU objcopy 2.35 :)

[jhenderson]

I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can easily support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.

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

15

It might be a good idea to change this to --- !ELF so that other YAML docs could be added in the future, if needed, without needing to change this line.

[MaskRay]

Add --- to the test

[MaskRay]

In D90438#2367654, @jhenderson wrote:

I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can easily support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.

I know what you are getting at: objcopy -R .symtab --add-symbol newsym=1234 --set-section-flags=.symtab=exclude a a2 => the flags of .symtab are unchanged. This may be due to BFD's special handling of .symtab
It is probably indeed an unhelpful behavior.

In this patch I don't want to touch that area:/ Changing the flags of .symtab is still an unlikely operation. --add-sections .* set-section-flags is more common.

[jhenderson]

In D90438#2368556, @MaskRay wrote:

In D90438#2367654, @jhenderson wrote:

I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can easily support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.

I know what you are getting at: objcopy -R .symtab --add-symbol newsym=1234 --set-section-flags=.symtab=exclude a a2 => the flags of .symtab are unchanged. This may be due to BFD's special handling of .symtab
It is probably indeed an unhelpful behavior.

In this patch I don't want to touch that area:/ Changing the flags of .symtab is still an unlikely operation. --add-sections .* set-section-flags is more common.

My only change request was to just move the code you're moving in this patch to later in the same function. It's no more a complicated a change than the change you are doing already. This may not give all the support necessary, but it should make it easier to add support later on, if the need arises. See inline comment for the concrete location I'm suggesting. If it does start working at that point, you might want a test, but I don't mind too much either way (since as you say it would be a rare behaviour).

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

16–17

Tends to be more common in the YAML I've seen elsewhere.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

745–754

I suggest moving this block to...

773

... here.

[MaskRay]

Comments

[jhenderson]

LGTM!

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

+## Check --set-section-flags works with sections added by --add-section.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --add-section=foo=/dev/null --set-section-flags=foo=alloc,exclude %t %t.out
+# RUN: llvm-readobj -S %t.out | FileCheck %s
+
+# CHECK:      Name: foo
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT:   SHF_EXCLUDE
+# CHECK-NEXT:   SHF_WRITE
+# CHECK-NEXT: ]
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static Error handleArgs(const CopyConfig &Config, Object &Obj,
   if (!Config.SetSectionAlignment.empty()) {
     for (SectionBase &Sec : Obj.sections()) {
       auto I = Config.SetSectionAlignment.find(Sec.Name);
       if (I != Config.SetSectionAlignment.end())
         Sec.Align = I->second;
     }
   }
 
-  if (!Config.SetSectionFlags.empty()) {
-    for (auto &Sec : Obj.sections()) {
-      const auto Iter = Config.SetSectionFlags.find(Sec.Name);
-      if (Iter != Config.SetSectionFlags.end()) {
-        const SectionFlagsUpdate &SFU = Iter->second;
-        setSectionFlagsAndType(Sec, SFU.NewFlags);
-      }
-    }
-  }
-
   if (Config.OnlyKeepDebug)
     for (auto &Sec : Obj.sections())
       if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
         Sec.Type = SHT_NOBITS;
 
   for (const auto &Flag : Config.AddSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
         MemoryBuffer::getFile(File);
     if (!BufOrErr)
       return createFileError(File, errorCodeToError(BufOrErr.getError()));
     std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
     ArrayRef<uint8_t> Data(
         reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
         Buf->getBufferSize());
     OwnedDataSection &NewSection =
         Obj.addSection<OwnedDataSection>(SecName, Data);
     if (SecName.startswith(".note") && SecName != ".note.GNU-stack")
       NewSection.Type = SHT_NOTE;
   }
 
+  // --set-section-flags works with sections added by --add-section.
+  if (!Config.SetSectionFlags.empty()) {
+    for (auto &Sec : Obj.sections()) {
+      const auto Iter = Config.SetSectionFlags.find(Sec.Name);
+      if (Iter != Config.SetSectionFlags.end()) {
+        const SectionFlagsUpdate &SFU = Iter->second;
+        setSectionFlagsAndType(Sec, SFU.NewFlags);
+      }
+    }
+  }
+
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
   // If the symbol table was previously removed, we need to create a new one
   // before adding new symbols.
   if (!Obj.SymbolTable && !Config.ELF->SymbolsToAdd.empty())
     if (Error E = Obj.addNewSymbolTable())
       return E;
 
   for (const NewSymbolInfo &SI : Config.ELF->SymbolsToAdd) {
     SectionBase *Sec = Obj.findSection(SI.SectionName);
     uint64_t Value = Sec ? Sec->Addr + SI.Value : SI.Value;
     Obj.SymbolTable->addSymbol(
         SI.SymbolName, SI.Bind, SI.Type, Sec, Value, SI.Visibility,
         Sec ? (uint16_t)SYMBOL_SIMPLE_INDEX : (uint16_t)SHN_ABS, 0);
   }
 
   if (Config.EntryExpr)
     Obj.Entry = Config.EntryExpr(Obj.Entry);
   return Error::success();
 }
 
 static Error writeOutput(const CopyConfig &Config, Object &Obj, Buffer &Out,
                          ElfType OutputElfType) {
   std::unique_ptr<Writer> Writer =