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

run-make: Delete cat-and-grep-sanity-check and restrict branch-protection-check-IBT to stable #129156

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Oneirical
Copy link
Contributor

@Oneirical Oneirical commented Aug 16, 2024

Part of #121876 and the associated Google Summer of Code project.

First, this PR deletes the now useless cat-and-grep-sanity-check test.

Second, it revisits the branch-protection-check-IBT test, which was disabled due to a nonsensical llvm_components check. #126720 states that the test does work on stable rustc, so let's check this: added //@ only-stable.

If this works, some of the FIXME and commented-out lines will need cleanup before merging.

Please try:

try-job: x86_64-gnu-stable

@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@jieyouxu
Copy link
Member

@bors delegate+ (try jobs)

@bors
Copy link
Contributor

bors commented Aug 16, 2024

✌️ @Oneirical, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@Oneirical
Copy link
Contributor Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2024
run-make: Delete `cat-and-grep-sanity-check` and restrict `branch-protection-check-IBT` to stable

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

First, this PR deletes the now useless `cat-and-grep-sanity-check` test.

Second, it revisits the `branch-protection-check-IBT` test, which was disabled due to a nonsensical `llvm_components` check. rust-lang#126720 states that the test does work on stable rustc, so let's check this: added `//@ only-stable`.

If this works, some of the FIXME and commented-out lines will need cleanup before merging.

Please try:

try-job: x86_64-gnu-stable
@bors
Copy link
Contributor

bors commented Aug 16, 2024

⌛ Trying commit 41cd029 with merge 2b85d0b...

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 16, 2024
@Oneirical
Copy link
Contributor Author

Oneirical commented Aug 16, 2024

@bors try- (feature request: this should be called bors give up)

@Oneirical
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 16, 2024

⌛ Trying commit d85d292 with merge 2f217ea...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2024
run-make: Delete `cat-and-grep-sanity-check` and restrict `branch-protection-check-IBT` to stable

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

First, this PR deletes the now useless `cat-and-grep-sanity-check` test.

Second, it revisits the `branch-protection-check-IBT` test, which was disabled due to a nonsensical `llvm_components` check. rust-lang#126720 states that the test does work on stable rustc, so let's check this: added `//@ only-stable`.

If this works, some of the FIXME and commented-out lines will need cleanup before merging.

Please try:

try-job: x86_64-gnu-stable
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-stable failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
file:.git/config remote.origin.url=https://github.com/rust-lang-ci/rust
file:.git/config remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config gc.auto=0
file:.git/config http.https://github.com/.extraheader=AUTHORIZATION: basic ***
file:.git/config branch.try.remote=origin
file:.git/config branch.try.merge=refs/heads/try
file:.git/config submodule.library/backtrace.url=https://github.com/rust-lang/backtrace-rs.git
file:.git/config submodule.library/stdarch.active=true
file:.git/config submodule.library/stdarch.url=https://github.com/rust-lang/stdarch.git
file:.git/config submodule.src/doc/book.active=true
---
##[endgroup]
[TIMING] core::build_steps::tool::ToolBuild { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu, tool: "tidy", path: "src/tools/tidy", mode: ToolBootstrap, source_type: InTree, extra_features: [], allow_features: "", cargo_args: [] } -- 29.943
[TIMING] core::build_steps::tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu } -- 0.000
tidy check
tidy error: Makefile `/checkout/tests/run-make/branch-protection-check-IBT/Makefile` no longer exists and should be removed from the exclusions in `src/tools/tidy/src/allowed_run_make_makefiles.txt`, you can run `x test tidy --bless` to update the allow list
tidy error: Makefile `/checkout/tests/run-make/cat-and-grep-sanity-check/Makefile` no longer exists and should be removed from the exclusions in `src/tools/tidy/src/allowed_run_make_makefiles.txt`, you can run `x test tidy --bless` to update the allow list
some tidy checks failed
some tidy checks failed
Command has failed. Rerun with -v to see more details.
  local time: Fri Aug 16 15:35:30 UTC 2024
  network time: Fri, 16 Aug 2024 15:35:30 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-stable failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
file:.git/config remote.origin.url=https://github.com/rust-lang-ci/rust
file:.git/config remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config gc.auto=0
file:.git/config http.https://github.com/.extraheader=AUTHORIZATION: basic ***
file:.git/config branch.try.remote=origin
file:.git/config branch.try.merge=refs/heads/try
file:.git/config submodule.library/backtrace.url=https://github.com/rust-lang/backtrace-rs.git
file:.git/config submodule.library/stdarch.active=true
file:.git/config submodule.library/stdarch.url=https://github.com/rust-lang/stdarch.git
file:.git/config submodule.src/doc/book.active=true
---
---- [run-make] tests/run-make/branch-protection-check-IBT stdout ----

