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

[hexagon] Remove aliases w/o leading __ #682

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

androm3da
Copy link
Contributor

These hexagon builtins incorrectly created aliases in the global namespace which can (and in at least one case, did) conflict with symbols defined by other programs.

This should address the issue reported as rust-lang/rust#129823:

   Compiling compiler_builtins v0.1.123
   Compiling core v0.0.0 (/home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core)
   Compiling rustc-std-workspace-core v1.99.0 (/home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rustc-std-workspace-core)
   Compiling byteorder v1.5.0
   Compiling zerocopy v0.7.34
error: symbol 'fma' is already defined

error: could not compile `compiler_builtins` (lib) due to 1 previous error

Also: some of the symbols defined were not just aliases, so those are now qualified with __hexagon_. The compiler does not yet emit calls to these particular ones, but if/when it does, it can use the new names.

These hexagon builtins incorrectly created aliases in the global
namespace which can (and in at least one case, did) conflict
with symbols defined by other programs.

This should address the issue reported as rust-lang/rust#129823:

	   Compiling compiler_builtins v0.1.123
	   Compiling core v0.0.0 (/home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core)
	   Compiling rustc-std-workspace-core v1.99.0 (/home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rustc-std-workspace-core)
	   Compiling byteorder v1.5.0
	   Compiling zerocopy v0.7.34
	error: symbol 'fma' is already defined

	error: could not compile `compiler_builtins` (lib) due to 1 previous error

Also: some of the symbols defined were not just aliases, so those are
now qualified with `__hexagon_`.  The compiler does not yet emit calls
to these particular ones, but if/when it does, it can use the new names.
@androm3da
Copy link
Contributor Author

@tgross35 or @Amanieu are you willing to merge this change?

@tgross35
Copy link
Contributor

tgross35 commented Sep 1, 2024

Does changing the symbols from .global to .weak resolve this issue without losing whatever benefit there is to having them in the first place? That is effectively what we do for the symbols on other platforms.

@tgross35 or @Amanieu are you willing to merge this change?

Fyi we get pinged on new PRs already, just need to allow some time for reviews (this PR has only been open 8 hours).

@androm3da
Copy link
Contributor Author

It would, but it was an error to define them that way at all - I don't believe we ever had calls to the unqualified symbols. Probably just an artifact from how it was originally written and tested.

benefit there is to having them in the first place

I don't think there is any.

Fyi we get pinged on new PRs already

Ok, my mistake. I'm not in any hurry to get this merged, just wanted to avoid it falling through the cracks. Good to know it wouldn't have.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks for confirming!

@tgross35 tgross35 merged commit a2f9bcd into rust-lang:master Sep 1, 2024
24 checks passed
@tgross35
Copy link
Contributor

tgross35 commented Sep 1, 2024

This change will be in 0.1.124, you can put up a PR to rust-lang/rust to update compiler-builtins after #683 merges.

@androm3da
Copy link
Contributor Author

you can put up a PR to rust-lang/rust to update compiler-builtins after #683 merges.

Ok - will, do. Thanks!

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 5, 2024
This commit updates the compiler-builtins crate from 0.1.123 to 0.1.125.
The changes in this update are:

* rust-lang/compiler-builtins#682
* rust-lang/compiler-builtins#678
* rust-lang/compiler-builtins#685
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…ins, r=tgross35

Update compiler-builtins to 0.1.125

This commit updates the compiler-builtins crate from 0.1.123 to 0.1.125. The changes in this update are:

* rust-lang/compiler-builtins#682
* rust-lang/compiler-builtins#678
* rust-lang/compiler-builtins#685

Fixes: rust-lang#129823
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…s, r=<try>

Update compiler-builtins to 0.1.125

This commit updates the compiler-builtins crate from 0.1.123 to 0.1.125. The changes in this update are:

* rust-lang/compiler-builtins#682
* rust-lang/compiler-builtins#678
* rust-lang/compiler-builtins#685

try-job: aarch64-apple
try-job: aarch64-gnu
try-job: armhf-gnu
try-job: test-various
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2024
…ins, r=tgross35

Update compiler-builtins to 0.1.125

This commit updates the compiler-builtins crate from 0.1.123 to 0.1.125. The changes in this update are:

* rust-lang/compiler-builtins#682
* rust-lang/compiler-builtins#678
* rust-lang/compiler-builtins#685
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
…s, r=tgross35

Update compiler-builtins to 0.1.125

This commit updates the compiler-builtins crate from 0.1.123 to 0.1.125. The changes in this update are:

* rust-lang/compiler-builtins#682
* rust-lang/compiler-builtins#678
* rust-lang/compiler-builtins#685
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 14, 2024
…ss35

Update compiler-builtins to 0.1.125

This commit updates the compiler-builtins crate from 0.1.123 to 0.1.125. The changes in this update are:

* rust-lang/compiler-builtins#682
* rust-lang/compiler-builtins#678
* rust-lang/compiler-builtins#685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants