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

Allow rustdoc to handle asm! of foreign architectures #82838

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 6, 2021

This allows rustdoc to process code containing asm! for architectures other than the current one. Since this never reaches codegen, we just replace target-specific registers and register classes with a dummy one.

Fixes #82869

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 6, 2021

Why does this make it to rustdoc at all? Special casing it with actually_rustdoc doesn't seem right, rustdoc shouldn't need to see the asm! in the first place because it doesn't typeck bodies:

rust/src/librustdoc/core.rs

Lines 396 to 405 in 51748a8

// Certain queries assume that some checks were run elsewhere
// (see https://github.com/rust-lang/rust/pull/73566#issuecomment-656954425),
// so type-check everything other than function bodies in this crate before running lints.
// NOTE: this does not call `tcx.analysis()` so that we won't
// typeck function bodies or run the default rustc lints.
// (see `override_queries` in the `config`)
// HACK(jynelson) this calls an _extremely_ limited subset of `typeck`
// and might break if queries change their assumptions in the future.

Comment on lines 1245 to 1345
if self.sess.asm_arch.is_none() && !self.sess.opts.actually_rustdoc {
struct_span_err!(self.sess, sp, E0472, "asm! is unsupported on this target").emit();
}
if asm.options.contains(InlineAsmOptions::ATT_SYNTAX)
&& !matches!(
self.sess.asm_arch,
Some(asm::InlineAsmArch::X86 | asm::InlineAsmArch::X86_64)
)
&& !self.sess.opts.actually_rustdoc
{
self.sess
.struct_span_err(sp, "the `att_syntax` option is only supported on x86")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check done so early? I would expect it to be in codegen somewhere, or at least in MIR lowering.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 6, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 6, 2021

Also, I'm not sure how this change helps with the stdarch failure:

error[E0428]: the name `__breakpoint` is defined multiple times
  --> crates/core_arch/src/arm/armclang.rs:49:1
   |
22 | pub unsafe fn __breakpoint<const VAL: i32>() {
   | -------------------------------------------- previous definition of the value `__breakpoint` here
...
49 | pub unsafe fn __breakpoint<const VAL: i32>() {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `__breakpoint` redefined here
   |
   = note: `__breakpoint` must be defined only once in the value namespace of this module

@Amanieu
Copy link
Member Author

Amanieu commented Mar 6, 2021

I've fixed the other issues in the stdarch PR. The latest CI results show the problem.

The main issue is the representation of register and register class constraints:

  • In the AST these are strings since macro expansion don't have access to target-specific information yet.
  • In the HIR these are target-specific enums representing an actual register or register class. This lowering is needed because typeck and asm validation (intrinsicck in rustc_passes) need to know the actual register class.

Even though rustdoc doesn't run the HIR passes it still performs AST lowering so we need to lower registers and register classes to a dummy type so we can still produce a valid HIR.

@Amanieu Amanieu force-pushed the rustdoc_asm branch 2 times, most recently from bd60df1 to 4131e86 Compare March 6, 2021 18:58
@Amanieu
Copy link
Member Author

Amanieu commented Mar 6, 2021

I've simplified the code: it turns out that rustdoc is happy enough if I just lower the entire asm! to hir::ExprKind::Err.

@Amanieu Amanieu force-pushed the rustdoc_asm branch 2 times, most recently from 32cda4a to d5d6408 Compare March 6, 2021 19:00
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

It seems unfortunate to keep special-casing rustdoc everywhere, but I don't know how hard it would be move this later in the compiler. This seems no worse than the other hacks for rustdoc.

compiler/rustc_ast_lowering/src/expr.rs Outdated Show resolved Hide resolved
@nagisa
Copy link
Member

nagisa commented Mar 7, 2021

LGTM from the compiler impl standpoint.

@bors r+

I, too, agree that having to special-case rustdoc here is unfortunate, IMHO the best way to fix this would be to make it possible for rustdoc itself to entirely skip any handling of function bodies in any part of the compiler it ends up invoking. A most straightforward way would probably be walking the AST and replacing all the bodies with Err? (I for some reason remember that rustdoc did something along the lines of -Zeverybody-loops but that's not the case anymore?)

@nagisa
Copy link
Member

nagisa commented Mar 7, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2021

📌 Commit 4d2413296e5d849f75001128b2a9df026df6acdb has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2021
@nagisa nagisa assigned nagisa and unassigned davidtwco Mar 7, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 7, 2021

A most straightforward way would probably be walking the AST and replacing all the bodies with Err? (I for some reason remember that rustdoc did something along the lines of -Zeverybody-loops but that's not the case anymore?)

This broke quite a lot of rustdoc, it was changed about a year ago: #73566. The way to do this properly is to move the error checking to after AST desugaring, when typechecking is being done, then rustdoc should ignore it automatically.

@Amanieu
Copy link
Member Author

Amanieu commented Mar 7, 2021

Now that you mention it, could this change cause issues since we aren't lowering the Exprs in the asm! operands?

@jyn514
Copy link
Member

jyn514 commented Mar 7, 2021

Now that you mention it, could this change cause issues since we aren't lowering the Exprs in the asm! operands?

I think so, yes. Try this test case:

#![feature(decl_macro)]
#![feature(asm)]

pub unsafe fn aarch64(a: f64, b: f64) {
    let c;
    asm!("add {:d}, {:d}, d0", out(vreg) c, in(vreg) a, in("d0") {
        || {
            macro m() {}
        };
        b
    });
    c
}

@jyn514
Copy link
Member

jyn514 commented Mar 7, 2021

lol that causes rustc itself to fail, even without these changes:

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', compiler/rustc_middle/src/hir/map/mod.rs:300:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
stack backtrace:
   0: rust_begin_unwind
             at /rustc/45b3c28518e4c45dfd12bc2c4400c0d0e9639927/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/45b3c28518e4c45dfd12bc2c4400c0d0e9639927/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/45b3c28518e4c45dfd12bc2c4400c0d0e9639927/library/core/src/panicking.rs:50:5
   3: rustc_middle::hir::map::Map::body_owner
   4: rustc_middle::hir::map::Map::body_owner_def_id
   5: rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::par_body_owners
   6: rustc_typeck::check::typeck_item_bodies
   7: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task_impl
   8: rustc_data_structures::stack::ensure_sufficient_stack
   9: rustc_query_system::query::plumbing::force_query_with_job
  10: rustc_query_system::query::plumbing::get_query_impl
  11: <rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::typeck_item_bodies
  12: rustc_session::utils::<impl rustc_session::session::Session>::time
  13: rustc_typeck::check_crate
  14: rustc_interface::passes::analysis
  15: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task_impl
  16: rustc_data_structures::stack::ensure_sufficient_stack
  17: rustc_query_system::query::plumbing::force_query_with_job
  18: rustc_query_system::query::plumbing::get_query_impl
  19: <rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::analysis
  20: rustc_interface::passes::QueryContext::enter
  21: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter
  22: rustc_span::with_source_map
  23: rustc_interface::interface::create_compiler_and_run
  24: rustc_span::with_session_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@jyn514
Copy link
Member

jyn514 commented Mar 7, 2021

Here's an example that compiles when you add --target aarch64-unknown-linux-gnu:

#![feature(decl_macro)]
#![feature(asm)]

pub unsafe fn aarch64(a: f64, b: f64) -> f64 {
    let c;
    asm!("add {:d}, {:d}, d0", out(vreg) c, in(vreg) a, in("d0") {
        || {
            macro m() {}
        };
        b
    });
    c
}

@nagisa
Copy link
Member

nagisa commented Mar 7, 2021

asm!("...", in(reg) {
    /// documentation
    pub fn banana(...) { ... }
    42
})

kind of thing? Yeah, not lowering this would most likely be prone to causing all the same issues as what everybody-loops did.

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 7, 2021
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 7, 2021
@Amanieu
Copy link
Member Author

Amanieu commented Mar 8, 2021

I think the root cause is probably linked to #82869, so let's figure that out first.

@Amanieu
Copy link
Member Author

Amanieu commented Mar 9, 2021

I can see 2 ways of solving this properly:

  • Add an Invalid variant to InlineAsmReg and InlineAsmRegClass so we can still produce valid HIR even for asm! with invalid registers for the current target.
  • Defer all asm register & type validation to HIR->MIR lowering. HIR would then simply represent register names with Symbol like the AST does.

@Amanieu
Copy link
Member Author

Amanieu commented Mar 13, 2021

Updated, this should also fix #82869.

r? @nagisa

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

this should also fix #82869.

Can you please add a regression test for this as well? Overall LGTM! r=me.

@@ -309,6 +313,7 @@ impl InlineAsmReg {
Self::RiscV(r) => r.emit(out, arch, modifier),
Self::Hexagon(r) => r.emit(out, arch, modifier),
Self::Mips(r) => r.emit(out, arch, modifier),
Self::Err => unreachable!(),
Copy link
Member

Choose a reason for hiding this comment

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

Many of these unreachable!s here and in {modifier,reg}_to_llvm feel like they should be a bug! instead. These aren't very obviously unreachable and in the future it might be the case that somebody does change some code in a way that would cause this to become reached too (thus introducing a bug).

Copy link
Member Author

Choose a reason for hiding this comment

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

bug! is defined in rustc_middle which rustc_target can't depend on since that would result in a circular dependency. I've added a message to the unreachable though.

compiler/rustc_ast_lowering/src/expr.rs Show resolved Hide resolved
@Amanieu Amanieu force-pushed the rustdoc_asm branch 2 times, most recently from 8f76dda to ba00ddc Compare March 14, 2021 23:22
@Amanieu
Copy link
Member Author

Amanieu commented Mar 15, 2021

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Mar 15, 2021

📌 Commit ba00ddc has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 16, 2021
Allow rustdoc to handle asm! of foreign architectures

This allows rustdoc to process code containing `asm!` for architectures other than the current one. Since this never reaches codegen, we just replace target-specific registers and register classes with a dummy one.

Fixes rust-lang#82869
@bors
Copy link
Contributor

bors commented Mar 16, 2021

⌛ Testing commit ba00ddc with merge f24ce9b...

@bors
Copy link
Contributor

bors commented Mar 16, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing f24ce9b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2021
@bors bors merged commit f24ce9b into rust-lang:master Mar 16, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE on nested items when asm! has errors
8 participants