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

Set the default ABI to C for extern blocks and extern functions #2806

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

nobel-sh
Copy link
Contributor

Previously, the default ABI was set to Rust, which is incorrect for extern blocks and extern functions. This patch changes the default ABI to C for these cases.

Reference: https://doc.rust-lang.org/reference/items/functions.html#extern-function-qualifier
Zulip discussion: https://gcc-rust.zulipchat.com/#narrow/stream/266897-general/topic/extern.20blocks/near/346429045

gcc/rust/ChangeLog:

* hir/rust-ast-lower-base.cc (ASTLoweringBase::lower_qualifiers): (ASTLoweringBase::lower_extern_block): Change ABI to C for extern

Previously, the default ABI was set to Rust, which is not correct for
extern blocks and extern functions. This patch changes the default
ABI to C for these cases.

gcc/rust/ChangeLog:

	* hir/rust-ast-lower-base.cc (ASTLoweringBase::lower_qualifiers):
	Change default ABI to C for extern functions
	(ASTLoweringBase::lower_extern_block): Likewise

Signed-off-by: Nobel Singh <[email protected]>
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

LGTM, waiting on @philberty as he owns that part of the code. thanks for looking into this :)

@CohenArthur CohenArthur requested a review from philberty January 26, 2024 10:29
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

Is this the right thing? iirc we can do:

extern {
}

I assumed this would default to Rust ABI. Do you have a reference for this it would be good to capture that.

Otherwise the code looks good.

@philberty
Copy link
Member

apologies i see it now:
"Just as with external block, when the extern keyword is used and the "ABI" is omitted, the ABI used defaults to "C". That is, this:"

Thanks for linking this.

@philberty philberty added this pull request to the merge queue Feb 3, 2024
Merged via the queue into Rust-GCC:master with commit 7c0daba Feb 3, 2024
9 checks passed
@nobel-sh nobel-sh deleted the change_default_abi branch February 3, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants