[PATCH] D101332: [llvm-objcopy] Keep ihex sections with same address

[Ian McIntyre via Phabricator via llvm-commits]

mciantyre added a comment.

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.F16620294: input.elf <https://reviews.llvm.org/F16620294>

F16620293: before.hex <https://reviews.llvm.org/F16620293>

F16620292: after.hex <https://reviews.llvm.org/F16620292>



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:371
 
-class IHexWriter : public Writer {
-  struct SectionCompare {
-    bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const;
-  };
+namespace internal {
+struct SectionCompare {
----------------
jhenderson wrote:
> Any particular reason you've wrapped this function in a distinct namespace?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101332/new/

https://reviews.llvm.org/D101332