[llvm-objcopy] Make ihex writer similar to binary writer

[MaskRay]

Summary

There is no need to differentiate whether UseSegments is true or
false. Unifying the cases makes the behavior closer to BinaryWriter.

This improves compatibility with objcopy because SHF_ALLOC sections not in
a PT_LOAD will not be skipped. Such cases are usually erroneous input, though.

[jhenderson]

Just trying to follow the logic through. I think there is a potential behaviour change, which may be fine, but I just want to highlight it to see what others think (I have no use for IHEX myself, so don't feel like I can make a completely fair judgment either way). If I follow it correctly, if a writable section were not in a PT_LOAD segment, then previously it was skipped, if there was at least one writable section that was in such a segment, whereas now it won't be skipped. This is probably harmless (all such sections should have been placed in a segment anyway). Assuming I'm right, should we have a test-case to illustrate this behaviour change?

[MaskRay]

In D104186#2816334, @jhenderson wrote:

Just trying to follow the logic through. I think there is a potential behaviour change, which may be fine, but I just want to highlight it to see what others think (I have no use for IHEX myself, so don't feel like I can make a completely fair judgment either way). If I follow it correctly, if a writable section were not in a PT_LOAD segment, then previously it was skipped, if there was at least one writable section that was in such a segment, whereas now it won't be skipped. This is probably harmless (all such sections should have been placed in a segment anyway). Assuming I'm right, should we have a test-case to illustrate this behaviour change?

I think the coverage value is low. SHF_WRITE sections should have the SHF_ALLOC flag and be in a PT_LOAD segment. If not, that looks like an odd case. Not sure such case does arise for ihex users.

[mciantyre]

if a writable section were not in a PT_LOAD segment, then previously it was skipped, if there was at least one writable section that was in such a segment, whereas now it won't be skipped.

We can manually demonstrate this by adding SHF_ALLOC to the .dummy section's flags in ihex-elf-segments.yaml. The .dummy section is intentionally excluded from the segment in that test. As of this patch, when .dummy is SHF_ALLOC, we'll write the section's contents even though its not part of a segment.

GNU objcopy always emits data-containing sections in IHEX output, even if the section is not part of a segment. If meeting parity with GNU objcopy is a goal, this patch makes LLVM objcopy behave similarly. We can check GNU objcopy with the ihex-elf-segments.yaml output, when .dummy is SHF_ALLOC. I tested this with GNU objcopy 2.36.1.

[MaskRay]

add a test

[jhenderson]

LGTM.

llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-segments.yaml

Sections:
   - Name:            .bss
     Type:            SHT_NOBITS
     Flags:           [ SHF_ALLOC ]
     Address:         0x10100
     Size:            0x1000
     AddressAlign:    0x8
   - Name:            .dummy
     Type:            SHT_PROGBITS
-    Flags:           [ ]
+    Flags:           [ SHF_ALLOC ]
     Address:         0x20FFF8
-    Size:            65536
+    Size:            3
     AddressAlign:    0x8
+  - Name:            .nonalloc
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+    Address:         0x300000
+    Size:            1
 ProgramHeaders:
   - Type:     PT_LOAD
     Flags:    [ PF_X, PF_R ]
     VAddr:    0xF00000000
     PAddr:    0x100000
     FirstSec: .text
     LastSec:  .bss

llvm/test/tools/llvm-objcopy/ELF/ihex-writer.test

 # CHECK-NEXT: :00000001FF
 
 # SEGMENTS:       :020000040010EA
 # SEGMENTS-NEXT:  :10000000000102030405060708090A0B0C0D0E0F78
 # SEGMENTS-NEXT:  :05001000101112131491
 # SEGMENTS-NEXT:  :0B001800303132333435363738394090
 # SEGMENTS-NEXT:  :0400280040414243CE
 # SEGMENTS-NEXT:  :0B003000505152535455565758596018
+# SEGMENTS-NEXT:  :020000040020DA
+# SEGMENTS-NEXT:  :03FFF80000000006
 # SEGMENTS-NEXT:  :0400000500100000E7
 # SEGMENTS-NEXT:  :00000001FF
+# SEGMENTS-NOT:   {{.}}
 
 # 'ExtendedAddr' (04) record shouldn't be created
 # PT_NULL-NOT: :02000004
 
 # SIGN_EXTENDED:      :0200000480007A
 # SIGN_EXTENDED-NEXT: :051000000001020304E1
 # SIGN_EXTENDED-NEXT: :00000001FF
 

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

if (addressOverflows32bit(Addr) || addressOverflows32bit(Addr + Sec.Size - 1))
     return createStringError(
         errc::invalid_argument,
         "Section '%s' address range [0x%llx, 0x%llx] is not 32 bit",
         Sec.Name.c_str(), Addr, Addr + Sec.Size - 1);
   return Error::success();
 }
 
 Error IHexWriter::finalize() {
-  bool UseSegments = false;
-  auto ShouldWrite = [](const SectionBase &Sec) {
-    return (Sec.Flags & ELF::SHF_ALLOC) && Sec.Type != ELF::SHT_NOBITS &&
-           Sec.Size > 0;
-  };
-  auto IsInPtLoad = [](const SectionBase &Sec) {
-    return Sec.ParentSegment && Sec.ParentSegment->Type == ELF::PT_LOAD;
-  };
-
   // We can't write 64-bit addresses.
   if (addressOverflows32bit(Obj.Entry))
     return createStringError(errc::invalid_argument,
                              "Entry point address 0x%llx overflows 32 bits",
                              Obj.Entry);
 
-  // If any section we're to write has segment then we
-  // switch to using physical addresses. Otherwise we
-  // use section virtual address.
-  for (const SectionBase &Sec : Obj.sections())
-    if (ShouldWrite(Sec) && IsInPtLoad(Sec)) {
-      UseSegments = true;
-      break;
-    }
-
   for (const SectionBase &Sec : Obj.sections())
-    if (ShouldWrite(Sec) && (!UseSegments || IsInPtLoad(Sec))) {
+    if ((Sec.Flags & ELF::SHF_ALLOC) && Sec.Type != ELF::SHT_NOBITS &&
+        Sec.Size > 0) {
       if (Error E = checkSection(Sec))
         return E;
       Sections.insert(&Sec);
     }
 
   std::unique_ptr<WritableMemoryBuffer> EmptyBuffer =
       WritableMemoryBuffer::getNewMemBuffer(0);
   if (!EmptyBuffer)