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

Undefined external symbols from compiler_builtins #124042

Closed
fenhl opened this issue Apr 16, 2024 · 8 comments
Closed

Undefined external symbols from compiler_builtins #124042

fenhl opened this issue Apr 16, 2024 · 8 comments
Labels
A-linkage Area: linking into static, shared libraries and binaries O-MIPS Target: MIPS processors T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fenhl
Copy link
Contributor

fenhl commented Apr 16, 2024

I have a project where I'm experimenting with introducing Rust code into an existing codebase by compiling it into a static library for a custom target and linking it using armips. This process no longer works on the latest nightly because armips complains about undefined external symbols such as _rnvntcse1eqnti5u6g_17compiler_builtins4math5atan2.

A minimal example is at https://github.com/fenhl/rust-builtins-min which can be tested by running python build.py after installing armips (scoop install extras/armips on Windows, see https://github.com/Kingcom/armips for binaries and build instructions for other platforms).

I ran cargo-bisect-rustc on the original project where I ran into this (not the minimal example, hence the different script in the command line) and it points to #123719 as the commit that introduced the regression.

cargo-bisect-rustc output

searched nightlies: from nightly-2022-06-21 to nightly-2024-04-16
regressed nightly: nightly-2024-04-15
searched commit range: 0bf471f...0d8b334
regressed commit: 29b1207

bisected with cargo-bisect-rustc v0.6.8

Host triple: x86_64-pc-windows-msvc
Reproduce with:

cargo bisect-rustc --start=2022-06-21 -c rust-src --script python -- .\cargo-bisect-rustc-test.py
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 16, 2024
@Noratrieb
Copy link
Member

@Noratrieb
Copy link
Member

@Amjad50 @Amanieu

@Noratrieb Noratrieb added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-MIPS Target: MIPS processors and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 17, 2024
@Amjad50
Copy link
Contributor

Amjad50 commented Apr 17, 2024

Thanks for letting me know.

The changes in 0.1.109 do not work as expected, I'm working on a fix here rust-lang/compiler-builtins#594, I'll test it with this project.

Amjad50 added a commit to Amjad50/compiler-builtins that referenced this issue Apr 29, 2024
export all symbols as `weak` for non `windows`/`apple` targets.

Additionally fixes an issue when creating `weak` instrinsics:
Before this commit, generated code will be
```rust
#[linkage = "weak"]
pub extern "C" fn <name>(...) -> ... {
    // code...
}
pub mod <name> {
    #[linkage = "weak"]
    #[no_mangle]
    pub extern "C" fn <name>(...) -> ... {
        super::<name>(...)
    }
}
```

The issue is that there is 2 `weak` linkage, the first one is not required, and this is the cause for the bug in rust-lang/rust#124042.

Along with the remval of the `weak-intrinsics` feature, we fixed this issue as well.
Amjad50 added a commit to Amjad50/compiler-builtins that referenced this issue Apr 29, 2024
Removed the `weak-intrinsics` feature, so that
all functions will have the `weak` linkage attribute.

But this caused issues for `memset` (maybe other mem functions are related), so added a special feature for those `mem-weak-intrinsics`.

Mem functions won't have the `weak` attribute unless this feature is enabled.

Also this fixed the bug in
rust-lang/rust#124042.

Before this commit, generated code will be
```rust
pub extern "C" fn <name>(...) -> ... {
        // code...
}
pub mod <name> {
    #[linkage = "weak"]
    #[no_mangle]
    pub extern "C" fn <name>(...) -> ... {
        super::<name>(...)
    }
}
```

The issue is that there is 2 `weak` linkage, the first one is not required.
Along refactoring `weak` attributes, this was fixed.
@Amjad50
Copy link
Contributor

Amjad50 commented Apr 29, 2024

So after looking around and trying to fix the issue, the main issue is from armips, it doesn't handle weak symbols correctly

Added a quick small fix here, Kingcom/armips#242, but not sure if the maintainer is around anymore

Amjad50 added a commit to Amjad50/compiler-builtins that referenced this issue Apr 30, 2024
Removed the `weak-intrinsics` feature, so that all functions
will have the `weak` linkage attribute.

Also this fixed the bug in
rust-lang/rust#124042.

