This is an archive of the discontinued LLVM Phabricator instance.

[LLD][PowerPC][test] Update thunk range error report for PPC64PCRelLongBranchThunk
ClosedPublic

Authored by NeHuang on Sep 9 2020, 7:52 AM.

Details

Summary

Update thunk range error report for PPC64PCRelLongBranchThunk after re-basing the branch with https://reviews.llvm.org/D87486
Add a range error test for PPC64R12SetupStub

Diff Detail

Event Timeline

NeHuang created this revision.Sep 9 2020, 7:52 AM
NeHuang requested review of this revision.Sep 9 2020, 7:52 AM
MaskRay added inline comments.Sep 9 2020, 2:08 PM
lld/ELF/Thunks.cpp
913

We have an existing diagnostic like

error: relocation R_AARCH64_ABS32 cannot be used against symbol hidden; recompile with -fPIC

MaskRay added inline comments.Sep 9 2020, 2:13 PM
lld/test/ELF/ppc64-pcrel-call-to-extern-error.s
1 ↗(On Diff #290739)

Delete linux

9 ↗(On Diff #290739)

If a linked .so is used by another link, add -soname to avoid a gotcha

See commit c2a5459d52b94a50581dec7a542ef00915b8d2b2

NeHuang marked 2 inline comments as done.Sep 10 2020, 1:41 PM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
913

Just want to clarify, are you suggesting to follow the same error message print format as error: relocation R_AARCH64_ABS32 cannot be used against symbol hidden; recompile with -fPIC ?

lld/test/ELF/ppc64-pcrel-call-to-extern-error.s
1 ↗(On Diff #290739)

We want to keep the linux to run the test on ppc-linux only since writing to an allocated file will cause time error for this case on freebsd bots. Will add some comments in the tests

9 ↗(On Diff #290739)

Will do.

MaskRay added inline comments.Sep 10 2020, 4:11 PM
lld/ELF/Thunks.cpp
913

I mean to make the diagnostic closer with existing styles if possible.

For this particular one, the second sentence is probably "; recompile with -mcmodel=large"

lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
34

.size is insignificant and can be deleted.

Created D87486 to improve the error messages.

NeHuang updated this revision to Diff 291242.Sep 11 2020, 9:14 AM
NeHuang marked 2 inline comments as done.

Addressed review comments to

  • Change the error message
  • Change the LIT test accordingly
NeHuang marked 3 inline comments as done.Sep 11 2020, 9:14 AM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
913

Thanks. Based on Sean's comment in https://reviews.llvm.org/D86706, I would change the second sentence to "; recompile using the large code model".

NeHuang marked an inline comment as done.Sep 11 2020, 9:15 AM
NeHuang updated this revision to Diff 292618.EditedSep 17 2020, 2:28 PM

Rebased with ToT to integrate with the changes in https://reviews.llvm.org/D87486

NeHuang retitled this revision from [LLD][PowerPC][test] Add test cases for all pc-rel based stubs when the offset not fit in 34 bits to [LLD][PowerPC][test] Update thunk range error report for PPC64PCRelLongBranchThunk.Sep 17 2020, 2:40 PM
NeHuang edited the summary of this revision. (Show Details)
NeHuang edited the summary of this revision. (Show Details)
NeHuang updated this revision to Diff 292625.Sep 17 2020, 2:43 PM

Clang-format

MaskRay added inline comments.Sep 17 2020, 2:56 PM
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

Delete system-linux

I think this test is portable and runs on other systems.

26

Consider deleting unneeded setup instructions

nemanjai accepted this revision.Sep 18 2020, 5:53 AM

This LGTM but of course, wait to hear from @MaskRay because he had test case concerns.

lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

It appears that the file systems on FreeBSD and Linux behave differently. On Linux, writing the large file (8GB in this case) happens in a fraction of a second which suggests that it is not actually writing the bytes to storage. However, on FreeBSD this takes a significant amount of time - perhaps it is actually writing the bytes to storage. So we limit these test cases that produce large files to Linux to avoid this problem.

The issue can be avoided on FreeBSD by writing out to /dev/null or some other special file because that won't mmap - it creates a memory buffer. However, that causes a different problem on systems that limit virtual memory - the OS won't allow the process to allocate a memory buffer that size.

So the easiest solution we found was to write to a real file and limit this to Linux.

This revision is now accepted and ready to land.Sep 18 2020, 5:53 AM
NeHuang marked 3 inline comments as done.Sep 18 2020, 7:42 AM
NeHuang added inline comments.
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
26

I could remove them but it would be better to keep them because the asm doesn't really make sense for a toc-based callee without the toc setup. Of course, I can remove them if you insist.

NeHuang marked an inline comment as done.Sep 18 2020, 7:42 AM
sfertile added inline comments.Sep 21 2020, 8:03 AM
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

Thanks for the thorough explanation.
Really minor nit: can we have the comment that explains this right after the requires directive instead of after the linker script?

MaskRay added inline comments.Sep 21 2020, 8:34 AM
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

In this case, PHDRS and :segment should be specified to assign sections to different text segments.

NeHuang updated this revision to Diff 293188.Sep 21 2020, 8:46 AM

Move up the comment in the test case.

MaskRay requested changes to this revision.Sep 21 2020, 8:51 AM
This revision now requires changes to proceed.Sep 21 2020, 8:51 AM
sfertile added inline comments.Sep 21 2020, 10:30 AM
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

This is outside my wheelhouse, so I have to ask for clarification: The test can use PHDRS command to create multiple small load segments, then in the SECTIONS command we would have 2 lines like:

.output_section_name  { input_sections_to_match } : load_segment_name

The first to match callers section, then increase the address sufficiently far away and have a second similar line for the callees section?

MaskRay added inline comments.Sep 21 2020, 11:11 AM
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

See D88037 for an example

sfertile added inline comments.Sep 21 2020, 11:21 AM
lld/test/ELF/ppc64-pcrel-call-to-toc-error.s
1

Nice, thanks!

NeHuang updated this revision to Diff 293237.Sep 21 2020, 1:23 PM

Thanks @MaskRay and @sfertile Test cases have been updated with PHDRS to assign sections to different text segments.

NeHuang marked 5 inline comments as done.Sep 21 2020, 1:24 PM
MaskRay accepted this revision.Sep 21 2020, 1:25 PM

Thanks!

This revision is now accepted and ready to land.Sep 21 2020, 1:25 PM