error: rmake recipe failed to complete
status: exit status: 101
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/branch-protection-check-IBT/rmake_out" && env -u RUSTFLAGS AR="ar" CC="cc" CC_DEFAULT_FLAGS="-ffunction-sections -fdata-sections -fPIC -m64" CXX="c++" CXX_DEFAULT_FLAGS="-ffunction-sections -fdata-sections -fPIC -m64" HOST_RPATH_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" LD_LIB_PATH_ENVVAR="LD_LIBRARY_PATH" LLVM_BIN_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm/bin" LLVM_COMPONENTS="aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen codegendata codegentypes core coroutines coverage csky cskyasmparser cskycodegen cskydesc cskydisassembler cskyinfo debuginfobtf debuginfocodeview debuginfodwarf debuginfogsym debuginfologicalview debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker dwarflinkerclassic dwarflinkerparallel dwp engine executionengine extensions filecheck frontenddriver frontendhlsl frontendoffloading frontendopenacc frontendopenmp fuzzercli fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo hipstdpar instcombine instrumentation interfacestub interpreter ipo irprinter irreader jitlink libdriver lineeditor linker loongarch loongarchasmparser loongarchcodegen loongarchdesc loongarchdisassembler loongarchinfo lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts objcopy object objectyaml option orcdebugging orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvtargetmca runtimedyld sandboxir scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target targetparser textapi textapibinaryreader transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo webassemblyutils windowsdriver windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info x86targetmca xray xtensa xtensaasmparser xtensacodegen xtensadesc xtensadisassembler xtensainfo" LLVM_FILECHECK="/checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm/bin/FileCheck" PYTHON="/usr/bin/python3" RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUSTDOC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" SOURCE_ROOT="/checkout" TARGET="x86_64-unknown-linux-gnu" TARGET_RPATH_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" TARGET_RPATH_ENV="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/branch-protection-check-IBT/rmake_out:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/branch-protection-check-IBT/rmake"
--- stderr -------------------------------
--- stderr -------------------------------
=== HAYSTACK ===
Displaying notes found in: .note.gnu.build-id
  Owner                Data size  Description
  GNU                  0x00000014 NT_GNU_BUILD_ID (unique build ID bitstring)
    Build ID: f41d700c73e7e4aacdbf360ebe994bb0564b9efb
=== NEEDLE ===
.note.gnu.property
thread 'main' panicked at /checkout/tests/run-make/branch-protection-check-IBT/rmake.rs:27:51:
needle was not found in haystack

@bors
Copy link
Contributor

bors commented Aug 16, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Hm. I'll need to find PG-exploit-mitigations experts to help us take a look. This seems weird.

@@ -4,17 +4,16 @@
// python3 x.py test --target x86_64-unknown-linux-gnu tests/run-make/branch-protection-check-IBT/

//@ only-x86_64
//@ only-stable
Copy link
Member

@jieyouxu jieyouxu Aug 21, 2024

Choose a reason for hiding this comment

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

Problem: this can't be correct, it's been in stable since forever, and I don't see any changes that would cause this to not be emitted (yet).


//@ ignore-test
// FIXME(jieyouxu): see the FIXME in the Makefile
Copy link
Member

Choose a reason for hiding this comment

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

Remark (for myself): FIXME

Comment on lines -14 to +16
let llvm_components = env_var("LLVM_COMPONENTS");
if !format!(" {llvm_components} ").contains(" x86 ") {
return;
}
// if !llvm_components_contain("x86") {
// panic!();
// }
Copy link
Member

@jieyouxu jieyouxu Aug 21, 2024

Choose a reason for hiding this comment

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

Discussion (self-remark mostly): it's not entirely clear to me why we need to check for llvm-components.

EDIT: because if we want to run target-specific codegen, it will need the llvm component. Right.

@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Aug 29, 2024

cc @bjorn3 do you have any idea if .note.gnu.property is still needed for linkers to properly handle Intel IBT? Or do you know anyone else who might have a clue if this note is still needed?

AFAICT (at least on nightly/master), on a x86_64-linux-unknown-gnu host, .note.gnu.property may have never been emitted, or have regressed since #110304

$ rustc \
    --target=x86_64-linux-unknown-gnu \
    -Z cf-protection=branch \
    -C link-args='-nostartfiles' \
    hello_world.rs \
    -o hello_world
$ llvm-readelf -nW hello_world

only has build-id, not .note.gnu.property.

Asked in https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Branch.20protection.20and.20.60.2Enote.2Egnu.2Eproperty.60/near/465997093.


Update: may need -Z build-std...

@jieyouxu
Copy link
Member

jieyouxu commented Aug 29, 2024

As nikic suggested, I was able to repro the note via

$ RUSTFLAGS="-Z cf-protection=branch" cargo +nightly build -Z build-std --target=x86_64-unknown-linux-gnu

so we probably want to use bootstrap cargo like in some other tests (yes I know this will require internet connection and is probably not ideal).

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2024
@bjorn3
Copy link
Member

bjorn3 commented Aug 29, 2024

do you have any idea if .note.gnu.property is still needed for linkers to properly handle Intel IBT? Or do you know anyone else who might have a clue if this note is still needed?

Not all that familiar with this, but I would IBT to only be enabled for the process if .note.gnu.property indicates that the executable was compiled with IBT support and the linker to only tell that IBT is supported if all input object files indicate that they support IBT, which in turn requires the standard library to be compiled with IBT enabled. I wonder if we can just unconditionally compile the standard library with IBT support. As I understand it, it only inserts endbr instructions at the start of each function and endbr is a nop on CPU's that don't support IBT. Many distros nowadays compile everything with IBT support enabled anyway.

@alex-semenyuk alex-semenyuk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2024
@bors
Copy link
Contributor

bors commented Oct 8, 2024

☔ The latest upstream changes (presumably #131387) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@Oneirical any updates on this? thanks

@jieyouxu
Copy link
Member

jieyouxu commented Nov 7, 2024

@rustbot review (I need to look at it again)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

9 participants