[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

5

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

5

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

16

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...

762–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/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test

 # CHECK-NEXT: [ 3] .shstrtab STRTAB   0000000000000000 {{.+}} 000028 00      0  0   1
 # CHECK-NEXT: [ 4] .symtab   SYMTAB   0000000000000000 {{.+}} 000030 18      3  1   8
 
 # CHECK: Symbol table '.symtab' contains 2 entries:
 # CHECK-NEXT:   Num:    Value          Size Type    Bind   Vis       Ndx Name
 # CHECK-NEXT:     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND
 # CHECK-NEXT:     1: 00000000000004d2     0 NOTYPE  GLOBAL DEFAULT   ABS newsym
 
+## --set-section-flags works for newly created .symtab, which GNU objcopy does not do.
+# RUN: llvm-objcopy -R .symtab --add-symbol newsym=1234 --set-section-flags .symtab=readonly,exclude %t %t3
+# RUN: llvm-readelf -S %t3 | FileCheck %s --check-prefix=CHECK2
+
+# CHECK2:     [ 4] .symtab   SYMTAB   0000000000000000 {{.+}} 000030 18  E   3  1   8
+
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:
   - Name:            .text

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;
   }
 
   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);
   }
 
+  // --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.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 =