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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: evgeny777.
jhenderson added a subscriber: evgeny777.
jhenderson added a comment.

In D101332#2732114 <https://reviews.llvm.org/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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101332