This is an archive of the discontinued LLVM Phabricator instance.

[ELF][ARM] Allow PT_LOAD to have overlapping p_offset ranges on EM_ARM
ClosedPublic

Authored by MaskRay on Aug 26 2019, 8:52 AM.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 26 2019, 8:52 AM

It feels can be splitted into NFC commit(s) + the actual change.

For example in many places you removed --hash-style=sysv,
renamed the output (e.g. arm-thumb-plt-reloc.s). Sometimes you added
-soname or merged llvm-objdump calls (arm-thumb-thunk-empty-pass.s).
Or you changed llvm-objdump->llvm-readelf. (arm-pie-relative.s)
Or just removed something tested by llvm-readelf (arm-execute-only.s).

Seems it would be much easier to read if you make unnecessary improvements/cleanups
you want to do in a separate NFC commit. What do you think?

ELF/Writer.cpp
2232 ↗(On Diff #217173)

This became large. I am not sure, but does it make sense to invert the condition?

I.e:

bool disabled = ....;
test/ELF/arm-fpic-got.s
63 ↗(On Diff #217173)

0x12134 is not .got address, .got is at 0x12128

test/ELF/arm-tls-ldm32.s
7 ↗(On Diff #217173)

This seems to be unrelative fix/improvement.

65 ↗(On Diff #217173)

This one should be:
(0x2224 - 0x11c4) + (0x11c4 - 0x11c0 - 8) = 0x105C

ruiu added a comment.Aug 27 2019, 2:14 AM

I think this patch doesn't have to be split. As long as the modified tests test the same thing as before, I'm not worried too much, and splitting it into multiple pieces would probably bee too laborious.

ELF/Writer.cpp
2232 ↗(On Diff #217173)

I think this is fine -- this will go away soon once he migrated all the ports.

MaskRay updated this revision to Diff 217336.Aug 27 2019, 2:49 AM
MaskRay marked 5 inline comments as done.

Move some NFC changes to a separate patch to minimize diff.

Adding -soname=t.so to stabilize the output usually does not change layout
but removing --hash-style=sysv changes the layout.

--hash-style=sysv was a remnant (to minimize diff) when we change the default from "sysv" to "both" (r315051). Since this change will change the layout anyway, so delete it as well.

ELF/Writer.cpp
2232 ↗(On Diff #217173)

I plan to do this after RISC-V is fixed => then I think I'll do config->emachine != EM_MIPS && config->emachine != EM_X86_64.

test/ELF/arm-fpic-got.s
63 ↗(On Diff #217173)

Thanks!

MaskRay updated this revision to Diff 217343.Aug 27 2019, 3:05 AM

Some cleanups

grimar accepted this revision.Aug 27 2019, 3:12 AM

This looks much better, thanks! I think it is fine.

(I didn't recheck all the math in comments (though it seems OK),
anyways we will probably remove it and be able to use FileCheck features sooner or later (hopefully)).

This revision is now accepted and ready to land.Aug 27 2019, 3:12 AM

I'd like to see this change on ARM. I'm running a bit behind due to UK public holiday yesterday so I've not had chance to go through all the test comments yet, although these could be fixed up later if needed.

I've done a basic smoke test of doing a check-all build of clang with LLD with this change applied and I didn't run into any problems.

I've done a basic smoke test of doing a check-all build of clang with LLD with this change applied and I didn't run into any problems.

Thanks for testing!

This revision was automatically updated to reflect the committed changes.