[llvm-objcopy] Exclude empty sections in IHexWriter output

[mciantyre]

Summary

IHexWriter was evaluating a section's physical address when deciding if
that section should be written to an output. This approach does not
account for a zero-sized section that has the same physical address as a
sized section. The behavior varies from GNU objcopy, and may result in a
HEX file that does not include all program sections.

The IHexWriter now excludes zero-sized sections when deciding what
should be written to the output. This affects the contents of the
writer's Sections collection; we will not try to insert multiple
sections that could have the same physical address. The behavior seems
consistent with GNU objcopy, which always excludes empty sections,
no matter the address.

The new test case evaluates the IHexWriter behavior when provided a
variety of empty sections that overlap or append a filled section. See
the input file's comments for more information. Given that test input,
and the change to the IHexWriter, GNU objcopy and llvm-objcopy produce
the same output.

[mciantyre]

First time LLVM contributor. Let me know if there are any issues with this submission.

The extra empty section in this patch's test case approximates what I saw in some of my embedded development. This patch allows me to drop GNU objcopy, though I'll admit it hasn't had more real-world testing beyond one example. Thanks for your time and consideration.

[jhenderson]

Thanks for the patch! I'm not particularly familiar with IHex myself. Others in the reviewing list might be able to help out better.

Could you post an example of the output before your change, and after, so that we can see the difference? Also, what happens to empty sections in GNU objcopy when they don't share a physical address with another section? Finally, can you explain why switching the ordering from an address-based ordering to an offset-based ordering is safe?

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

371 ↗

