Sync x86 chkstk intrinsics with LLVM #575

kleisauke · 2024-02-10T14:14:59Z

Incorporates the following commits:
llvm/llvm-project@885d7b7
llvm/llvm-project@1f9eff1
llvm/llvm-project@7a5cba8

kleisauke · 2024-02-10T14:15:49Z

For reference, this resolves this linker failure on i686-pc-windows-gnullvm with LLVM 18.1.0-rc2:

ld.lld: error: undefined symbol: ___chkstk
>>> referenced by librsvg_c_api.a(compiler_builtins-3a76121a5ec728e8.compiler_builtins.3baf6562b232e6be-cgu.0.rcgu.o):(.weak.___alloca.default.__ZN17compiler_builtins3int6addsub7UAddSub4uadd17hff4d6b09d2fec294E)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

/cc @mati865 FYI.

mati865 · 2024-02-10T14:37:54Z

Thank, looks good at the quick glance but I don't understand why it'd fail with rc2 (unless it's rc2 with llvm/llvm-project@7a5cba8 backported).

kleisauke · 2024-02-10T15:00:14Z

Commit llvm/llvm-project@248aeac was cherry-picked/backported as llvm/llvm-project@7a5cba8 in LLVM 18.1.0-rc2.

However, it's likely that LLVM 18.1.0-rc1 would also encounter the same linker failure on i386, since commit llvm/llvm-project@885d7b7 removed the misspelled form of the MSVC _chkstk function, with three underscores instead of two.

mati865 · 2024-02-10T16:13:42Z

Oh, GitHub for some reason didn't show this commit belongs to rc2 tag and I assumed it'll be part of rc3. Makes sense now.

mati865 · 2024-03-23T09:01:48Z

CI should be fixed in today's nightly: rust-lang/rust#121552

Can you somehow trigger the build?

kleisauke · 2024-03-23T10:27:16Z

Great! I just triggered the build via:

$ git rebase --force-rebase upstream/master

kleisauke · 2024-03-23T10:31:52Z

... it looks like this is blocked on #582.

Also, fix 0011 patch to Cargo.lock (it was adding the embed-manifest dependency to the wrong crate due to fuzziness), and add a patch to vendor embed-manifest. This allows CLANG32 to build with --enable-vendor, which is required to use our now-patched version of compiler-builtins.

Amanieu · 2024-03-28T10:23:00Z

CI should be fixed now, can you rebase?

Also, fix 0011 patch to Cargo.lock (it was adding the embed-manifest dependency to the wrong crate due to fuzziness), and add a patch to vendor embed-manifest. This allows CLANG32 to build with --enable-vendor, which is required to use our now-patched version of compiler-builtins.

Amanieu · 2024-03-29T10:37:43Z

src/x86.rs

-        );
-    }
-
-    // FIXME: __alloca should be an alias to __chkstk
    #[naked]
    #[cfg(all(
        windows,
        target_env = "gnu",
        not(feature = "no-asm")
    ))]
    pub unsafe extern "C" fn __alloca() {


Are you sure that this is correct? rustc's name mangling will add another underscore in front of this function, so the final symbol will be ___alloca.

Good point, this should of course be _alloca. 🤦‍♂️

Commit d32d5ee fixes this.

Incorporates the following commits: llvm/llvm-project@885d7b7 llvm/llvm-project@1f9eff1 llvm/llvm-project@7a5cba8

jeremyd2019 · 2024-03-30T20:19:50Z

This seems to have broken MSYS2's build of i686-pc-windows-gnu (with gcc):

    = note: D:/M/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.2.0/../../../../i686-w64-mingw32/bin/ld.exe: D:/M/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.2.0/libgcc.a(_chkstk.o):(.text+0x0): multiple definition of `_alloca'; C:\_\B\src\MINGW32\build\i686-pc-windows-gnu\stage0-sysroot\lib\rustlib\i686-pc-windows-gnu\lib/libstd-394a669d59370585.dll.a(std_394a669d59370585_dll_d001695.o):(.text+0x0): first defined here

We needed to backport this change to make i686-pc-windows-gnullvm (with clang) work, because of an error linking to ___chkstk, but the latest update renaming to _alloca has caused this error with gcc.

kleisauke · 2024-03-31T11:24:09Z

@jeremyd2019 Ah, bummer. PR #452 ensures that each intrinsic is compiled into its own separate object file, which would avoid duplicate symbol definition errors. However, IIUC, this approach would only work if _alloca is also placed in a separate object file within libgcc.a, which does not appear to be the case:

$ ar t i686-w64-mingw32/lib/gcc/i686-w64-mingw32/13.2.0/libgcc.a | grep -E "chkstk|alloca"
_chkstk.o
_chkstk_ms.o

Perhaps adding _alloca to LIB1ASMFUNCS here would fix this?:
https://github.com/gcc-mirror/gcc/blob/releases/gcc-13.2.0/libgcc/config/i386/t-chkstk#L2
(untested)

Otherwise, I think marking _alloca as a weak symbol (with the #[weak] attribute) would fix this as well.

jeremyd2019 · 2024-03-31T16:36:27Z

@jeremyd2019 Ah, bummer. PR #452 ensures that each intrinsic is compiled into its own separate object file, which would avoid duplicate symbol definition errors. However, IIUC, this approach would only work if _alloca is also placed in a separate object file within libgcc.a, which does not appear to be the case

I just applied the changes from this PR on top of whatever version of compiler_builtins was vendored into rust 1.77.1. I can try to backport that PR as well, if it hasn't already been included in that version. UPDATE: it does appear to already be present, at least _alloca is wrapped in intrinsics!

Perhaps adding _alloca to LIB1ASMFUNCS here would fix this?: https://github.com/gcc-mirror/gcc/blob/releases/gcc-13.2.0/libgcc/config/i386/t-chkstk#L2 (untested)

Hmm, I think there are a lot of GCC versions out there, and requiring a new one (or a patched one) may not be viable.

Otherwise, I think marking _alloca as a weak symbol (with the #[weak] attribute) would fix this as well.

Maybe. Any time I see "weak" symbols it scares me now, we have had a lot of bugs/issues with weak symbols in gcc/binutils on Windows. I think this case would be fine though, the issues had to do with shared libraries IIRC. If the PR you referenced is already present, I can try slapping #[weak] on _alloca and seeing what happens. (I don't know much rust, so I'm trusting your syntax 😁)

As a procedure question, should I have opened a new issue on this rather than commenting on the PR? I usually like to comment on the PR in cases like this so I know that all the relevant people (including the author) are subscribed to it.

jeremyd2019 · 2024-03-31T16:55:30Z

I guess I missed something:

error: cannot find attribute `weak` in this scope
  --> D:\M\mingw-w64-rust\src\rustc-1.77.1-src\vendor\compiler_builtins\src\x86.rs:12:7
   |
12 |     #[weak]
   |       ^^^^

bjorn3 · 2024-03-31T16:58:00Z

It would have to be #[linkage = "weak"] except if you are inside an intrinsics! {} block, in that case #[weak] is the correct attribute. (intrinsics! translates #[weak] to #[linkage = "weak"] except making sure that it doesn't get duplicated #[linkage = "weak"] if the weak-intrinsics feature is enabled)

jeremyd2019 · 2024-03-31T16:59:48Z

It was inside an intrinsics! block, but #[linkage = "weak"] seems to be going further.

jeremyd2019 · 2024-03-31T17:02:37Z

Nope:

  = note: D:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.2.0/../../../../i686-w64-mingw32/bin/ld.exe: D:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.2.0/libgcc.a(_chkstk.o):(.text+0x0): multiple definition of `_alloca'; D:\M\mingw-w64-rust\src\MINGW32\build\i686-pc-windows-gnu\stage0-sysroot\lib\rustlib\i686-pc-windows-gnu\lib/libstd-394a669d59370585.dll.a(std_394a669d59370585_dll_d001695.o):(.text+0x0): first defined here
          collect2.exe: error: ld returned 1 exit status

I noticed that there is a dll import library involved after all. I don't know if/how weak symbols work there, especially with binutils (as opposed to lld, which seems to handle weak symbols on Windows better).

jeremyd2019 · 2024-03-31T17:32:35Z

At least for us, everything worked while the function was called __alloca rather than _alloca. Did renaming it cause some other previously-broken scenario to start working? If it was never called because it had the "wrong" name, perhaps it is not needed at all? It was removed from x86_64.rs after all.

Let me restate in terms of msys2/MINGW-packages#20397:

as shipped, rust 1.77.1 fails to build in CLANG32 (think i686-pc-windows-gnullvm) due to note: ld.lld: error: undefined symbol: ___chkstk
if the version of this PR from 67c1c0a is patched into the version of compiler_builtins vendored, everything works
if the version of this PR from d32d5ee (which is the version that was merged) is patched into the version of compiler_builtins vendored, MINGW32 (think i686-pc-windows-gnu) fails with = note: D:/M/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.2.0/../../../../i686-w64-mingw32/bin/ld.exe: D:/M/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.2.0/libgcc.a(_chkstk.o):(.text+0x0): multiple definition of `_alloca'; C:\_\B\src\MINGW32\build\i686-pc-windows-gnu\stage0-sysroot\lib\rustlib\i686-pc-windows-gnu\lib/libstd-394a669d59370585.dll.a(std_394a669d59370585_dll_d001695.o):(.text+0x0): first defined here

kleisauke · 2024-03-31T18:04:55Z

UPDATE: it does appear to already be present, at least _alloca is wrapped in intrinsics!

Yes exactly, sorry for the confusion.

I think there are a lot of GCC versions out there, and requiring a new one (or a patched one) may not be viable.

Indeed, marking _alloca as a weak symbol is probably better.

error: cannot find attribute `weak` in this scope
  --> D:\M\mingw-w64-rust\src\rustc-1.77.1-src\vendor\compiler_builtins\src\x86.rs:12:7
   |
12 |     #[weak]
   |       ^^^^

I think adding #[weak] above #[naked] would make it work (i.e. on line 11 instead).

If it was never called because it had the "wrong" name, perhaps it is not needed at all? It was removed from x86_64.rs after all.

According to commit llvm/llvm-project@885d7b7 only the symbols named __alloca on i386 and ___chkstk_ms on x86_64 are utilized on MinGW; the remaining functions are not used and can lead to confusion. On i386, this is the raw symbol name - if we consider a C-level function name with the additional implicit leading underscore, it would be called _alloca.

If the #[weak] trick doesn't work and we can safely assume the i686-pc-windows-gnu target will always link against libgcc, then we could guard the _alloca symbol only for target_abi = "llvm" (i.e. making it only available for the i686-pc-windows-gnullvm target).

jeremyd2019 · 2024-03-31T18:10:48Z

error: cannot find attribute `weak` in this scope
  --> D:\M\mingw-w64-rust\src\rustc-1.77.1-src\vendor\compiler_builtins\src\x86.rs:12:7
   |
12 |     #[weak]
   |       ^^^^
I think adding #[weak] above #[naked] would make it work (i.e. on line 11 instead).

I will try that, but don't hold out much hope of it working because there is in fact a dll import library involved. As I recall, Windows PE format does not have weak shared library symbols as ie ELF does, so this cannot really be expected to work. Perhaps @mstorsjo could elaborate?

If the #[weak] trick doesn't work and we can safely assume the i686-pc-windows-gnu target will always link against libgcc, then we could guard the _alloca symbol only for target_abi = "llvm" (i.e. making it only available for the i686-pc-windows-gnullvm target).

I don't know about that assumption. I only deal with rust where there is a C toolchain for the target in question already present. I assume rust supports situations where that's not the case (otherwise, what's the point of this crate? why not always just link against the intrinsics from the C toolchain?)

jeremyd2019 · 2024-03-31T18:17:02Z

I think adding #[weak] above #[naked] would make it work (i.e. on line 11 instead).

This compiles, but still fails linking with the same multiple definition of _alloca error as before.

mstorsjo · 2024-03-31T20:22:51Z

I will try that, but don't hold out much hope of it working because there is in fact a dll import library involved. As I recall, Windows PE format does not have weak shared library symbols as ie ELF does, so this cannot really be expected to work. Perhaps @mstorsjo could elaborate?

I didn't try to read up the whole discussion above to see which semantic of weak symbols this refers to.

For static linking, LLVM/lld (and to some extend also GCC/binutils, although it's buggy in some cases) does support weak symbols in COFF - you can have a weak undefined, which resolves as a null pointer if there's no definition of the symbol, or you can have multiple weak definitions and zero/one non-weak definitions.

This works for symbols within one module, but for symbols imported from a DLL, it gets resolved at link time, and if referenced, the symbol must exist at runtime when the module is loaded too, there's no weak behaviour left at runtime. (There's a mechanism for delay loading, but that just shifts the runtime cost of loading a DLL, it does not allow the symbol to be missing. And I guess this isn't the behaviour you're discussing above anyway.)

jeremyd2019 · 2024-03-31T20:59:14Z

That's how I understood weak symbols to work, that it meant at reference time. This discussion came about because of the idea that marking the function (one of the symbols) as weak would fix a multiple definition error at link time. That seems to be a different semantic of weak symbols that I was not familiar with.

jeremyd2019 · 2024-03-31T23:01:22Z

If the #[weak] trick doesn't work and we can safely assume the i686-pc-windows-gnu target will always link against libgcc, then we could guard the _alloca symbol only for target_abi = "llvm" (i.e. making it only available for the i686-pc-windows-gnullvm target).

This probably wouldn't help in our particular case, we are still patching rust to build the {i686,x86_64}-pc-windows-gnu targets but actually be -gnullvm (not sure if I explained that well). We cannot switch to not lie about the target yet, due to microsoft/windows-rs#2960

* rust: update to 1.77.1 * libc++: enable frame apis in libunwind. The bootstrapping hack used by the rust package on clang subsystems (namely, substituting compiler-rt and libunwind for libgcc and using the official gcc-based binaries to bootstrap) attempts to link to __register_frame_info and __deregister_frame_info on i686. Enabling this option brings them back. * rust: backport rust-lang/compiler-builtins#575 Also, fix 0011 patch to Cargo.lock (it was adding the embed-manifest dependency to the wrong crate due to fuzziness), and vendor embed-manifest. This allows CLANG32 to build with --enable-vendor, which is required to use our now-patched version of compiler-builtins. Note the 67c1c0a71a204c089ddae4aec21ec75aa778c11b commit was *not* the version merged upstream, but *is* the version that does not fail on either MINGW32 or CLANG32. * rust: enable uac patch for all clang prefixes. It turns out to be required on i686 *and* aarch64, but shouldn't hurt on x86_64 either. --------- Co-authored-by: Jeremy Drake <[email protected]>

mati865 approved these changes Feb 12, 2024

View reviewed changes

This was referenced Mar 18, 2024

Provide prebuilt std for gnullvm targets rust-lang/rust#121712

Merged

rust: update to 1.77.1 msys2/MINGW-packages#20397

Merged

kleisauke force-pushed the sync-chkstk branch from fee6641 to f201441 Compare March 23, 2024 10:25

kleisauke force-pushed the sync-chkstk branch from f201441 to 67c1c0a Compare March 28, 2024 10:25

Amanieu reviewed Mar 29, 2024

View reviewed changes

Sync x86 chkstk intrinsics with LLVM

d32d5ee

Incorporates the following commits: llvm/llvm-project@885d7b7 llvm/llvm-project@1f9eff1 llvm/llvm-project@7a5cba8

kleisauke force-pushed the sync-chkstk branch from 67c1c0a to d32d5ee Compare March 29, 2024 18:52

Amanieu merged commit d8ab794 into rust-lang:master Mar 30, 2024
22 checks passed

jeremyd2019 mentioned this pull request Apr 3, 2024

Regression: build failure on i686-pc-windows-gnu due to multiple definition of _alloca #585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync x86 chkstk intrinsics with LLVM #575

Sync x86 chkstk intrinsics with LLVM #575

kleisauke commented Feb 10, 2024

kleisauke commented Feb 10, 2024

mati865 commented Feb 10, 2024

kleisauke commented Feb 10, 2024

mati865 commented Feb 10, 2024

mati865 commented Mar 23, 2024

kleisauke commented Mar 23, 2024

kleisauke commented Mar 23, 2024

Amanieu commented Mar 28, 2024

Amanieu Mar 29, 2024

kleisauke Mar 29, 2024

jeremyd2019 commented Mar 30, 2024 •

edited

Loading

kleisauke commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024

bjorn3 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

kleisauke commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024

mstorsjo commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024

Sync x86 chkstk intrinsics with LLVM #575

Sync x86 chkstk intrinsics with LLVM #575

Conversation

kleisauke commented Feb 10, 2024

kleisauke commented Feb 10, 2024

mati865 commented Feb 10, 2024

kleisauke commented Feb 10, 2024

mati865 commented Feb 10, 2024

mati865 commented Mar 23, 2024

kleisauke commented Mar 23, 2024

kleisauke commented Mar 23, 2024

Amanieu commented Mar 28, 2024

Amanieu Mar 29, 2024

Choose a reason for hiding this comment

kleisauke Mar 29, 2024

Choose a reason for hiding this comment

jeremyd2019 commented Mar 30, 2024 • edited Loading

kleisauke commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024 • edited Loading

jeremyd2019 commented Mar 31, 2024

bjorn3 commented Mar 31, 2024 • edited Loading

jeremyd2019 commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024 • edited Loading

jeremyd2019 commented Mar 31, 2024 • edited Loading

kleisauke commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024

mstorsjo commented Mar 31, 2024

jeremyd2019 commented Mar 31, 2024 • edited Loading

jeremyd2019 commented Mar 31, 2024

jeremyd2019 commented Mar 30, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

bjorn3 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024 •

edited

Loading

jeremyd2019 commented Mar 31, 2024 •

edited

Loading