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

Rollup of 7 pull requests #95938

Closed
wants to merge 26 commits into from
Closed

Conversation

Dylan-DPC
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

JakobDegen and others added 26 commits April 7, 2022 21:43
We may sometimes emit an `invoke` instead of a `call` for inline
assembly during the MIR -> LLVM IR lowering. But we failed to update
the IR builder's current basic block before writing the results to the
outputs. This would result in invalid IR because the basic block would
end in a `store` instruction, which isn't a valid terminator.
Specifically, make it clear that it is immediately UB to pass ill-formed UTF-8 into the function. The previous wording left space to interpret that the UB only occurred when calling another function, which "assumes that `&str`s are valid UTF-8."

This does not change whether str being UTF-8 is a safety or a validity invariant. (As per previous discussion, it is a safety invariant, not a validity invariant.) It just makes it clear that valid UTF-8 is a precondition of str::from_utf8_unchecked, and that emitting an Abstract Machine fault (e.g. UB or a sanitizer error) on invalid UTF-8 is a valid thing to do.

If user code wants to create an unsafe `&str` pointing to ill-formed UTF-8, it must be done via transmutes. Also, just, don't.
Bootstrap already allows selecting these in `PathSet::has`, which allows
any string that matches the end of a full path.

I found these by adding `assert!(path.exists())` in `StepDescription::paths`.
I think ideally we wouldn't have any aliases that aren't paths, but I've held
off on enforcing that here since it may be controversial, I'll open a separate PR.
These paths (`_cranelift` and `_gcc`) are somewhat misleading, since they
actually tell bootstrap to build *all* codegen backends. But this seems like
a useful improvement in the meantime.
Document the current MIR semantics that are clear from existing code

This PR adds documentation to places, operands, rvalues, statementkinds, and terminatorkinds that describes their existing semantics and requirements. In many places the semantics depend on the Rust memory model or other T-Lang decisions - when this is the case, it is just noted as such with links to UCG issues where possible. I'm hopeful that none of the documentation added here can be used to justify optimizations that depend on the memory model. The documentation for places and operands probably comes closest to running afoul of this - if people think that it cannot be merged as is, it can definitely also be taken out.

The goal here is to only document parts of MIR that seem to be decided already, or are at least depended on by existing code. That leaves quite a number of open questions - those are marked as "needs clarification." I'm not sure what to do with those in this PR - we obviously can't decide all these questions here. Should I just leave them in as is? Take them out? Keep them in but as `//` instead of `///` comments?

If this is too big to review at once, I can split this up.

r? rust-lang/mir-opt
…compile, r=Amanieu

Fix miscompilation of inline assembly with outputs in cases where we emit an invoke instead of call instruction.

We ran into this bug where rustc would segfault while trying to compile certain uses of inline assembly.

Here is a simple repro that demonstrates the issue:
```rust
#![feature(asm_unwind)]

fn main() {
    let _x = String::from("string here just cause we need something with a non-trivial drop");
    let foo: u64;
    unsafe {
        std::arch::asm!(
            "mov {}, 1",
            out(reg) foo,
            options(may_unwind)
        );
    }
    println!("{}", foo);
}
```
([playground link](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=7d6641e83370d2536a07234aca2498ff))

But crucially `feature(asm_unwind)` is not actually needed and this can be triggered on stable as a result of the way async functions/generators are handled in the compiler. e.g.:

```rust
extern crate futures; // 0.3.21

async fn bar() {
    let foo: u64;
    unsafe {
        std::arch::asm!(
            "mov {}, 1",
            out(reg) foo,
        );
    }
    println!("{}", foo);
}

fn main() {
    futures::executor::block_on(bar());
}
```
([playground link](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1c7781c34dd4a3e80ae4bd936a0c82fc))

An example of the incorrect LLVM generated:
```llvm
bb1:                                              ; preds = %start
  %1 = invoke i64 asm sideeffect alignstack inteldialect unwind "mov ${0:q}, 1", "=&r,~{dirflag},~{fpsr},~{flags},~{memory}"()
          to label %bb2 unwind label %cleanup, !srcloc !9
  store i64 %1, i64* %foo, align 8

bb2:
[...snip...]
```

The store should not be placed after the asm invoke but rather should be in the normal control flow basic block (`bb2` in this case).

[Here](https://gist.github.com/luqmana/be1af5b64d2cda5a533e3e23a7830b44) is a writeup of the investigation that lead to finding this.
Fix formatting error in pin.rs docs

Not sure if there's more formatting issues I missed; I kinda lost interest reading midway through.
Clarify str::from_utf8_unchecked's invariants

Specifically, make it clear that it is immediately UB to pass ill-formed UTF-8 into the function. The previous wording left space to interpret that the UB only occurred when calling another function, which "assumes that `&str`s are valid UTF-8."

This does not change whether str being UTF-8 is a safety or a validity invariant. (As per previous discussion, it is a safety invariant, not a validity invariant.) It just makes it clear that valid UTF-8 is a precondition of str::from_utf8_unchecked, and that emitting an Abstract Machine fault (e.g. UB or a sanitizer error) on invalid UTF-8 is a valid thing to do.

If user code wants to create an unsafe `&str` pointing to ill-formed UTF-8, it must be done via transmutes. Also, just, don't.

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/str.3A.3Afrom_utf8_unchecked.20Safety.20requirement
…Mark-Simulacrum

Remove duplicate aliases for `check codegen_{cranelift,gcc}` and fix `build codegen_gcc`

* Remove duplicate aliases
    Bootstrap already allows selecting these in `PathSet::has`, which allows
    any string that matches the end of a full path.

    I found these by adding `assert!(path.exists())` in `StepDescription::paths`.
    I think ideally we wouldn't have any aliases that aren't paths, but I've held
    off on enforcing that here since it may be controversial, I'll open a separate PR.

* Add `build compiler/rustc_codegen_gcc` as an alias for `CodegenBackend`

    These paths (`_cranelift` and `_gcc`) are somewhat misleading, since they
    actually tell bootstrap to build *all* codegen backends. But this seems like
    a useful improvement in the meantime.

cc `@bjorn3` `@antoyo`
CI: do not compile libcore twice when performing LLVM PGO

I forgot the delete the first compilation when modifying this file in a previous PR.

r? `@lqd`
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Apr 11, 2022
@Dylan-DPC
Copy link
Member Author

@bors r+ rollup=never p=5

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📌 Commit ba25dc9 has been approved by Dylan-DPC

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit ba25dc9 with merge de2534989b5bbf354895bc7b6dc2a9af5bce17ad...

@bors
Copy link
Contributor

bors commented Apr 11, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 11, 2022
@Dylan-DPC Dylan-DPC closed this Apr 11, 2022
@rust-log-analyzer
Copy link
Collaborator

The job dist-i686-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] addr2line test:false 0.279
[RUSTC-TIMING] object test:false 3.891
[RUSTC-TIMING] profiler_builtins test:false 0.059
 Documenting std v0.0.0 (D:\a\rust\rust\library\std)
error: unresolved link to `AsFd::as_fd`
   |
   |
35 |     /// However, borrowing is not strictly required. See [`AsFd::as_fd`]
   |                                                            ^^^^^^^^^^^ no item named `AsFd` in scope
   |
   = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
error: could not document `std`

Caused by:
Caused by:
  process didn't exit successfully: `D:\a\rust\rust\build\bootstrap\debug\rustdoc --edition=2021 --crate-type dylib --crate-type rlib --crate-name std library\std\src\lib.rs --target i686-pc-windows-gnu -o D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\doc --cfg "feature=\"addr2line\"" --cfg "feature=\"backtrace\"" --cfg "feature=\"compiler-builtins-c\"" --cfg "feature=\"gimli-symbolize\"" --cfg "feature=\"miniz_oxide\"" --cfg "feature=\"object\"" --cfg "feature=\"panic_unwind\"" --cfg "feature=\"profiler\"" --cfg "feature=\"profiler_builtins\"" --cfg "feature=\"std_detect_dlsym_getauxval\"" --cfg "feature=\"std_detect_file_io\"" --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --markdown-css rust.css --markdown-no-toc -Z unstable-options --resource-suffix 1.62.0 --index-page D:\a\rust\rust\src/doc/index.md -C metadata=72d154eb834e1844 -L dependency=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps -L dependency=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\release\deps --extern addr2line=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libaddr2line-7e3c4a2a68f27b37.rmeta --extern alloc=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\liballoc-f546deb7a20d985d.rmeta --extern cfg_if=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libcfg_if-cb7f9cf2d51fa85a.rmeta --extern compiler_builtins=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libcompiler_builtins-06f90072d3965483.rmeta --extern core=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libcore-d881520c36ca2fd9.rmeta --extern hashbrown=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libhashbrown-cfce6880c181de10.rmeta --extern libc=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\liblibc-2dd5fa5bd6609a02.rmeta --extern miniz_oxide=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libminiz_oxide-207f87b86bd0fd66.rmeta --extern object=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libobject-b1c3c4c07f6e6700.rmeta --extern panic_abort=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libpanic_abort-04d3752b4563fbc2.rmeta --extern panic_unwind=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libpanic_unwind-68d0b1413d1e94cd.rmeta --extern profiler_builtins=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libprofiler_builtins-1afa42484a30b914.rmeta --extern rustc_demangle=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\librustc_demangle-506591228ac141d9.rmeta --extern std_detect=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libstd_detect-d867cc74e1ed2b7e.rmeta --extern unwind=D:\a\rust\rust\build\i686-pc-windows-gnu\stage2-std\i686-pc-windows-gnu\release\deps\libunwind-46a41b6b5121c2e7.rmeta -Csymbol-mangling-version=legacy -Zunstable-options -Zunstable-options --check-cfg=names() --check-cfg=values() --check-cfg=values(bootstrap) --check-cfg=values(stdarch_intel_sde) --check-cfg=values(no_fp_fmt_parse) --check-cfg=values(no_global_oom_handling) --check-cfg=values(freebsd12) --check-cfg=values(backtrace_in_libstd) "--check-cfg=values(target_env,\"libnx\")" "--check-cfg=values(target_os,\"watchos\")" "--check-cfg=values(target_arch,\"asmjs\",\"spirv\",\"nvptx\",\"nvptx64\",\"le32\",\"xtensa\")" --check-cfg=values(dont_compile_me) -Dwarnings -Wrustdoc::invalid_codeblock_attributes --crate-version "1.62.0-nightly
  (de2534989
  2022-04-11)" "-Zcrate-attr=doc(html_root_url=\"https://doc.rust-lang.org/nightly/\")" --cfg backtrace_in_libstd` (exit code: 1)

@Dylan-DPC Dylan-DPC deleted the rollup-rb17ghm branch April 11, 2022 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.