[PATCH] D90897: [llvm-objcopy] --only-keep-debug: place zero-size segment according to its parent segment

Tue Nov 10 08:30:58 PST 2020

MaskRay added a comment.

In D90897#2385176 <https://reviews.llvm.org/D90897#2385176>, @jhenderson wrote:

> In D90897#2379198 <https://reviews.llvm.org/D90897#2379198>, @MaskRay wrote:
>
>> In D90897#2378385 <https://reviews.llvm.org/D90897#2378385>, @jhenderson wrote:
>>
>>> Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?
>>
>> PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

It is both Linux and FreeBSD (according to rL288808 <https://reviews.llvm.org/rL288808>). LLD has removeEmptyPTLoad.

> This is an implementation detail that won't necessarily be true for all systems. Kernels could also quite happily ignore zero-sized segments with a trivial piece of code to check the size before calling mmap.
>
>>> In such a case, the PT_TLS segment won't be nested inside anything (see `segmentOverlapsSegment`).
>>
>> Where PT_TLS is nested is an insignificant detail. For --only-keep-debug, the program headers are retained just so that address mapping can work without access to the original program headers. For other program headers, it is rather unimportant whether they are mapped.
>
> I only used PT_TLS here as an example. The same would apply for any zero-sized segment unless it appeared in the middle or at the start of a non-empty segment. The point being that in such a case, it will contain no sections nor will it be nested inside a segment, and so the offset will not get changed by my understanding. This in turn could cause the issue this patch is trying to address.
>
> One thought: how about an empty segment without a parent segment have its offset set to either 0 or `MaxOffset`? That would mean the segment offset isn't outside the file, which would avoid the issue. If the offset of segments in an --only-keep-debug output is unimportant, that would work, I think? Might need some more thought to figure out if there were any other problems there.

Maybe. However, this does not practically exist so I don't think it is important to handle it.
(An alternative is to change the program header type to `PT_NULL` and don't error in `ELFBuilder<ELFT>::readProgramHeaders`, but we may have to fix other binary manipulation tools if pity)

--only-keep-debug is a very specific practical feature with very strong needs. I write the `Seg->MemSize == 0` condition because in D74755 <https://reviews.llvm.org/D74755> we have concluded that it is unclear how to handle such a segment. p_memsz!=0 does not need the rule. I am not sure why we want to emphasize p_memsz=0 PT_LOAD which does not practically exist. For an invalid (or at least, non-real) binary, why do we care so much about theoretical beauty for such a practical feature? (PT_LOAD is useful for mapping addresses. Other PT_LOAD are mostly useless. My first attempt adding --only-keep-debug did not even try adding program headers at all until many folks expressed favor for program headers)

I try to make the p_memsz=0 code simple before we have a good theory supporting empty segments. Why do we want to add more code for non-practical cases?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90897