[BoundsSafety] Add -fexperimental-bounds-safety CC1 and language option and use it to tweak counted_by's semantics
#92623
Conversation
delcypher
requested review from
AaronBallman,
apple-fcloutier,
bwendling,
erichkeane,
hnrklssn,
kees,
patrykstefanski and
rapidsna
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Dan Liew (delcypher) ChangesThis adds the This cc1 flag is not exposed as a driver flag yet because most of the implementation isn't upstream yet. The language option is used to make a small semantic change to how the E.g. Full diff: https://github.com/llvm/llvm-project/pull/92623.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 09eb92d6f10d2..77ac7cfbddfca 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -504,6 +504,8 @@ COMPATIBLE_LANGOPT(IncrementalExtensions, 1, 0, " True if we want to process sta
BENIGN_LANGOPT(CheckNew, 1, 0, "Do not assume C++ operator new may not return NULL")
+LANGOPT(BoundsSafety, 1, 0, "Bounds safety extension for C")
+
#undef LANGOPT
#undef COMPATIBLE_LANGOPT
#undef BENIGN_LANGOPT
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 7bb781667e926..798ccbe3b94d5 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1871,6 +1871,14 @@ def fapinotes_swift_version : Joined<["-"], "fapinotes-swift-version=">,
MetaVarName<"<version>">,
HelpText<"Specify the Swift version to use when filtering API notes">;
+defm bounds_safety : BoolFOption<
+ "experimental-bounds-safety",
+ LangOpts<"BoundsSafety">, DefaultFalse,
+ PosFlag<SetTrue, [], [CC1Option], "Enable">,
+ NegFlag<SetFalse, [], [CC1Option], "Disable">,
+ BothFlags<[], [CC1Option],
+ " experimental bounds safety extension for C">>;
+
defm addrsig : BoolFOption<"addrsig",
CodeGenOpts<"Addrsig">, DefaultFalse,
PosFlag<SetTrue, [], [ClangOption, CC1Option], "Emit">,
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index e816ea3647a7c..7eb29499dec7d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8695,7 +8695,7 @@ static bool CheckCountedByAttrOnField(
} else if (PointeeTy->isFunctionType()) {
InvalidTypeKind = CountedByInvalidPointeeTypeKind::FUNCTION;
} else if (PointeeTy->isStructureTypeWithFlexibleArrayMember()) {
- if (FieldTy->isArrayType()) {
+ if (FieldTy->isArrayType() && !S.getLangOpts().BoundsSafety) {
// This is a workaround for the Linux kernel that has already adopted
// `counted_by` on a FAM where the pointee is a struct with a FAM. This
// should be an error because computing the bounds of the array cannot be
diff --git a/clang/test/Sema/attr-counted-by-bounds-safety-vlas.c b/clang/test/Sema/attr-counted-by-bounds-safety-vlas.c
new file mode 100644
index 0000000000000..7d9c9a90880ff
--- /dev/null
+++ b/clang/test/Sema/attr-counted-by-bounds-safety-vlas.c
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -fexperimental-bounds-safety -verify %s
+//
+// This is a portion of the `attr-counted-by-vla.c` test but is checked
+// under the semantics of `-fexperimental-bounds-safety` which has different
+// behavior.
+
+#define __counted_by(f) __attribute__((counted_by(f)))
+
+struct has_unannotated_VLA {
+ int count;
+ char buffer[];
+};
+
+struct has_annotated_VLA {
+ int count;
+ char buffer[] __counted_by(count);
+};
+
+struct buffer_of_structs_with_unnannotated_vla {
+ int count;
+ // expected-error@+1{{'counted_by' cannot be applied to an array with element of unknown size because 'struct has_unannotated_VLA' is a struct type with a flexible array member}}
+ struct has_unannotated_VLA Arr[] __counted_by(count);
+};
+
+
+struct buffer_of_structs_with_annotated_vla {
+ int count;
+ // expected-error@+1{{'counted_by' cannot be applied to an array with element of unknown size because 'struct has_annotated_VLA' is a struct type with a flexible array member}}
+ struct has_annotated_VLA Arr[] __counted_by(count);
+};
+
+struct buffer_of_const_structs_with_annotated_vla {
+ int count;
+ // Make sure the `const` qualifier is printed when printing the element type.
+ // expected-error@+1{{'counted_by' cannot be applied to an array with element of unknown size because 'const struct has_annotated_VLA' is a struct type with a flexible array member}}
+ const struct has_annotated_VLA Arr[] __counted_by(count);
+};
|
|
This is based on #70480 but removes the driver part of the change and makes the flag a CC1 flag only. |
delcypher
force-pushed
the
users/delcypher/bounds-safety-frontend-option
branch
from
cc6adf2 to
ef46dd5
Compare
delcypher
added
the
clang:bounds-safety
|
|
delcypher
force-pushed
the
users/delcypher/bounds-safety-frontend-option
branch
from
ef46dd5 to
ffe5280
Compare
This is a good question so let me first give you some context. When I originally landed #90786 (which allowed This downgrading of the error diagnostic to a warning is a departure from the intended semantics of To be clear about the behavior of To your point about proliferation of flags. Stepping back a bit. The downgraded error diagnostic is a temporary measure so eventually this diagnostic will be an error again in all cases ( |
Okay, let me make sure I'm on the same page -- what we've been calling |
|
@AaronBallman We haven't exposed the Does it sound like a reasonable strategy to you? Please let us know if you have other preference. |
Ah, thank you for the explanation! That sounds like a reasonable strategy to me; I was mostly worried that we'd be exposing two driver flags for this and it wasn't clear what the distinction was. But for a CC1 flag to enable testing and for early adopters, it seems reasonable. Thanks! |
…ption and use it to tweak `counted_by`'s semantics
This adds the `-fexperimental-bounds-safety` CC1 and corresponding
language option. This language option enables "-fbounds-safety" which
is a bounds-safety extension for C that is being incrementally
upstreamed.
This CC1 flag is not exposed as a driver flag yet because most of the
implementation isn't upstream yet.
The language option is used to make a small semantic change to how the
`counted_by` attribute is treated. Without
`-fexperimental-bounds-safety` the attribute is allowed (but emits a
warning) on a flexible array member where the element type is a struct
with a flexible array member. With the flag this situation is an error.
E.g.
```
struct has_unannotated_FAM {
int count;
char buffer[];
};
struct buffer_of_structs_with_unnannotated_FAM {
int count;
// Forbidden with `-fexperimental-bounds-safety`
struct has_unannotated_FAM Arr[] __counted_by(count);
};
```
The above code **should always** be an error. However, when llvm#90786 was
originally landed (which allowed `counted_by` to be used on pointers in
structs) it exposed an issue in code in the Linux kernel that was using
the `counted_by` attribute incorrectly (see
llvm#90786 (comment))
which was now caught by a new error diagnostic in the PR. To unbreak the
build of the Linux kernel the error diagnostic was temporarily
downgraded to be a warning to give the kernel authors time to fix their
code.
This downgrading of the error diagnostic to a warning is a departure
from the intended semantics of `-fbounds-safety` so in order to have
both behaviors (error and warning) it is necessary for Clang to actually
have a notion of `-fbounds-safety` being on vs off.
rdar://125400392
delcypher
force-pushed
the
users/delcypher/bounds-safety-frontend-option
branch
from
ffe5280 to
4af2708
Compare
|
@AaronBallman Thanks for approving. I've rebased this patch and tweaked the commit message to give the context for this change. Landing now. |
|
Sigh. I messed updating the commit message. I forgot that when GitHub does the squash it takes the message from the pull request description. |
…tion and use it to tweak `counted_by`'s semantics (#92623) Summary: This adds the `-fexperimental-bounds-safety` cc1 and corresponding language option. This language option enables "-fbounds-safety" which is a bounds-safety extension for C that is being incrementally upstreamed. This cc1 flag is not exposed as a driver flag yet because most of the implementation isn't upstream yet. The language option is used to make a small semantic change to how the `counted_by` attribute is treated. Without `-fexperimental-bounds-safety` the attribute is allowed (but emits a warning) on a flexible array member where the element type is a struct with a flexible array member. With the flag this situation is an error. E.g. ``` struct has_unannotated_FAM { int count; char buffer[]; }; struct buffer_of_structs_with_unnannotated_FAM { int count; // Forbidden with `-fexperimental-bounds-safety` struct has_unannotated_FAM Arr[] __counted_by(count); }; ``` rdar://125400392 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251547
4 participants
This adds the
-fexperimental-bounds-safetycc1 and corresponding language option. This language option enables "-fbounds-safety" which is a bounds-safety extension for C that is being incrementally upstreamed.This cc1 flag is not exposed as a driver flag yet because most of the implementation isn't upstream yet.
The language option is used to make a small semantic change to how the
counted_byattribute is treated. Without-fexperimental-bounds-safetythe attribute is allowed (but emits a warning) on a flexible array member where the element type is a struct with a flexible array member. With the flag this situation is an error.E.g.
The above code should always be an error. However, when #90786 was
originally landed (which allowed
counted_byto be used on pointers instructs) it exposed an issue in code in the Linux kernel that was using
the
counted_byattribute incorrectly (see#90786 (comment))
which was now caught by a new error diagnostic in the PR. To unbreak the
build of the Linux kernel the error diagnostic was temporarily
downgraded to be a warning to give the kernel authors time to fix their
code.
This downgrading of the error diagnostic to a warning is a departure
from the intended semantics of
-fbounds-safetyso in order to haveboth behaviors (error and warning) it is necessary for Clang to actually
have a notion of
-fbounds-safetybeing on vs off.rdar://125400392