[BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics #92623

delcypher · 2024-05-18T00:28:33Z

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 #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
#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

llvmbot · 2024-05-18T00:29:03Z

@llvm/pr-subscribers-clang

Author: Dan Liew (delcypher)

Changes

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);
};

Full diff: https://github.com/llvm/llvm-project/pull/92623.diff

4 Files Affected:

(modified) clang/include/clang/Basic/LangOptions.def (+2)
(modified) clang/include/clang/Driver/Options.td (+8)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+1-1)
(added) clang/test/Sema/attr-counted-by-bounds-safety-vlas.c (+37)

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);
+};

delcypher · 2024-05-18T00:29:05Z

This is based on #70480 but removes the driver part of the change and makes the flag a CC1 flag only.

delcypher · 2024-05-23T01:32:14Z

~~This is blocked by #93121~~. No longer blocked

AaronBallman

I'd like to understand why we need a flag for this rather than changing the behavior of -fbounds-safety. (I'd like to avoid a proliferation of flags unless this flag is going to gate more changes in the near future.)

delcypher · 2024-05-31T19:12:12Z

@AaronBallman

I'd like to understand why we need a flag for this rather than changing the behavior of -fbounds-safety. (I'd like to avoid a proliferation of flags unless this flag is going to gate more changes in the near future.)

This is a good question so let me first give you some context.

When I originally landed #90786 (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 #90786 (comment)) which was now caught by an error diagnostic that I added. To unbreak the build of the Linux kernel I temporarily downgraded the error diagnostic to be an 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. This is the very first time in upstream where we have needed to distinguish between -fbounds-safety being on-vs-off. So this seemed like a good opportunity to upstream the bounds-safety CC1 flag which we will need to have upstream anyway for future patches.

To be clear about the behavior of -fbounds-safety. It doesn't need to be changed, it's already correct. It's existing uses of the counted_by attribute that are wrong.

To your point about proliferation of flags. -fexperimental-bounds-safety is the CC1 flag that guard the -fbounds-safety feature in our internal Clang (modulo the name, we don't have the experimental- prefix internally). It will need to exist as we continue upstream more of our implementation that depends on this flag.

Stepping back a bit. The downgraded error diagnostic is a temporary measure so eventually this diagnostic will be an error again in all cases (-fbounds-safety on or off). So adding the CC1 flag on the basis of something that's temporary might be considered questionable. If you really have a problem with this I can abandon this PR and we can land the -fexperimental-bounds-safety CC1 flag later when it is needed to guard something more permanent.

AaronBallman · 2024-06-18T14:26:31Z

To your point about proliferation of flags. -fexperimental-bounds-safety is the CC1 flag that guard the -fbounds-safety feature in our internal Clang (modulo the name, we don't have the experimental- prefix internally). It will need to exist as we continue upstream more of our implementation that depends on this flag.

Okay, let me make sure I'm on the same page -- what we've been calling -fbounds-safety but never actually exposed as a CC1 or driver option is actually going to be exposed under the name -fexperimental-bounds-safety as a CC1 option? So there's not going to be two flags for users to have to distinguish between?

rapidsna · 2024-06-18T17:31:57Z

@AaronBallman We haven't exposed the -fbounds-safety flag yet. The idea was to guard it under the experimental flag called -fexperimental-bounds-safety as a CC1 flag for testing until we have the full feature available (the feature is work-in-progress and is going to take some time to implement). Then, we would create -fbounds-safety to be exposed as driver and CC1 flags. We would deprecate -fexperimental-bounds-safety after that point but might have to keep as an alias of -fbounds-safety for a release so people can switch over to -fbounds-safety.

Does it sound like a reasonable strategy to you? Please let us know if you have other preference.

AaronBallman · 2024-06-20T16:32:28Z

@AaronBallman We haven't exposed the -fbounds-safety flag yet. The idea was to guard it under the experimental flag called -fexperimental-bounds-safety as a CC1 flag for testing until we have the full feature available (the feature is work-in-progress and is going to take some time to implement). Then, we would create -fbounds-safety to be exposed as driver and CC1 flags. We would deprecate -fexperimental-bounds-safety after that point but might have to keep as an alias of -fbounds-safety for a release so people can switch over to -fbounds-safety.

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!

AaronBallman

LGTM; when we add -fbounds-safety in the future, we may want the language option to become an enumeration so we can distinguish between the experimental, non-experimental, and disabled bounds safety options, but we can cross that bridge when we get to it.

…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 · 2024-07-19T11:34:51Z

@AaronBallman Thanks for approving. I've rebased this patch and tweaked the commit message to give the context for this change. Landing now.

delcypher · 2024-07-19T11:38:47Z

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

delcypher requested review from kees, AaronBallman, bwendling, erichkeane, hnrklssn, rapidsna, patrykstefanski and apple-fcloutier May 18, 2024 00:28

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 18, 2024

delcypher force-pushed the users/delcypher/bounds-safety-frontend-option branch from cc6adf2 to ef46dd5 Compare May 18, 2024 00:30

delcypher added the clang:bounds-safety Issue/PR relating to the experimental -fbounds-safety feature in Clang label May 18, 2024

delcypher force-pushed the users/delcypher/bounds-safety-frontend-option branch from ef46dd5 to ffe5280 Compare May 24, 2024 17:56

AaronBallman reviewed May 31, 2024

View reviewed changes

AaronBallman approved these changes Jun 20, 2024

View reviewed changes

delcypher force-pushed the users/delcypher/bounds-safety-frontend-option branch from ffe5280 to 4af2708 Compare July 19, 2024 11:30

delcypher merged commit 6da23b6 into llvm:main Jul 19, 2024
4 of 7 checks passed

delcypher mentioned this pull request Jul 19, 2024

[clang][NFC] Move Bounds Safety Sema code to SemaBoundsSafety.cpp #99330

Merged

rapidsna mentioned this pull request Aug 27, 2024

[Driver][BoundsSafety] Add -fexperimental-bounds-safety flag #70480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics #92623

[BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics #92623

Uh oh!

delcypher commented May 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 18, 2024

Uh oh!

delcypher commented May 18, 2024

Uh oh!

delcypher commented May 23, 2024 •

edited

Loading

Uh oh!

AaronBallman left a comment

Uh oh!

delcypher commented May 31, 2024

Uh oh!

AaronBallman commented Jun 18, 2024

Uh oh!

rapidsna commented Jun 18, 2024 •

edited

Loading

Uh oh!

AaronBallman commented Jun 20, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

delcypher commented Jul 19, 2024

Uh oh!

Uh oh!

delcypher commented Jul 19, 2024

Uh oh!

Uh oh!

[BoundsSafety] Add -fexperimental-bounds-safety CC1 and language option and use it to tweak counted_by's semantics #92623

[BoundsSafety] Add -fexperimental-bounds-safety CC1 and language option and use it to tweak counted_by's semantics #92623

Uh oh!

Conversation

delcypher commented May 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 18, 2024

Uh oh!

delcypher commented May 18, 2024

Uh oh!

delcypher commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

delcypher commented May 31, 2024

Uh oh!

AaronBallman commented Jun 18, 2024

Uh oh!

rapidsna commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Jun 20, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

delcypher commented Jul 19, 2024

Uh oh!

Uh oh!

delcypher commented Jul 19, 2024

Uh oh!

Uh oh!

[BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics #92623

[BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics #92623

delcypher commented May 18, 2024 •

edited

Loading

delcypher commented May 23, 2024 •

edited

Loading

rapidsna commented Jun 18, 2024 •

edited

Loading