Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For compiletests, use the linker if present. #109544

Closed
wants to merge 1 commit into from

Conversation

jfgoog
Copy link
Contributor

@jfgoog jfgoog commented Mar 23, 2023

When running x.py test tests/ui with a custom linker script, we found that 204 tests actually invoke the default linker (/usr/bin/ld) instead of the linker script. Depending on the system linker, this will cause those tests to fail. Many of the failing tests (196) use // aux-build:... directives. One such example is tests/ui/annotate-snippet/multispan.rs.

In cases where there were failures, we ran with -v and confirmed that we saw "linker: Some(<path deleted>)" when the compiletest configuration was dumped. Nevertheless, the rustc command lines for the failing tests did not contain -Clinker=XXX, resulting in errors like the following:

= note: /usr/bin/ld: /path/to/libunwind.a(UnwindLevel1-gcc-ext.o): unrecognized relocation (0x2a) in section `.text._Unwind_GetDataRelBase'
        /usr/bin/ld: final link failed: Bad value
	collect2: error: ld returned 1 exit status

@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 23, 2023

I suspect this fix is too crude, but I'm not familiar enough with the code to do better. Nevertheless, I wanted to document the problem and propose something concrete.

@albertlarsan68 albertlarsan68 removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 25, 2023
@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 30, 2023

Hello. Do you have any feedback or suggestions on this?

@albertlarsan68
Copy link
Member

Thanks for the PR and sorry for the delay.
@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 31, 2023

📌 Commit 64225f5a8d90ebef1e3962b1063af3ff2f45dd2c has been approved by albertlarsan68

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2023
@lqd
Copy link
Member

lqd commented Mar 31, 2023

Maybe squash the commits first so that there is no submodule change ?

@albertlarsan68
Copy link
Member

My bad.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 31, 2023
@jfgoog
Copy link
Contributor Author

jfgoog commented Apr 3, 2023

Thanks for the review. I'm actually not sure this is ready to be committed. The reason is that there is code elsewhere that adds -Clinker=XXX selectively, and this change enables it whenever it's present, and I wasn't entirely sure that was correct or excessive. If we really do use a custom linker whenever it is specified, we should presumably also remove the special cases where we enable it selectively.

When running x.py test tests/ui with a custom linker script, we found
that 204 tests actually invoke the default linker (/usr/bin/ld) instead
of the linker script. Depending on the system linker, this will cause
those tests to fail. Many of the failing tests (196) use
"// aux-build:..." directives. One such example is
tests/ui/annotate-snippet/multispan.rs.

In cases where there were failures, we ran with -v and confirmed that we
saw "linker: Some(<path deleted>)" when the compiletest configuration
was dumped. Nevertheless, the rustc command lines for the failing tests
did not contain -Clinker=XXX, resulting in errors like the following:

= note: /usr/bin/ld: /path/to/libunwind.a(UnwindLevel1-gcc-ext.o): unrecognized relocation (0x2a) in section `.text._Unwind_GetDataRelBase'
        /usr/bin/ld: final link failed: Bad value
	collect2: error: ld returned 1 exit status
@jfgoog
Copy link
Contributor Author

jfgoog commented Apr 3, 2023

@bors retry
@bors r? albertlarsan68

@bors
Copy link
Contributor

bors commented Apr 3, 2023

@jfgoog: 🔑 Insufficient privileges: not in try users

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2023

Could not assign reviewer from: albertlarsan68.
User(s) albertlarsan68 are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

@jfgoog
Copy link
Contributor Author

jfgoog commented Apr 5, 2023

Just following up again.

My original thinking with this CL was that if we went to the trouble of specifying a custom linker, we should use it when running the tests.

But I'm not sure if this is excessive, or counter to the intention of allowing a linker to be passed to the test runner.

What are your thoughts?

@jfgoog
Copy link
Contributor Author

jfgoog commented Apr 6, 2023

Following up from chat in t-infra/bootstrap: Current theory is that the host could have a custom linker as well, and there is no way to pass that from the bootstrap code, only the target linker. So the logic for // force-host (correctly) doesn't use the (target) linker, but (incorrectly) assumes it can just use the default linker.

@jfgoog
Copy link
Contributor Author

jfgoog commented Apr 6, 2023

Superseded by #110018

@jfgoog jfgoog closed this Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants