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.

Sign up for GitHub

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

asm/arm: allow r9 when usable, make diagnostics more specific #88879

Closed
wants to merge 1 commit into from
Closed

asm/arm: allow r9 when usable, make diagnostics more specific #88879

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions compiler/rustc_target/src/asm/arm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{InlineAsmArch, InlineAsmType};
use crate::spec::Target;
use crate::spec::{RelocModel, Target};
use rustc_macros::HashStable_Generic;
use std::fmt;

Expand Down Expand Up @@ -88,6 +88,18 @@ fn frame_pointer_r7(
}
}

fn rwpi_enabled(
_arch: InlineAsmArch,
_has_feature: impl FnMut(&str) -> bool,
target: &Target,
) -> Result<(), &'static str> {
if let RelocModel::Rwpi | RelocModel::RopiRwpi = target.relocation_model {
Copy link
Member

Choose a reason for hiding this comment

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

Should we only allow r9 when the relocation model was explicitly given to allow for more freedom in choosing which relocation model will be used by default without risking breaking this?

Copy link
Author

Choose a reason for hiding this comment

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

That might be a good idea, though it would make the diagnostic less clear if the user hasn't specifically opted in to that mode. I wanted to add a "note" to it, but this is my first time contributing to rustc and I don't know how to do that, much less how to make it work in the context of def_regs!.

How does one check if a target attribute is explicitly set?

Copy link
Member

Choose a reason for hiding this comment

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

You can check the value of sess.opts.cg.relocation_model. By the way I think target.relocation_model is only the default relocation model in all cases and not the one that will actually be used when compiling if -Crelocation-model is used.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, I didn't think of that since in my use case I'm using a custom target. I can fix that up. I'd still like to improve the diagnostics though.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, Session already has a relocation_model getter that does the logic I was going to write anyway.

Copy link
Author

@Skirmisher Skirmisher Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah right, I can't actually do that because the asm parser isn't passed the Session, only the Target... Looks like I would need to change the definition of parse in the entire rustc_target::asm module at least, including arch-specific filter functions, then alter rustc_ast_lowering::LoweringContext to pass Session as well (possibly in place of Target since it can be accessed through Session). That feels like a fairly invasive change just to make a small thing more correct, but the overall effect should be negligible. If the maintainers are okay with it, I can go ahead and do it.

Copy link
Member

Choose a reason for hiding this comment

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

This won't work since rustc_target can't depend on rustc_session since that would create a circular dependency.

Err("the RWPI static base register (r9) cannot be used as an operand for inline asm")
} else {
Ok(())
}
}

def_regs! {
Arm ArmInlineAsmReg ArmInlineAsmRegClass {
r0: reg, reg_thumb = ["r0", "a1"],
Expand All @@ -98,6 +110,7 @@ def_regs! {
r5: reg, reg_thumb = ["r5", "v2"],
r7: reg, reg_thumb = ["r7", "v4"] % frame_pointer_r7,
r8: reg = ["r8", "v5"],
r9: reg = ["r9", "v6", "rfp"] % rwpi_enabled,
r10: reg = ["r10", "sl"],
r11: reg = ["r11", "fp"] % frame_pointer_r11,
r12: reg = ["r12", "ip"],
Expand Down Expand Up @@ -183,9 +196,7 @@ def_regs! {
q14: qreg = ["q14"],
q15: qreg = ["q15"],
#error = ["r6", "v3"] =>
"r6 is used internally by LLVM and cannot be used as an operand for inline asm",
#error = ["r9", "v6", "rfp"] =>
"r9 is used internally by LLVM and cannot be used as an operand for inline asm",
"the base pointer (r6) cannot be used as an operand for inline asm",
#error = ["r13", "sp"] =>
"the stack pointer cannot be used as an operand for inline asm",
#error = ["r15", "pc"] =>
Expand Down