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

Tue Mar 31 03:18:19 PDT 2020

jhenderson added a subscriber: jakehehrlich.
jhenderson added a comment.

Sorry for the delay in coming back again to this. It's taken me quite some time to collect my thoughts and find time for it, amidst all the other reviews people want my input on, not to mention my actual own workload.

One of the things that @jakehehrlich and I tried hard to achieve when writing llvm-objcopy was to ensure that llvm-objcopy can handle cleanly any valid input. By "valid" I mean compliant with the ELF gABI. The intent of that was to ensure that the output could be consumed by any gABI-compliant tool, and can similarly consume any input generated by such tools. Hence, it is important to be able to handle any segment ordering, regardless of whether tools commonly emit it or not. The reasoning behind this is that there are many different usages of the tools out there, and we cannot account for all use-cases, especially if we are not accounting for valid things in the ELF standard (examples include segments that are not nested inside PT_LOADs, PT_TLS segments before PT_LOAD segments and so on). "Simplifying" code that removes support for a valid layout (by changing behaviour in an undesirable way) is not a good thing to do, no matter how much of an edge case that behaviour may be, in my opinion. Additionally, I don't think we should care about binary mainpulation tools that cannot handle valid input - any such tools are not gABI compliant. Anyway, that's all mostly just high-level comments, and not all that useful, given that there is a known issue here.

More specific to the issue at hand: whilst we can certainly do something in llvm-objcopy to improve the situation @kongyi faces, I wonder if we could (should?) workaround it by relaxing some of the checks that are failing with the output? An empty segment's p_offset to me doesn't seem to really matter. It could be 0, it could be 0xffffffffffffffff. Because it has no data, it doesn't really matter where the offset is, so perhaps the checks should ignore empty segments.

@kongyi, asides from llvm-objcopy, what other tools have you seen produce the failure?

Finally, back to the current fix in llvm-objcopy. I think, to avoid confusion, it would be good for nesting of segments within other segments and sections within segments to behave the same. I note that at the moment, an empty segment is considered to overlap (and therefore be nested inside) a segment that it is at the beginning of, but not at the end of. Empty segments are not considered to be part of other empty segments at the same offset ever. Segment parents are canonicalised such that no parent segment itself has a parent segment. This simplifies the layout algorithms, because segments with a parent simply follow their parent, rather than trying to figure out where they are supposed to go independently. Prior to this patch, sections follow the same rules: they have a most parental segment, and empty sections belong to a segment iff they are wholly within or at the start of a non-empty segment.

A relevant consideration here is that there is no way of identifying whether an empty section/segment should belong to another empty segment. For example, if there are two empty segments and an empty section, all with the same offset but different alignments, how should layout be performed? I don't think there's a clear correct answer here, without further context. Clearly it makes sense for a SHT_TLS section to belong to the TLS segment, but what about other sections and segment types? Currently, I believe llvm-objcopy will lay them all out independently, according to their respective alignment restrictions. With this patch, if I'm not mistaken the section will be associated with both segments, with its parent being the first of the two in the program header table, I believe. It will therefore get moved with that segment, which could result in incorrect output due to alignment violations or similar (before the section's alignment would be considered, but now it would not be).

A related note is that llvm-readelf does not consider empty sections to be part of a segment at the same location, unlike GNU readelf. I haven't identified the exact rules the two follow though. GNU objcopy doesn't have the same issue we do, because it moves at least an empty PT_TLS segment to offset 0, regardless of where it is placed in relation to anything else (the .tdata and .tbss sections remain in their PT_LOAD). I don't know why - it doesn't seem to do this for other segments.

Sorry, this is getting a bit rambly. I keep coming back to the issue being actually in the `layoutSegmentsForOnlyKeepDebug` code: should we really be completely ignoring segments without sections? I can't help but feel like we should either pad the output's file size to compensate for the empty segment's offset, if necessary, or change the empty segment's offset to 0 (or indeed any good offset). Alternatively, we should loosen the checks as mentioned earlier, so that the tools don't complain.

I've got to leave it here. I was hoping to come up with another reproducible example which this patch clearly breaks, rather than just relying on higher-level comments, but I've run out of time. I'll have to do that another time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74755