RFC: unsafe global_asm

[inkreasing]

View all comments

Require unsafe to use the global_asm macro, because it can overwrite symbols, which can lead to UB.

unsafe global_asm!("");

Important

Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.

If you don't have a particular section of the RFC to comment on, you can click on the "Comment on this file" button on the top-right corner of the diff, to the right of the "Viewed" checkbox. This will create a separate thread even if others have commented on the file too.

Rendered

[ds84182]

This could be an attribute #[unsafe] applied to the macro invocation, similar to #[unsafe(no_mangle)] on fns. It wouldn't require changes to the grammar as attributes can already precede macro items. https://doc.rust-lang.org/reference/grammar.html#railroad-summary-Item

EDIT: Missed the bullet point in the alternatives section. Could you expand on why this is more preferable than the attribute?

View changes since the review

[ChayimFriedman2]

This was mentioned in the Zulip thread which discussed a lot of threads. I think it will be good to link that thread in the RFC.

[inkreasing]

I don't feel like an #[unsafe] attribute would be similar to #[unsafe(no_mangle)]. Wouldn't it also have been possible to require something like:

#[unsafe]
#[no_mangle]
fn my_write() {}

which i feel like is actually the equivalent to requiring an unsafe attribute here.

In general i don't have that strong opinions on the syntax, so i found it hard to really argue against the others in the RFC, but i have some arguments that may be helpful and i can try to include the most convincing in the rfc.

• I think "native" syntax is in general preferable to attributes.
• An unsafe attribute would "imply much generality". why can't it be used instead of the unsafe keyword when implementing an unsafe trait, why can't it be used above a block to make it an unsafe block.
• If unsafe macros are added the attribute solution is probably not enough anymore and then another change needs to be done.

I think it will be good to link that thread in the RFC.

Do you think so? i think in the syntax alternatives i mentioned every proposed alternative and i would favor summarizing arguments in favor or against syntax alternatives rather than linking to them.

[ChayimFriedman2]

Please read the Zulip thread (and link it). This was already discussed there.

[inkreasing]

I have read (and participated in) the zulip thread. I added an argument against #[unsafe]. I am not convinced that linking the zulip thread in the RFC is superior to summarizing arguments. If i have missed one let me know and i can add it.

[ChayimFriedman2]

Linking previous discussions (in "Prior Art") is important in RFCs even if you summarize them perfectly.

I have not compared the thread and the RFC closely, but you do for example not mention the argument @ds84182 mentioned here, nor the response (#[unsafe(...)] was for things that are already attributes).

[tgross35]

Drawback: it doesn't really change anything other than making soundness risks more obvious, debatably. Making everyone add a keyword isn't likely to make more or fewer people use global_asm, or change what's written in the macro.

View changes since the review

[inkreasing]

I don't think this is a drawback? To be clear i completely agree that this won't change how of how often people use global_asm. For that a safe alternative (which i don't think is possible) would be necessary. But i also don't think this should be a goal of this.

From a quick search on github i haven't seen a single safety comment on global_asm and that is probably not great.

[clarfonthey]

This is particularly compelling because the principles of unsafe can apply to items as well as code.

For example, outputting unsafe impls for traits or generating methods with #[unsafe(...)] attributes necessarily hides the unsafety from the call that introduces it, and in general it's desirable to make the unsafety of a macro call dependent on the contents, e.g. generating unsafe impl or safe impl depending on situations.

View changes since the review

[dlight]

Rust should have the concept of a macro being unsafe to call, regardless of whether the macro itself expands to unsafe calls or unsafe items. That is, we should have unsafe macro_rules!(), unsafe macro, and #[unsafe_macro] on procedural macros and custom derives, etc. Macros called as #[macro] would need to be called as #[unsafe(macro)], and for derives we could have something like #[derive(Debug, unsafe(Something), SomethingElse]. And on expression position, it would just mean the macro needs to go inside an unsafe { } block.

(Right now what exists is some hacks to force the user to wrap the macro call into an unsafe { } block, but I don't think that's as widely used as it should be, and it's only for expression position)

This leaves out, what's the syntax to put unsafe annotations on a top level mymacro! { .. } call that only defines items. unsafe mymacro! { .. } and #[unsafe] mymacro! { .. } both seems sensible.

[inkreasing]

I don't want to get into unsafe macros too much in this RFC. As i tried to argue in eb78e5e i don't think this closes any design space except for syntax.

[kennytm]

With this RFC's proposed syntax we would have 3 ways to consume/assert unsafety,

• unsafe { ... } for expressions
• #[unsafe(...)] for attributes
• unsafe ... for items (unsafe impl, unsafe extern "ABI" {} and now unsafe core::arch::global_asm!())¹

I think this suggests, if unsafe macros were to exist, they should be restricted to produce items only (unlike the suggestion in line 141), to avoid further confusion (whether we should write unsafe { x!() } vs unsafe x!()).

View changes since the review

Footnotes

1. unfortunately coincides with the syntax to define unsafety (unsafe fn, unsafe trait) ↩

[dlight]

I think that unsafe macros on expression position should just require being put inside an unsafe { .. } block.

[inkreasing]

I think the discussions about unsafe macros in expression positions and if unsafe macros are only allowed to produce items are out of scope for this RFC, since i don't think this limits any direction that could be taken.

eb78e5e

[clarfonthey]

So this is a very lukewarm drawback, but it is worth mentioning that, right now, the set of unsafeties in code can be determined with a simple fixed-string search, and this would technically break this in general for macros, although would work fine if global_asm! is the only macro that does this.

Right now, with rust-lang/rust#163781, the set of strings we need to search to find unsafe code (which has been formatted by rustfmt correctly) is:

• unsafe {
• unsafe impl
• #[unsafe(
• #![unsafe(

With just this change, we add:

• unsafe global_asm!

But in general, we add the regular expression:

• unsafe \u{XID_Start}\u{XID_Continue}*!

Note that this is only a lukewarm justification because the proposal for unsafe derives would require full name resolution and checking inside derive macros and in general we should probably not rely on this. Right now, tidy being based upon a simple search is extremely brittle, as shown by the fact that I'm just working on adding unsafe impls and attributes now.

But, it is worth noting that this syntax is unique enough in Rust to be weird. The fact that it doesn't matter whether you're in an unsafe block is noteworthy to point out, although I think it would need something like this in general since macros can trivially "break out" of unsafe blocks:

macro_rules! malicious {
    () => {
        fn hidden_scope(x: *const u32) -> u32 {
            // this is not covered by the containing `unsafe { ... }` block
            *x
        }
        hidden_scope()
    }
}

fn overt_scope() {
    unsafe {
        malicious!();
    }
}

View changes since the review

[ChayimFriedman2]

It's already not true because you need to consider comments and strings. The only way to do it reliably is to over-approximate and search for \bunsafe\b or parse the AST.

[inkreasing]

This doesn't change your argument but you also need to add unsafe extern to the list.

[inkreasing]

This may be a weak drawback, but it is one, so i added it to the list.

35adb44