Before this commit, generated code will be
```rust
pub extern "C" fn <name>(...) -> ... {
        // code...
}
pub mod <name> {
    #[linkage = "weak"]
    #[no_mangle]
    pub extern "C" fn <name>(...) -> ... {
        super::<name>(...)
    }
}
```

The issue is that there is 2 `weak` linkage, the first one is not required.
Along refactoring `weak` attributes, this was fixed.
Amjad50 added a commit to Amjad50/compiler-builtins that referenced this issue Apr 30, 2024
Removed the `weak-intrinsics` feature, so that all functions
will have the `weak` linkage attribute.

Also this fixed the bug in
rust-lang/rust#124042.

Before this commit, generated code will be
```rust
pub extern "C" fn <name>(...) -> ... {
        // code...
}
pub mod <name> {
    #[linkage = "weak"]
    #[no_mangle]
    pub extern "C" fn <name>(...) -> ... {
        super::<name>(...)
    }
}
```

The issue is that there is 2 `weak` linkage, the first one is not required.
Along refactoring `weak` attributes, this was fixed.
Amjad50 added a commit to Amjad50/compiler-builtins that referenced this issue May 1, 2024
Removed the `weak-intrinsics` feature, so that all functions
will have the `weak` linkage attribute.

Also this fixed the bug in
rust-lang/rust#124042.

Before this commit, generated code will be
```rust
pub extern "C" fn <name>(...) -> ... {
        // code...
}
pub mod <name> {
    #[linkage = "weak"]
    #[no_mangle]
    pub extern "C" fn <name>(...) -> ... {
        super::<name>(...)
    }
}
```

The issue is that there is 2 `weak` linkage, the first one is not required.
Along refactoring `weak` attributes, this was fixed.
@Amjad50
Copy link
Contributor

Amjad50 commented May 1, 2024

So, the PR in armips is fixed, and now it can handle weak symbols.

Though, there appear to be another issue.

When running armips with the library from latest nightly, it gives an error that memset is not found.

Looking at the llvm-ir for compiler_builtins, it seems the function compiler_builtins::math::libm::rem_pio2_large::rem_pio2_large calls

call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(80) %iq, i8 0, i64 80, i1 false)

which is converted into memset function call for some reason.

There is a way to fix this, which is to add -Zbuild-std-features=compiler-builtins-mem flag which would add the memory builtin functions.

I'm not sure if that is the correct behavior of llvm or not. since the target in the example has "os" = "none".

This is a static library, so maybe its assuming memset will be available when linking any executable?

@bjorn3
Copy link
Member

bjorn3 commented May 1, 2024

memcpy, memmove and memset are expected to be present by LLVM. They are either provided by libc (if any) or compiler-builtins (on no_std targets) Compiler-builtins has a hard coded set of no_std targets. For any other you have to explicitly use -Zbuild-std-features=compiler-builtins-mem.

https://github.com/rust-lang/compiler-builtins/blob/2cbde5b9538e2398716510b16b1b2c3d60d6b649/build.rs#L26-L36

@Amjad50
Copy link
Contributor

Amjad50 commented May 1, 2024

Thanks @bjorn3, so I guess this is expected behavior.

I think then this ticket can be closed, the fix is to use the newer armips build.

@fenhl
Copy link
Contributor Author

fenhl commented May 1, 2024

Can confirm this is fixed by using the new armips and adding -Zbuild-std-features=compiler-builtins-mem to the cargo command line. Thanks everyone for the help!

@fenhl fenhl closed this as completed May 1, 2024
Amanieu pushed a commit to Amjad50/compiler-builtins that referenced this issue May 3, 2024
Removed the `weak-intrinsics` feature, so that all functions
will have the `weak` linkage attribute.

Also this fixed the bug in
rust-lang/rust#124042.

Before this commit, generated code will be
```rust
pub extern "C" fn <name>(...) -> ... {
        // code...
}
pub mod <name> {
    #[linkage = "weak"]
    #[no_mangle]
    pub extern "C" fn <name>(...) -> ... {
        super::<name>(...)
    }
}
```

The issue is that there is 2 `weak` linkage, the first one is not required.
Along refactoring `weak` attributes, this was fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries O-MIPS Target: MIPS processors T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants