Skip to content

Migrate branch-protection-check-IBT to rmake.rs #134760

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 28, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Dec 25, 2024

  • The Makefile version never ran because of Makefile syntax confusion because ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64) compares x86 to x86_64, which always evaluates to false.
  • The test would've always failed because precompiled std is not built with -Z cf-protection=branch, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
  • Thus, the test input file is instead changed to a no_std program.
  • The test is currently limited to only x86_64-unknown-linux-gnu host, there are various other problems when the test is cross-compiled that I didn't want to fix atm, and is left as an exercise for the -Z cf-protection implementers.

The GNU property note was added by #110304 in order to address #103001.

Partially supersedes #129156.
The rmake.rs port was initially authored by @Rejyr in #126720.
This PR is co-authored with @Oneirical and @Rejyr.

r? @bjorn3 or reroll

try-job: x86_64-mingw-1
try-job: x86_64-mingw-2
try-job: x86_64-msvc
try-job: x86_64-apple-1
try-job: x86_64-apple-2

@rustbot rustbot added 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 25, 2024
@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 Dec 25, 2024
@jieyouxu

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 25, 2024
…eck-IBT, r=<try>

Migrate `branch-protection-check-IBT` to rmake.rs

- The Makefile version *never* ran because of Makefile syntax confusion because `ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64)` [compares `x86` to `x86_64`, which always evaluates to false](rust-lang#126720 (comment)).
- The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` program.

The GNU property note was added by rust-lang#110304 in order to address rust-lang#103001.

Partially supersedes rust-lang#129156.
The rmake.rs port was initially authored by `@Rejyr` in rust-lang#126720.
This PR is co-authored with `@Oneirical` and `@Rejyr.`

r? `@bjorn3` or reroll

try-job: x86_64-msvc
try-job: x86_64-apple
@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment was marked as off-topic.

@jieyouxu
Copy link
Member Author

@bors try

@bors
Copy link
Collaborator

bors commented Dec 25, 2024

⌛ Trying commit 5871cee with merge a90422e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 25, 2024
…eck-IBT, r=<try>

Migrate `branch-protection-check-IBT` to rmake.rs

- The Makefile version *never* ran because of Makefile syntax confusion because `ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64)` [compares `x86` to `x86_64`, which always evaluates to false](rust-lang#126720 (comment)).
- The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` program.

The GNU property note was added by rust-lang#110304 in order to address rust-lang#103001.

Partially supersedes rust-lang#129156.
The rmake.rs port was initially authored by `@Rejyr` in rust-lang#126720.
This PR is co-authored with `@Oneirical` and `@Rejyr.`

r? `@bjorn3` or reroll

try-job: x86_64-msvc
try-job: x86_64-apple-1
try-job: x86_64-apple-2
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Dec 25, 2024

💔 Test failed - checks-actions

@jieyouxu
Copy link
Member Author

... Right.

@jieyouxu jieyouxu force-pushed the enable-branch-protection-check-IBT branch from 5871cee to 5cd8653 Compare December 26, 2024 10:55
@jieyouxu
Copy link
Member Author

@bors try

@bors
Copy link
Collaborator

bors commented Dec 26, 2024

⌛ Trying commit 5cd8653 with merge d4c0af3...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 26, 2024
…eck-IBT, r=<try>

Migrate `branch-protection-check-IBT` to rmake.rs

- The Makefile version *never* ran because of Makefile syntax confusion because `ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64)` [compares `x86` to `x86_64`, which always evaluates to false](rust-lang#126720 (comment)).
- The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` program.
- Ignore cross-compile for now, because that will require us to build `core` for the cross-compile target (something like `minicore` will not suffice because we need to reach and go past the linking stage).

The GNU property note was added by rust-lang#110304 in order to address rust-lang#103001.

Partially supersedes rust-lang#129156.
The rmake.rs port was initially authored by `@Rejyr` in rust-lang#126720.
This PR is co-authored with `@Oneirical` and `@Rejyr.`

r? `@bjorn3` or reroll

try-job: x86_64-msvc
try-job: x86_64-apple-1
try-job: x86_64-apple-2
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Dec 26, 2024

💔 Test failed - checks-actions

@jieyouxu jieyouxu force-pushed the enable-branch-protection-check-IBT branch from 5cd8653 to 0734389 Compare December 26, 2024 13:13
@jieyouxu
Copy link
Member Author

Ok, what if I just try to produce a minimal executable...
@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 26, 2024
…eck-IBT, r=<try>

Migrate `branch-protection-check-IBT` to rmake.rs

- The Makefile version *never* ran because of Makefile syntax confusion because `ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64)` [compares `x86` to `x86_64`, which always evaluates to false](rust-lang#126720 (comment)).
- The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` program.
- Only specifically `x86_64-unknown-linux-gnu` for now.

The GNU property note was added by rust-lang#110304 in order to address rust-lang#103001.

Partially supersedes rust-lang#129156.
The rmake.rs port was initially authored by `@Rejyr` in rust-lang#126720.
This PR is co-authored with `@Oneirical` and `@Rejyr.`

r? `@bjorn3` or reroll

try-job: x86_64-mingw-1
try-job: x86_64-mingw-2
try-job: x86_64-msvc
try-job: x86_64-apple-1
try-job: x86_64-apple-2
@bors
Copy link
Collaborator

bors commented Dec 26, 2024

⌛ Trying commit 0734389 with merge 3d945a5...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Dec 26, 2024

💔 Test failed - checks-actions

@jieyouxu jieyouxu force-pushed the enable-branch-protection-check-IBT branch from 0734389 to 4b4bc52 Compare December 27, 2024 16:37
@jieyouxu
Copy link
Member Author

jieyouxu commented Dec 27, 2024

Ok I gave up fighting for cross-compile scenarios, I made the test x86_64-unknown-linux-gnu host only for now.

@rustbot ready

@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 Dec 27, 2024
Copy link
Member

@lqd lqd left a comment

Choose a reason for hiding this comment

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

If you're happy with the previous try builds,

image

with/without the couple nits below

@jieyouxu jieyouxu assigned lqd and unassigned bjorn3 Dec 28, 2024
- The Makefile version *never* ran because of Makefile syntax confusion.
- The test would've always failed because precompiled std is not built
  with `-Z cf-protection=branch`, but linkers require all input object
  files to indicate IBT support in order to enable IBT for the
  executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` + `no_core`
  program.

Co-authored-by: Jerry Wang <jerrylwang123@gmail.com>
Co-authored-by: Oneirical <manchot@videotron.ca>
@jieyouxu jieyouxu force-pushed the enable-branch-protection-check-IBT branch from 4b4bc52 to b32591e Compare December 28, 2024 03:58
@jieyouxu
Copy link
Member Author

jieyouxu commented Dec 28, 2024

The current formulation of the test only runs on x86_64-unknown-linux-gnu host. However, the original test simply never ran due to the Makefile being broken. So this is still a net gain in overall test coverage (lol). As for how to expand test coverage of this test, that is left to -Z cf-protection=branch / Intel CET (cf. #93754) implementors as an exercise.

@bors r=@lqd rollup

@bors
Copy link
Collaborator

bors commented Dec 28, 2024

📌 Commit b32591e has been approved by lqd

It is now in the queue for this repository.

@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 Dec 28, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 28, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#134737 (Implement `default_overrides_default_fields` lint)
 - rust-lang#134760 (Migrate `branch-protection-check-IBT` to rmake.rs)
 - rust-lang#134829 (Migrate `libs-through-symlink` to rmake.rs)
 - rust-lang#134832 (Update `compiler-builtins` to 0.1.140)
 - rust-lang#134840 (compiletest: Only pass the post-colon value to `parse_normalize_rule`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dd03fba into rust-lang:master Dec 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 28, 2024
Rollup merge of rust-lang#134760 - jieyouxu:enable-branch-protection-check-IBT, r=lqd

Migrate `branch-protection-check-IBT` to rmake.rs

- The Makefile version *never* ran because of Makefile syntax confusion because `ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64)` [compares `x86` to `x86_64`, which always evaluates to false](rust-lang#126720 (comment)).
- The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` program.
- The test is currently limited to only `x86_64-unknown-linux-gnu` host, there are various other problems when the test is cross-compiled that I didn't want to fix atm, and is left as an exercise for the `-Z cf-protection` implementers.

The GNU property note was added by rust-lang#110304 in order to address rust-lang#103001.

Partially supersedes rust-lang#129156.
The rmake.rs port was initially authored by `@Rejyr` in rust-lang#126720.
This PR is co-authored with `@Oneirical` and `@Rejyr.`

r? `@bjorn3` or reroll

try-job: x86_64-mingw-1
try-job: x86_64-mingw-2
try-job: x86_64-msvc
try-job: x86_64-apple-1
try-job: x86_64-apple-2
@jieyouxu jieyouxu deleted the enable-branch-protection-check-IBT branch December 28, 2024 11:17
poliorcetics pushed a commit to poliorcetics/rust that referenced this pull request Dec 28, 2024
…check-IBT, r=lqd

Migrate `branch-protection-check-IBT` to rmake.rs

- The Makefile version *never* ran because of Makefile syntax confusion because `ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64)` [compares `x86` to `x86_64`, which always evaluates to false](rust-lang#126720 (comment)).
- The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std.
- Thus, the test input file is instead changed to a `no_std` program.
- The test is currently limited to only `x86_64-unknown-linux-gnu` host, there are various other problems when the test is cross-compiled that I didn't want to fix atm, and is left as an exercise for the `-Z cf-protection` implementers.

The GNU property note was added by rust-lang#110304 in order to address rust-lang#103001.

Partially supersedes rust-lang#129156.
The rmake.rs port was initially authored by `@Rejyr` in rust-lang#126720.
This PR is co-authored with `@Oneirical` and `@Rejyr.`

r? `@bjorn3` or reroll

try-job: x86_64-mingw-1
try-job: x86_64-mingw-2
try-job: x86_64-msvc
try-job: x86_64-apple-1
try-job: x86_64-apple-2
poliorcetics pushed a commit to poliorcetics/rust that referenced this pull request Dec 28, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#134737 (Implement `default_overrides_default_fields` lint)
 - rust-lang#134760 (Migrate `branch-protection-check-IBT` to rmake.rs)
 - rust-lang#134829 (Migrate `libs-through-symlink` to rmake.rs)
 - rust-lang#134832 (Update `compiler-builtins` to 0.1.140)
 - rust-lang#134840 (compiletest: Only pass the post-colon value to `parse_normalize_rule`)

r? `@ghost`
`@rustbot` modify labels: rollup
# for free to join this conversation on GitHub. Already have an account? # 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

6 participants