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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.

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.



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2307
 
 // Rewrite p_offset and p_filesz of non-empty non-PT_PHDR segments after
 // sh_offset values have been updated.
----------------
This comment needs tweaking slightly, since the offset of empty segments is now potentially changing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90897