rust-lang / compiler-team

A home for compiler team planning documents, meeting minutes, and other such things.
https://rust-lang.github.io/compiler-team/
Apache License 2.0
387 stars 69 forks source link

Change `-Z asm_comments` to `-Z verbose_asm`, stop stripping handwritten assembly comments #762

Closed tgross35 closed 4 months ago

tgross35 commented 5 months ago

Proposal

-Z asm-comments currently controls two things: stripping all comments from assembly (PreserveAsmComments), and LLVM emitting its own verbose comments (AsmVerbose). By default this option is set to false, so even handwritten comments get stripped:

use core::arch::asm;

pub fn foo() {
    unsafe {
        asm!("nop # hello");
        asm!("nop // world");
    }
}

The output doesn't have the "hello" and "world" comments anywhere:

example::foo::h61551f5681388f69:
        push    rax
        #APP

        nop

        #NO_APP
        #APP

        nop

        #NO_APP
        pop     rax
        ret

Link: https://rust.godbolt.org/z/jxdEWY6oG

There doesn't seem to be a good reason to strip handwritten assembly comments ever (within the string), since a user can just write Rust comments if they don't want something to show up in the binary. So, proposal:

  1. Rename the configuration flag to -Z verbose-asm, which will be consistent with Clang/GCC
  2. Change that config option to only control LLVM-generated comments (AsmVerbose)
  3. Change PreserveAsmComments to be always true
  4. Start an actual tracking issue so -Z verbose-asm might wind up stabilized one day

The flag was originally added at https://github.com/rust-lang/rust/pull/53290

See background discussion at https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Comments.20in.20.60asm!.60

Mentors or Reviewers

I can do the change

Process

The main points of the Major Change Process are as follows:

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

rustbot commented 5 months ago

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

 @rustbot concern reason-for-concern 
 <description of the concern> 

Concerns can be lifted with:

 @rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

Amanieu commented 5 months ago

Clang seems to preserve asm comments by default: https://github.com/llvm/llvm-project/blob/4232dd586b65c9301304cab2fb166d8df97c591a/clang/include/clang/Driver/Options.td#L3737-L3741

So there's no downside to just always doing this in rustc.

nikic commented 5 months ago

@rustbot second

apiraino commented 4 months ago

@rustbot label -final-comment-period +major-change-accepted