(On Diff #340677)

Any particular reason you've wrapped this function in a distinct namespace?

[MaskRay]

The logic is likely similar to D79229 for -O binary. See the tests contributed in that patch. ihex-elf-segments.yaml adopting a similar style can probably make it easier to read.

[mciantyre]

Thanks for the review, and for all the guidance to get to this point.

Could you post an example of the output before your change, and after, so that we can see the difference?

See the attached before.hex and after.hex. They're derived from input.elf, my real-world test case. before.hex is missing the entire .apps section, which is available in after.hex. I observed that the location of the zero-sized .storage section prevented llvm-objcopy from emitting .apps. Given the same input, GNU objcopy produces after.hex.

what happens to empty sections in GNU objcopy when they don't share a physical address with another section?

When a section is empty, GNU objcopy doesn't emit any HEX output, regardless of its location.

can you explain why switching the ordering from an address-based ordering to an offset-based ordering is safe?

Thanks for bringing this up. I hadn't considered how the comparator would affect not just inclusion, but also ordering. I think it's OK. From my limited study, a HEX file contains lines of numbers resembling

[absolute address]
[relative address][data for section]
[relative address][data]
[relative address][data]
...
[another absolute address]
[relative address][data for next section]
[relative address][data]
...

The section ordering in the HEX file shouldn't matter, since a section always describes its absolute address, followed by data at relative addresses. I gave it a quick test by manually relocating sections in my test case's HEX output. My flashing tool and embedded system still worked as expected.

I have low confidence. I'm not too familiar with HEX either, nor these kinds of ELF details. If we're concerned, I'm happy to bring back the previous IHexWriter::SectionCompare comparator, and introduce a check for section indices that would indicate unique sections, similar to Segment's comparator.

input.elf1 MBDownload

before.hex191 KBDownload

after.hex197 KBDownload

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

371 ↗

(On Diff #340677)

The comparator was previously private to the Segment class. Now that it's shared with the IHexWriter, I wanted a way to signal "this is an implementation detail" to those who include this header. I usually use namespace details for this, but namespace internal had more occurrences throughout the LLVM code base.

[jhenderson]

In D101332#2732114, @mciantyre wrote:

Thanks for the review, and for all the guidance to get to this point.

Could you post an example of the output before your change, and after, so that we can see the difference?

See the attached before.hex and after.hex. They're derived from input.elf, my real-world test case. before.hex is missing the entire .apps section, which is available in after.hex. I observed that the location of the zero-sized .storage section prevented llvm-objcopy from emitting .apps. Given the same input, GNU objcopy produces after.hex.

Thanks, that helped me understand the problem a bit better.

what happens to empty sections in GNU objcopy when they don't share a physical address with another section?

When a section is empty, GNU objcopy doesn't emit any HEX output, regardless of its location.

Okay, thanks. I think it would be a good idea for us to expand the empty section testing for ihex. @MaskRay mentioned something similar earlier. Perhaps a dedicated ihex-empty-sections.test would be better than mixing it all in with the "main" one. In the test case, I'd recommend we have a mixture of sections that share their offset or address with the start and end of other sections, plus at least one that doesn't share with any other section, to show it isn't emitted either. I think you'd need 9 empty sections plus two or three non-empty sections to cover all the bases. The empty sections would be "does not share offset or address with any section", "shares start address", "shares start offset", "shares end address" and "shares end offset", with each of the "sharing" cases actually having two cases where the section appears before it in the section header table, and appears after it in the section header table.

can you explain why switching the ordering from an address-based ordering to an offset-based ordering is safe?

Thanks for bringing this up. I hadn't considered how the comparator would affect not just inclusion, but also ordering. I think it's OK. From my limited study, a HEX file contains lines of numbers resembling

[absolute address]
[relative address][data for section]
[relative address][data]
[relative address][data]
...
[another absolute address]
[relative address][data for next section]
[relative address][data]
...

The section ordering in the HEX file shouldn't matter, since a section always describes its absolute address, followed by data at relative addresses. I gave it a quick test by manually relocating sections in my test case's HEX output. My flashing tool and embedded system still worked as expected.

I have low confidence. I'm not too familiar with HEX either, nor these kinds of ELF details. If we're concerned, I'm happy to bring back the previous IHexWriter::SectionCompare comparator, and introduce a check for section indices that would indicate unique sections, similar to Segment's comparator.

Thanks for the explanation. It makes reasonable sense to me. I wonder whether it would just be simpler to filter out empty ihex sections entirely though, i.e. not adding them to the Sections set at all? There's a ShouldWrite lambda that would be easy to expand to cover this. I'd write the test coverage as described above, and confirm that GNU objcopy never writes the empty section. Assuming that's the case, it seems harmless to switch? @MaskRay/@rupprecht/@evgeny777, does that make sense to you?

[evgeny777]

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

371 ↗

(On Diff #340677)

You can use anonymous namespace here. Both classes are in the same TU, so no need for SectionCompare to have external visibility.

[evgeny777]

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

180

With this change you start using section offset/index instead of address for IHexWriter. Why? Is this something GNU objcopy does?

[mciantyre]

Sounds great, I'll get to work on that dedicated test case. Excluding all empty sections sounds reasonable, but I'll fully assess GNU objcopy's behaviors before making a recommendation.

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

180

I didn't consider the address- vs offset-ordering implications of this change. My goal was to share code, and I was hopeful that if it works for Segment, it might work here. See the discussion around James' previous remarks for ways to undo this.

I've so far treated GNU objcopy as a black box, and I'm not sure how it's generating HEX outputs.

[evgeny777]

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

180

Given that GNU objcopy creates ihex segments in sorted (by physical address) order, I have concerns about this change. ELF segment has designated p_paddr field, so section physical addresses can be of different order than section offsets. I don't know if (and how often) this happens in reality though.

[mciantyre]

Implement James' suggested tests; exclude zero-sized sections from output.

I skipped the "does not share offset or address with any section" test case. There's a similar section in the existing ihex-elf-sections2.yaml test input, part of ihex-writer.test.

[mciantyre]

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

180

Reverted the comparator change. IHex sections are still ordered by physical address.

[evgeny777]

LGTM

[jhenderson]

In D101332#2775675, @mciantyre wrote:

I skipped the "does not share offset or address with any section" test case. There's a similar section in the existing ihex-elf-sections2.yaml test input, part of ihex-writer.test.

Would it make sense to move that test case into the new test? That way all empty section beahviour is localised to a single test.

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

1 ↗

(On Diff #347221)

As this input is only used for a single test, I think it makes more sense to inline this directly in that test.

38 ↗

(On Diff #347221)

70 ↗

(On Diff #347221)

Perhaps also explain which cases each section is testing with comments, like you do above.

107 ↗

(On Diff #347221)

114 ↗

(On Diff #347221)

123 ↗

(On Diff #347221)

125 ↗

(On Diff #347221)

132 ↗

(On Diff #347221)

139 ↗

(On Diff #347221)

146 ↗

(On Diff #347221)

153 ↗

(On Diff #347221)

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

3

You probably want something like --implicit-check-not={{.}} in your FileCheck command, so that no other lines can appear in the output before are after the bits you are actually checking. If you do that, you can omit the trailing CHECK-EMPTY. At the moment, it looks like you could have output before your first checked line.

5

Nit: In newer llvm-objcopy tests, we use ## for comments to distinguish them from FileCheck and lit directives.

[mciantyre]

Would it make sense to move that test case into the new test? That way all empty section beahviour is localised to a single test.

Agreed. Moved the test case into the new test.

[mciantyre]

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

5

Favored ## throughout, including comments of the inlined YAML.

[mciantyre]

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

1 ↗

(On Diff #347221)

Inlined in the latest revision.

[jhenderson]

Sorry for the delay - I was off work last week. Some minor comments, otherwise basically this is good.

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

35

Whilst not required by yaml2obj, it is useful to have this leading ---, because it is required when there are multiple YAML documents in the same file. Adding it now will allow a future expansion of the test with a different YAML input without needing to change this line.

41

Nit: we don't usually leave blank lines within the YAML, as it may make it less clear that this is all one YAML document.

50

I think you can omit the AddressAlign field throughout this YAML.

58

I'm guessing the exact content isn't really important, in which case, I'd just go with something that's 2 or 4 bytes long for brevity (e.g. "0123"). Same goes elsewhere.

83

I think this is a little clearer.

[mciantyre]

Thanks for all the feedback. I'm happy to keep this in review if we're missing anyone's thoughts.

As a reminder: I don't have commit access. Let me know if there's anything you need from me before committing.

[mciantyre]

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

58

Simplified contents, and corrected addresses throughout.

[jhenderson]

LGTM. I'm not in a good position to push this myself right now, due to some other pending work. @evgeny777/@MaskRay (or anybody else), could you push this for @mciantyre, please?

@mciantyre, we'll need your name and email address that you want to appear in the git log in order to push this on your behalf.

[mciantyre]

Thanks all. I'll use Ian McIntyre <[e-mail]> for name and email.

[MaskRay]

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

2671–2672

Adding parentheses around != and > are not common. I'll delete them.

[MaskRay]

minor adjustment

[MaskRay]

reflow test comments using 80-column text width

[MaskRay]

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

180

Unordered p_paddr can happen with AT(addr) in a linker script. I agree that GNU objcopy sorts sections by physical address for -O binary and -O ihex output.

Created D104186 for simplification.

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

 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
-  - Name:            .text
-# Zero length sections are not exported to IHex
-# 'SegmentAddr' and 'ExtendedAddr' records aren't
-# created either.
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
-    Address:         0x7FFFFFFF
-    AddressAlign:    0x8
-    Size:            0
   - Name:            .text1
 # Section address is sign-extended 32-bit address
 # Data fits 32-bit range
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     Address:         0xFFFFFFFF80001000
     AddressAlign:    0x8
     Content:         "0001020304"

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

+## Evaluates the hex writer behavior with empty sections and segments.
+##
+## Show that the presence of an empty section placed at the same address of a
+## filled section doesn't affect the hex output. Also, show that the presence of
+## an empty section placed behind the filled section doesn't affect the hex
+## output. And, show that this happens regardless of the section ordering in the
+## section header table. (Two filled sections, and four empty sections, to
+## realize this test.)
+##
+## Then, show the same kind of behaviors for segments. (One filled section, four
+## empty sections, each in a single segment.)
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy -O ihex %t - | FileCheck %s --implicit-check-not={{.}}
+
+## .data0 address
+# CHECK:         :02000004333394
+## .data0 offset, contents, checksum
+# CHECK-NEXT:    :020000000123DA
+
+## .data1 address
+# CHECK-NEXT:    :02000004444472
+## .data1 offset, contents, checksum
+# CHECK-NEXT:    :02000000456752
+
+## .data2 address
+# CHECK-NEXT:    :0200000477770C
+## .data2 offset, contents, checksum
+# CHECK-NEXT:    :0200000089ABCA
+
+## End of file
+# CHECK-NEXT:    :00000001FF
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_ARM
+Sections:
+## An empty section that's placed at the same address as a populated section.
+## This won't be in the output. It also won't affect how the subsequent section
+## is written.
+  - Name:          .empty_at_data0
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x33330000
+    Size:          0
+## A section populated with data. This is in the output.
+  - Name:          .data0
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x33330000
+    Content:       "0123"
+## An empty section that's placed at the end of .data0. This won't be in the
+## output.
+  - Name:          .empty_behind_data0
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x33330002
+    Size:          0
+## An empty section declared before .data1, but placed behind .data1. This
+## won't be in the output.
+  - Name:          .empty_behind_data1
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x44440002
+    Size:          0
+## A section populated with data. This is in the output.
+  - Name:          .data1
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x44440000
+    Content:       "4567"
+## An empty section declared after .data1, but placed at the start of .data1.
+## This won't be in the output.
+  - Name:          .empty_at_data1
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x44440000
+    Size:          0
+## An empty section that's isolated (by address) from all others. This won't be
+## in the output.
+  - Name:         .empty_isolated
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_ALLOC ]
+    Address:      0x7FFFFFFF
+    AddressAlign: 0x1
+    Size:         0
+## The sections below are placed into segments of varying configurations.
+## Populated section in its own segment. This is in the output.
+  - Name:          .data2
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x77770000
+    Content:       "89AB"
+## Empty section in its own segment. That segment is declared before the .data2
+## segment in the program headers, and placed at an address just behind .data2.
+## This won't be in the output.
+  - Name:          .empty0
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x88880000
+    Size:          0
+## Empty section in its own segment. That segment is declared before the .data2
+## segment in the program headers, and placed at the same address as .data2.
+## This won't be in the output.
+  - Name:          .empty1
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0x99990000
+    Size:          0
+## Empty section in its own segment. That segment is declared after the .data2
+## segment in the program headers, and placed at the same address as .data2.
+## This won't be in the output.
+  - Name:          .empty2
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0xAAAA0000
+    Size:          0
+## Empty section in its own segment. That segment is declared after the .data2
+## segment in the program headers, and placed at an address just behind .data2.
+## This won't be in the output.
+  - Name:          .empty3
+    Type:          SHT_PROGBITS
+    Flags:         [ SHF_ALLOC ]
+    Address:       0xBBBB0000
+    Size:          0
+ProgramHeaders:
+## .data0 sections, with empty bookends.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x33330000
+    VAddr:         0x33330000
+    FirstSec:      .empty_at_data0
+    LastSec:       .empty_behind_data0
+## .data1 sections, with empty bookends.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x44440000
+    VAddr:         0x44440000
+    FirstSec:      .empty_behind_data1
+    LastSec:       .empty_at_data1
+## .empty_isolated section.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x7FFFFFFF
+    VAddr:         0x7FFFFFFF
+    FirstSec:      .empty_isolated
+    LastSec:       .empty_isolated
+## Segments below include a single empty segment, and are positioned around
+## .data2 in various ways.  Declared before, placed behind .data2 segment.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x77770002
+    VAddr:         0x77770002
+    FirstSec:      .empty0
+    LastSec:       .empty0
+## Declared before, placed at .data2 segment.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x77770000
+    VAddr:         0x77770000
+    FirstSec:      .empty1
+    LastSec:       .empty1
+## Segment for .data2.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x77770000
+    VAddr:         0x77770000
+    FirstSec:      .data2
+    LastSec:       .data2
+## Declared after, placed at .data2 segment.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x77770000
+    VAddr:         0x77770000
+    FirstSec:      .empty2
+    LastSec:       .empty2
+## Declared after, placed behind .data2 segment.
+  - Type:          PT_LOAD
+    Flags:         [ PF_R ]
+    PAddr:         0x77770002
+    VAddr:         0x77770002
+    FirstSec:      .empty3
+    LastSec:       .empty3

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

 Error SectionWriter::visit(const Section &Sec) {
   if (Sec.Type != SHT_NOBITS)
     llvm::copy(Sec.Contents, Out.getBufferStart() + Sec.Offset);
 
   return Error::success();
 }
 
 static bool addressOverflows32bit(uint64_t Addr) {
   // Sign extended 32 bit addresses (e.g 0xFFFFFFFF80000000) are ok
   return Addr > UINT32_MAX && Addr + 0x80000000 > UINT32_MAX;
 }
 
 template <class T> static T checkedGetHex(StringRef S) {
   T Value;
   bool Fail = S.getAsInteger(16, Value);
   assert(!Fail);
   (void)Fail;
return createStringError(
         "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);
+    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,