[PATCH] D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets.

Tue Mar 19 14:46:00 PDT 2019

jakehehrlich added a comment.

If you think about TLS as being in a sort of different address space (which was not a way I had thought about this before) then this is the function that determines if a section is in the same address area as a segment so its a semantically consistent idea I think. Layout should also always succeed because the PT_TLS will be given the respective PT_LOAD as a parent which will fix its offset correctly and then any sections assigned only to PT_TLS and not a PT_LOAD will still be laid out correctly (which should always be the case for any overlapping segment, we should have tests for this already). We are however changing what this function means even if in practice it's true/false in almost all of the same cases. For non-NOBITS `Segment.VAddr - Section.Addr == Segment.Offset - Section.OriginalOffset` and those sections will never fall in the range past `Segment.FileSize` so the predicate should return true in all the same cases for non-NOBITS. It would also now assign sections to the "expected" segment even if today that has no affect one way or the other.

Things get more tricky if you change segmentOverlapsSegment to behave that way. In that case its critical that PT_TLS have a PT_LOAD as its parent. The subtle reason behind why we use VAddr in one case and Offset in another is going to be confusing to explain to people (as if the reason we used Offset and not VAddr wasn't already complicated enough)

I was going to point the non-allocated section issue out as well but it seems you've already seen that. Since non-allocate sections don't have meaningful vaddrs this is tricky. Independent of this change as it exists right now I am 100% ok with using Offset/FileSize for most sections and using VAddr/MemSize for NOBITS (since in principal NOBITS should be allowed to have any offset). I think that would be a better heuristic for assigning NOBITS sections to segments. It should be the case that weather or not a NOBITS section is assigned to a segment, layout should not be affected anyhow. This seems like a less semantically consistent operation however. As the code exists today it assigns a size of 1 to NOBITS so they always wind up assigned to the wrong PT_LOAD if they wind up assigned at all. This only matters in so far as the offset is a bit wonky but this causes no general problem since the offset of a NOBITS section is meaningless (though an expected answer exists and we should match it).

An alternative is to do this change on allocated sections (e.g. vaddr/memsize and account for TLS), and then to use offset/filesize on non-allocated sections. I think this would solve the issues being seen in D59293 <https://reviews.llvm.org/D59293> and would always use the meaningful offset unless a non-allocated NOBITS section existed...but such a thing should just be assumed to never exist as part of a segment I think.

I still however maintain that if you're relaying on what segment a NOBITS section falls into using ParentSegment then you're doing something wrong. That said there might be an offset bug we should fix for expectation's sake. If we're only trying to fix the offset bug for expectations sake then I support using VAddr/MemSize for NOBITS only as the correct solution. Could you describe your use case a bit more perhaps?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58426