Skip to content

Cleanup fix for global initialization #93

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

Merged
merged 4 commits into from
Sep 27, 2021
Merged

Cleanup fix for global initialization #93

merged 4 commits into from
Sep 27, 2021

Conversation

antoyo
Copy link
Contributor

@antoyo antoyo commented Sep 26, 2021

No description provided.

@antoyo
Copy link
Contributor Author

antoyo commented Sep 26, 2021

Removing the linker script hack makes many tests fail because they can't find some symbols:

2021-09-26T18:43:00.0680157Z            /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o: in function `__LT__RF_T_u20_as_u20_core__fmt__Debug_GT_::fmt':
2021-09-26T18:43:00.0681711Z            fake.c:(.text+0x1ac): undefined reference to `core::fmt::num::__LT_impl_u20_core__fmt__UpperHex_u20_for_u20_i32_GT_::fmt'
2021-09-26T18:43:00.0682896Z            /usr/bin/ld: fake.c:(.text+0x1c7): undefined reference to `core::fmt::num::__LT_impl_u20_core__fmt__LowerHex_u20_for_u20_i32_GT_::fmt'
2021-09-26T18:43:00.0684089Z            /usr/bin/ld: fake.c:(.text+0x1e7): undefined reference to `core::fmt::num::imp::__LT_impl_u20_core__fmt__Display_u20_for_u20_i32_GT_::fmt'
2021-09-26T18:43:00.0685878Z            /usr/bin/ld: /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o: in function `__LT__RF_T_u20_as_u20_core__fmt__Debug_GT_::fmt':
2021-09-26T18:43:00.0687705Z            fake.c:(.text+0x2bc): undefined reference to `core::fmt::num::__LT_impl_u20_core__fmt__UpperHex_u20_for_u20_usize_GT_::fmt'
2021-09-26T18:43:00.0688967Z            /usr/bin/ld: fake.c:(.text+0x2d7): undefined reference to `core::fmt::num::__LT_impl_u20_core__fmt__LowerHex_u20_for_u20_usize_GT_::fmt'
2021-09-26T18:43:00.0690336Z            /usr/bin/ld: fake.c:(.text+0x2f7): undefined reference to `core::fmt::num::imp::__LT_impl_u20_core__fmt__Display_u20_for_u20_usize_GT_::fmt'
2021-09-26T18:43:00.0692145Z            /usr/bin/ld: /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o: in function `__LT__RF_T_u20_as_u20_core__fmt__Debug_GT_::fmt':
2021-09-26T18:43:00.0693875Z            fake.c:(.text+0x33c): undefined reference to `core::fmt::num::__LT_impl_u20_core__fmt__UpperHex_u20_for_u20_u8_GT_::fmt'
2021-09-26T18:43:00.0695096Z            /usr/bin/ld: fake.c:(.text+0x357): undefined reference to `core::fmt::num::__LT_impl_u20_core__fmt__LowerHex_u20_for_u20_u8_GT_::fmt'
2021-09-26T18:43:00.0696341Z            /usr/bin/ld: fake.c:(.text+0x377): undefined reference to `core::fmt::num::imp::__LT_impl_u20_core__fmt__Display_u20_for_u20_u8_GT_::fmt'
2021-09-26T18:43:00.0698575Z            /usr/bin/ld: /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o: in function `__LT__RF_T_u20_as_u20_core__fmt__Debug_GT_::fmt':
2021-09-26T18:43:00.0700333Z            fake.c:(.text+0x39e): undefined reference to `__LT_str_u20_as_u20_core__fmt__Debug_GT_::fmt'
2021-09-26T18:43:00.0702029Z            /usr/bin/ld: /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o: in function `__LT__RF_T_u20_as_u20_core__fmt__Debug_GT_::fmt':
2021-09-26T18:43:00.0703520Z            fake.c:(.text+0xa02): undefined reference to `__LT_str_u20_as_u20_core__fmt__Debug_GT_::fmt'
2021-09-26T18:43:00.0705583Z            /usr/bin/ld: /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o:(.data.rel+0x1338): undefined reference to `__LT_core__alloc__layout__LayoutError_u20_as_u20_core__fmt__Debug_GT_::fmt'
2021-09-26T18:43:00.0708509Z            /usr/bin/ld: /home/runner/work/rustc_codegen_gcc/rustc_codegen_gcc/rust/build/x86_64-unknown-linux-gnu/test/ui/unsized/unsized-tuple-impls/a.unsized_tuple_impls.901a0c5d-cgu.0.rcgu.o:(.data.rel+0x1358): undefined reference to `__LT_std__thread__local__AccessError_u20_as_u20_core__fmt__Debug_GT_::fmt'
2021-09-26T18:43:00.0709746Z            collect2: error: ld returned 1 exit status

