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

Fix ABI flags in RISC-V/LoongArch ELF file generated by rustc #114332

Merged
merged 4 commits into from
Aug 20, 2023

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Aug 1, 2023

Fix #114153

It turns out the current way to set these flags are completely wrong. In LLVM the target ABI is used instead of target features to determine these flags.

Not sure how to write a test though. Or maybe a test isn't necessary because this affects only those touching target json?

r? @Nilstrieb

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 1, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nbdd0121 nbdd0121 requested a review from Nilstrieb August 3, 2023 12:51
@@ -79,6 +79,14 @@ LL | let (A(A(..) | B(a), _) | B(A(a, _) | B(a))) = Y;
| |
| pattern doesn't bind `a`

error[E0408]: variable `c` is not bound in all patterns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened here... I assume this is related to adding c as a known symbol???
Yeah, I renamed c to builtin_syntax and the error changed in nightly. Looks like it's sorting the symbols by their index? That's strange and bad, but luckily not your problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I see, I'll fix it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nilstrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2023

📌 Commit 8c53a72df236240bb8926ecf52c39aee4c628640 has been approved by Nilstrieb

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 Aug 4, 2023
@bors
Copy link
Contributor

bors commented Aug 4, 2023

⌛ Testing commit 8c53a72df236240bb8926ecf52c39aee4c628640 with merge ee51c76c691b1bb00208115da9321d0422b8b1ee...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 4, 2023

💔 Test failed - checks-actions

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

nbdd0121 commented Aug 4, 2023

Some of the RISC-V targets don't have llvm_abiname set explicitly. I now default unset llvm_abiname to be the same as ilp32/lp64, i.e. not using hardware floating point.

@bors
Copy link
Contributor

bors commented Aug 7, 2023

☔ The latest upstream changes (presumably #114569) made this pull request unmergeable. Please resolve the merge conflicts.

@Nilstrieb
Copy link
Member

sorry for the late response, I've been away
r? compiler

@rustbot rustbot assigned compiler-errors and unassigned Nilstrieb Aug 17, 2023
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 20, 2023

📌 Commit 2f68d97 has been approved by compiler-errors

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 Aug 20, 2023
@bors
Copy link
Contributor

bors commented Aug 20, 2023

⌛ Testing commit 2f68d97 with merge 484cb4e...

@bors
Copy link
Contributor

bors commented Aug 20, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 484cb4e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 20, 2023
@bors bors merged commit 484cb4e into rust-lang:master Aug 20, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 20, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (484cb4e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [1.2%, 5.0%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.3% [-5.6%, -1.2%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -4.3% [-5.6%, -1.2%] 7

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.189s -> 635.452s (0.04%)
Artifact size: 347.03 MiB -> 347.05 MiB (0.01%)

@nbdd0121 nbdd0121 deleted the riscv branch August 25, 2023 12:17
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 24, 2023
Replace `HashMap` with `IndexMap` in pattern binding resolve

fixes rust-lang#114332 (comment)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2023
Replace `HashMap` with `IndexMap` in pattern binding resolve

fixes rust-lang#114332 (comment)
tmandry added a commit to tmandry/rust that referenced this pull request Oct 2, 2023
Replace `HashMap` with `IndexMap` in pattern binding resolve

fixes rust-lang#114332 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2023
Rollup merge of rust-lang#114454 - Nilstrieb:no-evil-sorting, r=cjgillot

Replace `HashMap` with `IndexMap` in pattern binding resolve

fixes rust-lang#114332 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of RiscV "+forced-atomics" target feature triggers hard-float ABI for symbols.o
8 participants