This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Nov 5 2020, 8:02 PM.

Details

Summary

Alternative to D74755. sectionWithinSegment() treats an empty section as having
a size of 1. Due to the rule, an empty .tdata will not be attributed to an
empty PT_TLS. (The empty p_align=64 PT_TLS is for Android Bionic's TCB
compatibility (ELF-TLS). See https://reviews.llvm.org/D62055#1507426)

Currently --only-keep-debug will not layout a segment with no section
(layoutSegmentsForOnlyKeepDebug()), thus p_offset of PT_TLS can go past the end
of the file. The strange p_offset can trigger validation errors for subsequent
tools, e.g. llvm-objcopy errors when reading back the separate debug file
(readProgramHeaders()).

This patch places such an empty segment according to its parent segment. This
special cases works for the empty PT_TLS used in Android. For a non-empty
segment, it should have at least one non-empty section and will be handled by
the normal code.

Diff Detail

Event Timeline

MaskRay created this revision.Nov 5 2020, 8:02 PM
MaskRay requested review of this revision.Nov 5 2020, 8:02 PM
MaskRay edited the summary of this revision. (Show Details)Nov 5 2020, 8:05 PM

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too? In such a case, the PT_TLS segment won't be nested inside anything (see segmentOverlapsSegment).

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test
238
252

It's not clear from the test comments why we need the .got, .note and .text sections in this output. Either remove them or add comments explaining their purpose.

277

Note: @grimar is working on D90458 which changes how sections within segments are represented.

llvm/tools/llvm-objcopy/ELF/Object.cpp
2314

Do you mean offset here?

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.

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.

MaskRay updated this revision to Diff 303472.Nov 6 2020, 8:51 AM
MaskRay marked 3 inline comments as done.

Comments

MaskRay marked an inline comment as done.Nov 6 2020, 8:51 AM
MaskRay added inline comments.
llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test
252

Added a comment.

277

I can rebase.

MaskRay marked an inline comment as done.Nov 9 2020, 10:18 AM

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.

llvm/tools/llvm-objcopy/ELF/Object.cpp
2307

This comment needs tweaking slightly, since the offset of empty segments is now potentially changing.

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). 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 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?

MaskRay updated this revision to Diff 304204.Nov 10 2020, 8:34 AM

Adjust comments

jhenderson accepted this revision.Nov 11 2020, 12:26 AM

LGTM - I realised last night whilst thinking about this that we can always enhance this further if the need arises - this change doesn't degrade the experience for any user, whilst improving it for the majority of use-cases.

llvm/tools/llvm-objcopy/ELF/Object.cpp
2313

"no containing section" implies to me that there is no section that is a parent of the segment.

This revision is now accepted and ready to land.Nov 11 2020, 12:26 AM
MaskRay marked 2 inline comments as done.Nov 11 2020, 12:38 AM

Thanks!

MaskRay updated this revision to Diff 304417.Nov 11 2020, 12:39 AM

Fix a comment

This revision was landed with ongoing or failed builds.Nov 11 2020, 9:21 AM
This revision was automatically updated to reflect the committed changes.
enh added a subscriber: enh.Nov 12 2020, 12:50 PM

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). 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.

that might have been sensible ... but POSIX actually _requires_ that an mmap() of length 0 fails with EINVAL (it's a "shall fail", not just a "may fail": https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html).

In D90897#2392316, @enh 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). 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.

that might have been sensible ... but POSIX actually _requires_ that an mmap() of length 0 fails with EINVAL (it's a "shall fail", not just a "may fail": https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html).

Right. What I was referring to was that a kernel which uses mmap to map PT_LOAD segments into memory could have a simple if(p_memsz != 0) clause to skip the call of mmap in the first place. So this wasn't a reference to what mmap may or may not do, it was a reference to what the kernel might do when faced with a PT_LOAD of size 0.