Skip to content

Mark all functions as DECL_PUBLIC such that they can be linked against. #137

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 1 commit into from
Jan 11, 2021

Conversation

philberty
Copy link
Member

This will change when we implement visibility but the main method will be reserved case that will always be DECL_PUBLC.

Fixes #136

@bjorn3
Copy link

bjorn3 commented Jan 10, 2021

Generic and #[inline] functions must never be public as there could be multiple instances of them in the same crate graph.

As for the linker error in #136, rustc always defines the main shim in the same codegen unit as the rust main function.

@philberty philberty force-pushed the phil/main_link_error branch from df04a07 to c1f4160 Compare January 10, 2021 18:12
@philberty
Copy link
Member Author

Generic and #[inline] functions must never be public as there could be multiple instances of them in the same crate graph.

As for the linker error in #136, rustc always defines the main shim in the same codegen unit as the rust main function.

Thanks for the review, I have changed this to only apply to the main function and any function with pub visibility. There will be changes to this later on when we start working on the cases you mention. I think this is a middle ground for now to fix the bug.

flags |= Backend::function_is_visible;

// FIXME need name mangling
std::string asm_name = function.function_name;
Copy link
Contributor

Choose a reason for hiding this comment

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

The main function doesn't need the name mangling. I mention it here in case you miss it.

Copy link

Choose a reason for hiding this comment

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

The rust main function needs symbol mangling. Only the main shim doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, I see.
In Rust, the main function will be mangling first, then connect to the main shim. And the main shim was generated as a convention automatically.
This is different from C/C++, the main function is always the default symbol linked to _start defined in the linker script.

Thanks for correcting me! @bjorn3

Copy link

Choose a reason for hiding this comment

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

Yeah, libstd does some extra work in addition to what the libc startup does like setting up a stack guard, adding a handler for SIGSEGV to give a nice error message on stack overflows, storing the program arguments in a static and running the main function within std::panic::catch_unwind to ensure that panicking doesn't immediately abort due to a missing unwind catcher. For reference the main shim calls https://github.com/rust-lang/rust/blob/a2cd91ceb0f156cb442d75e12dc77c3d064cdde4/library/std/src/rt.rs#L60 with the main function as first argument when using libstd. There are also a few other (unstable) options like marking a function with #[start], in which case argc and argv are directly passed to that function without any additional arguments: https://github.com/bjorn3/rustc_codegen_cranelift/blob/3ea8915d4a247b5b3c4cfb3424c230ccd2645b17/example/alloc_example.rs#L30

Copy link

Choose a reason for hiding this comment

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

Should I open an issue for handling the main shim?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I open an issue for handling the main shim?

Yes @bjorn3 that would be great can you reference this PR in the issue too.

Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for that @bjorn3

Copy link
Member

@SimplyTheOther SimplyTheOther left a comment

Choose a reason for hiding this comment

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

PR looks great.

This change will need more thought later when it comes to traits and
generics etc.

Fixes #136
@philberty philberty force-pushed the phil/main_link_error branch from c1f4160 to 1c72678 Compare January 11, 2021 17:32
@bjorn3 bjorn3 mentioned this pull request Jan 11, 2021
@philberty philberty merged commit 5a9ceba into master Jan 11, 2021
@philberty philberty deleted the phil/main_link_error branch January 12, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link error for main on Linux x86
4 participants