Page MenuHomePhabricator

[llvm-objcopy] Don't specialize the all zero p_paddr case
ClosedPublic

Authored by MaskRay on Apr 23 2020, 8:49 PM.

Details

Summary

Spotted by https://reviews.llvm.org/D74755#1998673

it looks like OrderedSegments in the function is only used to set the physical address to the virtual address when there are no physical addresses set amongst these sections.

I believe this behavior was copied from https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6ffd79000b45e77b3625143932ffbf781b6aecab (2008-05)
The commit was made for very old linkers.
This special rule does not seem useful and remove it can allow us to
delete a large chunk of code.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 23 2020, 8:49 PM
Herald added a reviewer: alexshap. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Apr 23 2020, 9:07 PM

IIRC from what I looked at yesterday, the segments aren't actually used for anything in binary output now, except in the calculation for MinAddr and SecAddr. Assuming that's correct (please check if you're going to do this!), does that mean we could simplify to looping over all Segments in the code you're modifying rather than just those which are ParentSegment instances?

grimar added inline comments.Apr 24 2020, 1:19 AM
llvm/tools/llvm-objcopy/ELF/Object.cpp
2218–2219

all_of -> llvm::all_of
(it is consistent with how LLVM uses algorithms and makes it clear that we use llvm version of a algorithm).

MaskRay updated this revision to Diff 259901.Apr 24 2020, 9:05 AM
MaskRay retitled this revision from [llvm-objcopy] Delete dead code after D71035. NFC to [llvm-objcopy] Don't special the all zero p_paddr case.
MaskRay edited the summary of this revision. (Show Details)

Drop a special case to simplify code. No longer a NFC.

jhenderson accepted this revision.Apr 27 2020, 12:23 AM
jhenderson added a subscriber: jakehehrlich.

I believe this behavior was copied from https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6ffd79000b45e77b3625143932ffbf781b6aecab (2008-05)

From reading the original review history etc of the change that implemented this in llvm-objcopy, I couldn't see evidence that this is true, except that I recall @jakehehrlich was trying to copy GNU objcopy's behaviour in all things that made some degree of sense. I believe he did this from a mixture of trial and error and reading objcopy code, but I'm not sure.

Assuming that it was to mirror the GNU behaviour, and the GNU behaviour is as you described (I'm getting timeout errors when I go to that link), I'm fine with this, but I'd like @jakehehrlich to chime in if he's still around. If he doesn't do so in the next few days, this can go in.

llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test
9–10

I'm not sure this sentence is right - I assume you mean p_paddr, not p_paddr.

13–14

If the two outputs should be the same, would simply cmp %t1.out %t0.out1 be simpler?

This revision is now accepted and ready to land.Apr 27 2020, 12:23 AM
grimar added inline comments.Apr 27 2020, 1:20 AM
llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test
44

nit: perhaps you can remove Align here and above while you are here. It doesn't seem to be useful for this test?

MaskRay updated this revision to Diff 260357.Apr 27 2020, 9:37 AM
MaskRay marked 3 inline comments as done.

Address comments

MaskRay marked an inline comment as done.Apr 27 2020, 9:37 AM

https://reviews.llvm.org/D41619 is where I think this stuff was introduced for context.

So confirmed from reading that this behavoir was just copied by a bit of digging in the code itself. I don't believe a real world use case came up that required this. Reading the bug that Fangrui linked but after that this has my LGTM

Yeah I don't suspect anyone will be bitten by this, it speeds up this use case, and speeds up all other use cases, and I don't think there was ever a strong reason for adding this specific behavior.

jakehehrlich accepted this revision.Apr 27 2020, 11:06 AM
MaskRay retitled this revision from [llvm-objcopy] Don't special the all zero p_paddr case to [llvm-objcopy] Don't specialize the all zero p_paddr case.Apr 27 2020, 11:17 AM
This revision was automatically updated to reflect the committed changes.