Is it because I do a different name mangling?

@bjorn3
Copy link
Member

bjorn3 commented Sep 26, 2021

You don't use tcx.symbol_name()? If so, then yes, the linker code expects that the names returned from tcx.symbol_name() are used when calculating the list of symbols to export from dylibs.

@antoyo
Copy link
Contributor Author

antoyo commented Sep 26, 2021

Some symbols are not currently supported in libgccjit so I replace them.

Hopefully, that's an easy fix in libgccjit ;) .

Thanks for confirming my intuition!

@bjorn3
Copy link
Member

bjorn3 commented Sep 26, 2021

Try -Zsymbol-mangling-version=v0.

@@ -171,7 +171,7 @@ git checkout src/test/ui/type-alias-impl-trait/auxiliary/cross_crate_ice.rs
git checkout src/test/ui/type-alias-impl-trait/auxiliary/cross_crate_ice2.rs
rm src/test/ui/llvm-asm/llvm-asm-in-out-operand.rs || true # TODO(antoyo): Enable back this test if I ever implement the llvm_asm! macro.

RUSTC_ARGS="-Zpanic-abort-tests -Zcodegen-backend="$(pwd)"/../target/"$CHANNEL"/librustc_codegen_gcc."$dylib_ext" --sysroot "$(pwd)"/../build_sysroot/sysroot -Cpanic=abort"
RUSTC_ARGS="-Zpanic-abort-tests -Zsymbol-mangling-version=v0 -Zcodegen-backend="$(pwd)"/../target/"$CHANNEL"/librustc_codegen_gcc."$dylib_ext" --sysroot "$(pwd)"/../build_sysroot/sysroot -Cpanic=abort"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this didn't affect sysroot building.

@antoyo antoyo force-pushed the cleanup/global-init branch from 176fa2b to 7280758 Compare September 26, 2021 20:46
@antoyo antoyo force-pushed the cleanup/global-init branch from 2d649b8 to 4f5761b Compare September 27, 2021 12:53
@antoyo antoyo merged commit ab4ff2d into master Sep 27, 2021
@antoyo antoyo deleted the cleanup/global-init branch September 27, 2021 13:34
@antoyo
Copy link
Contributor Author

antoyo commented Sep 30, 2021

@dkm Can you tell my if the switch to v0 symbol mangling is causing any issues for GodBolt? If so, I'll add support of the missing symbols in libgccjit to allow the usage of the previous symbol mangling (I'm just not sure if some of those symbols are unavailable on some targets).

@dkm
Copy link

dkm commented Sep 30, 2021

Not sure exactly what I should look for... ? Currently, on godbolt, only x86 is used, haven't tried to target another arch... Would be a good idea !

@antoyo
Copy link
Contributor Author

antoyo commented Sep 30, 2021

Not sure exactly what I should look for... ? Currently, on godbolt, only x86 is used, haven't tried to target another arch... Would be a good idea !

The name mangling is what turns a rust path like std::io::stdio::StderrLock::Write::write_fmt to something like _RNvYNtNtNtCshwLzeTMMd1B_3std2io5stdio10StderrLockNtB6_5Write9write_fmtB8_. This is using the new name mangling v0.

I was asking because I wondered if that would cause issues with GodBolt. I'm not sure if the tools (like objdump) currently support this new name mangling.

@dkm
Copy link

dkm commented Sep 30, 2021

Ok, I'll check that :)

@bjorn3
Copy link
Member

bjorn3 commented Sep 30, 2021

I think the v0 symbol mangling scheme will need to enabled on godbolt too.

@dkm
Copy link

dkm commented Oct 1, 2021

Screenshot 2021-10-01 at 21-11-30 Compiler Explorer

After a small test, seems that rustfilt is fresh enough on compiler-explorer to correctly handle v0 mangling. I could check with rustc_cg_gcc as I can give -Z..., but could not really verify with stable rust (could not find how to change mangling, if possible).

https://godbolt.org/z/Tqf5dPvaY

Do you want to have the v0 mangling forced on compiler-explorer ? I can add the -Z... option if you tell me to :)

note: in the above example, there's the example string that pops at the end of the mangled version of std::mem::align_of::<f64>, not really sure this is expected or something else. Didn't spend long enough trying to decode it by hand, so that may be normal and simply the file name being used here...

@antoyo
Copy link
Contributor Author

antoyo commented Oct 1, 2021

I'm not sure I understand this output, but shouldn't the name in the bottom-right window be unmangled?

Yes, you will have to use this mangling (with latest cg_gcc) because I removed the linker script hack, so the code won't link. I can fix the previous mangling if you would prefer to keep it.

I believe it is expected to have a suffix with the crate name.

@dkm
Copy link

dkm commented Oct 1, 2021 via email

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.

3 participants