-
Notifications
You must be signed in to change notification settings - Fork 343
[BoundsSafety][Attempt 2] Add warning diagnostics for uses of legacy bounds checks #10898
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
Conversation
@swift-ci test llvm |
349afa5
to
bf4fa08
Compare
auto FilteredArgs = | ||
Args.filtered(OPT_fbounds_safety_bringup_missing_checks_EQ, | ||
OPT_fno_bounds_safety_bringup_missing_checks_EQ); | ||
if (FilteredArgs.empty()) { | ||
// No flags present. Use the default | ||
return LangOptions::getDefaultBoundsSafetyNewChecksMask(); | ||
auto Result = LangOptions::getDefaultBoundsSafetyNewChecksMask(); | ||
if (DiagnoseMissingChecks && Diags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hnrklssn This is where the bug was in the first version. There was a missing guard for the call to DiagnoseDisabledBoundsSafetyChecks
which meant if DiagnoseMissingChecks
was false we'd try to diagnose the issues anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. So I think I need to write a test case for this because all the BoundsSafety tests for this passed without making this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I figured out it's not possible to (easily) write a test case for this. The buggy behavior depends on bounds checks being disabled by default which is not desirable behavior. While technically we could add a hidden flag that changes the default for the purposes of testing I think this adds a lot of complexity without a huge benefit.
bf4fa08
to
ddcf48d
Compare
…bounds checks ** This is the second attempt landing this patch. This first (swiftlang#10800 4c8f6e7) failed because there was a bug. ** The bug was in `ParseBoundsSafetyNewChecksMaskFromArgs` where the call to `DiagnoseDisabledBoundsSafetyChecks` was not guarded with `DiagnoseMissingChecks`. This missing check caused diagnostics to appear when they shouldn't and caused crashes in cases where the `Diags` pointer was nullptr. To fix this the missing guard has been added and an assert that `Diags` is not null has been added. Unfortunately it isn't possible to write a regression test for this because the behavior depends on having at least some bounds checks disabled by default, which is not desirable behavior. While techinically we could add a hidden flag to change the default for the purposes of testing the added complexity of doing this doesn't really justify the added test coverage. This version of the patch also guards assigning to `Opts.BoundsSafetyBringUpMissingChecks` in `CompilerInvocation::ParseLangArgs` so that it is only assigned to when `-fbounds-safety` is enabled. This is done for several reasons * It avoids unnecessarily parsing the `-fbounds-safety-bringup-missing-checks=` flags when `-fbounds-safety` is disabled. * Isolates code built without `-fbounds-safety` from bugs in `ParseBoundsSafetyNewChecksMaskFromArgs`. * It is more consistent with `CompilerInvocationBase::GenerateLangArgs` which only emits `-fbounds-safety-bring-checks=` when `-fbounds-safety` is enabled. --- 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
ddcf48d
to
0aab1a8
Compare
@swift-ci test llvm |
** This is the second attempt landing this patch. This first (#10800
4c8f6e7) failed because there was a bug. **
The bug was in
ParseBoundsSafetyNewChecksMaskFromArgs
where the call toDiagnoseDisabledBoundsSafetyChecks
was not guarded withDiagnoseMissingChecks
. This missing check caused diagnostics to appearwhen they shouldn't and caused crashes in cases where the
Diags
pointer was nullptr. To fix this the missing guard has been added and
an assert that
Diags
is not null has been added.Unfortunately it isn't possible to write a regression test for this
because the behavior depends on having at least some bounds checks
disabled by default, which is not desirable behavior. While techinically
we could add a hidden flag to change the default for the purposes of
testing the added complexity of doing this doesn't really justify the
added test coverage.
This version of the patch also guards assigning to
Opts.BoundsSafetyBringUpMissingChecks
inCompilerInvocation::ParseLangArgs
so that it is only assigned towhen
-fbounds-safety
is enabled. This is done for several reasons-fbounds-safety-bringup-missing-checks=
flagswhen
-fbounds-safety
is disabled.-fbounds-safety
from bugs inParseBoundsSafetyNewChecksMaskFromArgs
.CompilerInvocationBase::GenerateLangArgs
which only emits
-fbounds-safety-bring-checks=
when-fbounds-safety
is enabled.This adds warning diagnostics when any of the new bounds checks that can
be enabled with
-fbounds-safety-bringup-missing-checks=batch_0
aredisabled.
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 suggestremoving 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 workcorrectly if warnings are emitted from the frontend (rdar://152730261).
To workaround this the
-fbounds-safety-bringup-missing-checks=
flagsare 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