Skip to content

[BoundsSafety] Add warning diagnostics for uses of legacy bounds checks #10800

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

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

delcypher
Copy link

@delcypher delcypher commented Jun 6, 2025

This adds warning diagnostics when any of the new bounds checks that can
be enabled with -fbounds-safety-bringup-missing-checks=batch_0 are
disabled.

If all bounds checks in the batch are disabled a single diagnostic is
emitted. If only some of the bounds checks in the batch are disabled
then a diagnostic is emitted for each disabled bounds check. The
implementation will either suggest enabling a batch of checks (e.g.
-fbounds-safety-bringup-missing-checks=batch_0) or will suggest
removing a flag that is explicitly disabling a check (e.g.
-fno-bounds-safety-bringup-missing-checks=access_size).

The current implementation supports there being multple batches of
checks. However, there is currently only one batch (batch_0).

I originally tried to emit these warnings in the frontend. Unfortunately
it turns out warning suppression (i.e.
-Wno-bounds-safety-legacy-checks-enabled) and -Werror don't work
correctly if warnings are emitted from the frontend (rdar://152730261).
To workaround this the -fbounds-safety-bringup-missing-checks= flags
are now also parsed in the Driver and at this point (and only this
point) diagnostics for missing checks are emitted.

The intention is to make these warnings be errors eventually.

rdar://150805550

@delcypher delcypher self-assigned this Jun 6, 2025
@delcypher delcypher added the clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang label Jun 6, 2025
@hnrklssn
Copy link

hnrklssn commented Jun 6, 2025

What happens if you compile with %clang_cc1 -fbounds-safety -fbounds-safety-bringup-missing-checks=batch_0 -fno-bounds-safety-bringup-missing-checks=access_size

@delcypher
Copy link
Author

%clang_cc1 -fbounds-safety -fbounds-safety-bringup-missing-checks=batch_0 -fno-bounds-safety-bringup-missing-checks=access_size

You'll get all the checks in batch_0 except the access_size check. I'll add a test case for it.

@hnrklssn
Copy link

hnrklssn commented Jun 6, 2025

%clang_cc1 -fbounds-safety -fbounds-safety-bringup-missing-checks=batch_0 -fno-bounds-safety-bringup-missing-checks=access_size

You'll get all the checks in batch_0 except the access_size check. I'll add a test case for it.

I was thinking more along the lines of whether you still get compiling with "X" bounds check disabled is deprecated; compile with -fbounds-safety-bringup-missing-checks=batch_0 to enable the new bound checks, which would be confusing when you already use that flag

@delcypher
Copy link
Author

I was thinking more along the lines of whether you still get compiling with "X" bounds check disabled is deprecated; compile with -fbounds-safety-bringup-missing-checks=batch_0 to enable the new bound checks, which would be confusing when you already use that flag

Ah. That's a good point. Right now the diagnostic will say:

compiling with "access_size" bounds check disabled is deprecated; compile with -fbounds-safety-bringup-missing-checks=batch_0 to enable the new bound checks

That is confusing but I'm not sure how best to fix that.

If we suggest adding -fbounds-safety-bringup-missing-checks=access_size instead that's technically right if they add it to the end of their flags but not something we want to encourage. Being able to disable individual checks is mostly there to give us a convenient way to opt-out of certain checks. In general we want users to opt into the entire batch of checks instead.

If the user's flags are currently

-fbounds-safety-bringup-missing-checks=batch_0 -fno-bounds-safety-bringup-missing-checks=access_size

the right fix is not to add -fbounds-safety-bringup-missing-checks=access_size but instead remove -fno-bounds-safety-bringup-missing-checks=access_size.

In general there are multiple scenarios where we can end up with bounds checks disabled

  • The compiler default is all checks disabled (i.e. the user didn't pass any -fbounds-safety-bringup-missing-checks flags)
  • The user manually opts into some checks (e.g. -fbounds-safety-bringup-missing-checks=access_size,libc_attributes). In this case any checks they didn't list will be disabled.
  • The user opts into batch_0 but then explicitly opts out of some checks (your example).

In only the last case suggesting -fbounds-safety-bringup-missing-checks=batch_0 is confusing. If I move my logic into ParseBoundsSafetyNewChecksMaskFromArgs I can probably distinguish between these 3 cases cheaply. I'll give it a try.

@delcypher delcypher force-pushed the dliew/rdar-150805550 branch from 871ae81 to 5ad987d Compare June 7, 2025 07:20
@delcypher
Copy link
Author

I've reworked this so now it works a lot better but I need to fix a bunch of tests that have broken because of this diagnostic.

LangOptions::BS_CHK_None;

// Loop over all batches except "none"
for (size_t BatchIdx = 0; BatchIdx < BatchesAR.size() - 1; ++BatchIdx) {

Choose a reason for hiding this comment

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

Do we have plans to support more batches? Right now, we only have one batch.

Copy link
Author

Choose a reason for hiding this comment

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

I have a strong hunch that we'll need more in the future so I'm including support for multiple batches.

@delcypher delcypher force-pushed the dliew/rdar-150805550 branch 2 times, most recently from 90b234c to 0c39cd8 Compare June 10, 2025 19:35
@delcypher
Copy link
Author

@swift-ci test llvm

@delcypher
Copy link
Author

Linux failures:

13:33:22  ********************
13:33:22  Failed Tests (4):
13:33:22    Clang :: BoundsSafety-legacy-checks/CodeGen/flexible-array-member-shared-decls-O2.c
13:33:22    Clang :: BoundsSafety/CodeGen/compound-literal-ended_by-O2-disabled-checks.c
13:33:22    Clang :: BoundsSafety/CodeGen/compound-literal-ended_by-O2.c
13:33:22    Clang :: BoundsSafety/CodeGen/flexible-array-member-shared-decls-O2.c

@delcypher
Copy link
Author

So without my changes I'm seeing these failures for macOS so these are unrelated to my change.

********************
Failed Tests (6):
  Clang :: BoundsSafety-legacy-checks/CodeGen/flexible-array-member-shared-decls-O2.c
  Clang :: BoundsSafety/CodeGen/compound-literal-ended_by-O0-disabled-checks.c
  Clang :: BoundsSafety/CodeGen/compound-literal-ended_by-O0.c
  Clang :: BoundsSafety/CodeGen/compound-literal-ended_by-O2-disabled-checks.c
  Clang :: BoundsSafety/CodeGen/compound-literal-ended_by-O2.c
  Clang :: BoundsSafety/CodeGen/flexible-array-member-shared-decls-O2.c

@delcypher
Copy link
Author

@rapidsna @hnrklssn @patrykstefanski This PR is ready for another round of reviews.

This adds warning diagnostics when any of the new bounds checks that can
be enabled with `-fbounds-safety-bringup-missing-checks=batch_0` are
disabled.

If all bounds checks in the batch are disabled a single diagnostic is
emitted. If only some of the bounds checks in the batch are disabled
then a diagnostic is emitted for each disabled bounds check. The
implementation will either suggest enabling a batch of checks (e.g.
`-fbounds-safety-bringup-missing-checks=batch_0`) or will suggest
removing a flag that is explicitly disabling a check (e.g.
`-fno-bounds-safety-bringup-missing-checks=access_size`).

The current implementation supports there being multple batches of
checks. However, there is currently only one batch (`batch_0`).

I originally tried to emit these warnings in the frontend. Unfortunately
it turns out warning suppression (i.e.
`-Wno-bounds-safety-legacy-checks-enabled`) and `-Werror` don't work
correctly if warnings are emitted from the frontend (rdar://152730261).
To workaround this the `-fbounds-safety-bringup-missing-checks=` flags
are now also parsed in the Driver and at this point (and only this
point) diagnostics for missing checks are emitted.

The intention is to make these warnings be errors eventually.

rdar://150805550
@delcypher delcypher force-pushed the dliew/rdar-150805550 branch from 0c39cd8 to 63069eb Compare June 10, 2025 22:54
@delcypher
Copy link
Author

@swift-ci test llvm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants