[PATCH] D74755: [llvm-objcopy] Attribute an empty section to a segment ending at its address

Fri May 22 03:00:54 PDT 2020

jhenderson added a comment.

In D74755#2048026 <https://reviews.llvm.org/D74755#2048026>, @MaskRay wrote:

> So, is `uint64_t SecSize = Sec.Size ? Sec.Size : 1;` a subtle condition we may want to get rid of? Or is simplifying code can still be problematic? IMHO there are a few points regarding hypothetical use cases but: (1) they don't appear to be used (2) their reliance on the current behavior does not have a solid guarantee.

I'm not sure I entirely follow. I'm open to the logic being simplified if it can be done without breaking behaviour, but as things stand, this patch breaks at least one behaviour as demonstrated in my previous two comments. ihex is not a "hyopthetical use-case" as far as I'm aware (if it were, why was support for it added?) and the layout I experimented with was not unreasonable.

There are plenty of users out there with their own linker scripts with layouts that look unusual compared to the "default" layout, and for various reasons. Just because they are not involved in this conversation does not mean they are "hypothetical use cases" that can be ignored. I've had to help one team with such a linker script on more than one occasion. Usually, it's because they've broken gABI rules in some way (e.g. PT_LOAD isn't in ascending address order). I'd like to avoid having to explain to them that they need to change their layout from something they thought worked, just because we decided to introduce an implicit restriction into llvm-objcopy.

If we accept the principle that an empty section should be assigned to an empty segment with the same offset to fix --only-keep-debug layout behaviour (but see my earlier comments where I'm not convinced by that), it seems to me like the issue is "what should the parent of an empty section sandwiched between two segments be?" We have 4 options:

1. Both - behaviour of the current patch; works for empty segments, but which should be the parent segment? Current patch breaks ihex, whilst previous patch assigns it to the left segment which changes layout in a way inconsistent with GNU behaviour (it's admittedly debatable as to whether that behaviour matters, but I'm not comfortable with it, especially as I could see symbols being in those sections which are somehow being relied upon).
2. Neither - won't solve the issue - segment will continue to have no section so won't be laid out.
3. Segment before - won't solve the issue - an empty segment following a non-empty one will still not have any section.
4. Segment after - the current behaviour (but broken for empty segments appearing after).

As previously mentioned, readelf in its section to segment mapping does not include empty sections at the end of empty segments, unless the segment is also empty (llvm-readelf does not do the "unless", but I think that's a bug). I think we should aim to match this algorithm for consistency because that a) fixes the problem, and b) doesn't change any other behaviour. It also matches what GNU objcopy does. The downside is we don't get to significantly simplify the code, although I don't think it complicates it any more than it was before - I think the slightly subtle `size = 1` code gets replaced by the clearer `if (size == 0) do something` code.

If you aren't happy adding a `size == 0` special case, then I think we are at an impasse and need more input from others.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74755