[clang] [clang][NFC] Move Bounds Safety Sema code to `SemaBoundsSafety.cpp` (PR #99330)
https://github.com/delcypher closed https://github.com/llvm/llvm-project/pull/99330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][NFC] Move Bounds Safety Sema code to `SemaBoundsSafety.cpp` (PR #99330)
delcypher wrote: Rebased due to the conflict caused by landing #92623 https://github.com/llvm/llvm-project/pull/99330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][NFC] Move Bounds Safety Sema code to `SemaBoundsSafety.cpp` (PR #99330)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/99330
>From 951a2a3bdf2857e789cf484caf9c27633a715cc8 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 17 Jul 2024 14:53:52 +0100
Subject: [PATCH] [Bounds Safety][NFC] Move Bounds Safety Sema code to
`SemaBoundsSafety.cpp`
This patch adds a new `SemaBoundsSafety.cpp` source file and moves the
existing `CheckCountedByAttrOnField` function and related helper
functions and types from `SemaDeclAttr.cpp` into the new source file.
The `CheckCountedByAttrOnField` function is now a method on the Sema
class and now has doxygen comments.
The goal behind this refactor is to clearly separate the
`-fbounds-safety` Sema code from everything else.
Although `counted_by(_or_null)` and `sized_by(_or_null)` attributes have
a meaning outside of `-fbounds-safety` it seems reasonable to also have
the Sema logic live in `SemaBoundsSafety.cpp` since the intention is
that the attributes will have the same semantics (but not necessarily
the same enforcement).
As `-fbounds-safety` is upstreamed additional Sema checks will be
added to `SemaBoundsSafety.cpp`.
rdar://131777237
---
clang/include/clang/Sema/Sema.h | 38 ++
clang/lib/Sema/CMakeLists.txt | 1 +
clang/lib/Sema/SemaBoundsSafety.cpp | 193
clang/lib/Sema/SemaDeclAttr.cpp | 177 +
4 files changed, 233 insertions(+), 176 deletions(-)
create mode 100644 clang/lib/Sema/SemaBoundsSafety.cpp
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 3cb1aa935fe46..d638d31e050dc 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -15051,6 +15051,44 @@ class Sema final : public SemaBase {
void ProcessAPINotes(Decl *D);
///@}
+
+ //
+ //
+ // -
+ //
+ //
+
+ /// \name Bounds Safety
+ /// Implementations are in SemaBoundsSafety.cpp
+ ///@{
+public:
+ /// Check if applying the specified attribute variant from the "counted by"
+ /// family of attributes to FieldDecl \p FD is semantically valid. If
+ /// semantically invalid diagnostics will be emitted explaining the problems.
+ ///
+ /// \param FD The FieldDecl to apply the attribute to
+ /// \param E The count expression on the attribute
+ /// \param[out] Decls If the attribute is semantically valid \p Decls
+ /// is populated with TypeCoupledDeclRefInfo objects, each
+ /// describing Decls referred to in \p E.
+ /// \param CountInBytes If true the attribute is from the "sized_by" family
of
+ /// attributes. If the false the attribute is from
+ /// "counted_by" family of attributes.
+ /// \param OrNull If true the attribute is from the "_or_null" suffixed
family
+ /// of attributes. If false the attribute does not have the
+ /// suffix.
+ ///
+ /// Together \p CountInBytes and \p OrNull decide the attribute variant. E.g.
+ /// \p CountInBytes and \p OrNull both being true indicates the
+ /// `counted_by_or_null` attribute.
+ ///
+ /// \returns false iff semantically valid.
+ bool CheckCountedByAttrOnField(
+ FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl , bool CountInBytes,
+ bool OrNull);
+
+ ///@}
};
DeductionFailureInfo
diff --git a/clang/lib/Sema/CMakeLists.txt b/clang/lib/Sema/CMakeLists.txt
index 5934c8c30daf9..2cee4f5ef6e99 100644
--- a/clang/lib/Sema/CMakeLists.txt
+++ b/clang/lib/Sema/CMakeLists.txt
@@ -36,6 +36,7 @@ add_clang_library(clangSema
SemaAvailability.cpp
SemaBPF.cpp
SemaBase.cpp
+ SemaBoundsSafety.cpp
SemaCXXScopeSpec.cpp
SemaCast.cpp
SemaChecking.cpp
diff --git a/clang/lib/Sema/SemaBoundsSafety.cpp
b/clang/lib/Sema/SemaBoundsSafety.cpp
new file mode 100644
index 0..290c820938898
--- /dev/null
+++ b/clang/lib/Sema/SemaBoundsSafety.cpp
@@ -0,0 +1,193 @@
+//===-- SemaBoundsSafety.cpp - Bounds Safety specific routines-*- C++
-*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis functions specific to
`-fbounds-safety`
+/// (Bounds Safety) and also its attributes when used without `-fbounds-safety`
+/// (e.g. `counted_by`)
+///
+//===--===//
+#include "clang/Sema/Sema.h"
+
+namespace clang {
+
+static CountAttributedType::DynamicCountPointerKind
+getCountAttrKind(bool CountInBytes, bool OrNull) {
+ if (CountInBytes)
+return OrNull ? CountAttributedType::SizedByOrNull
+ : CountAttributedType::SizedBy;
+ return OrNull ? CountAttributedType::CountedByOrNull
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
delcypher wrote: @AaronBallman Thanks for approving. I've rebased this patch and tweaked the commit message to give the context for this change. Landing now. https://github.com/llvm/llvm-project/pull/92623 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/92623
>From 4af270878cb243832f5aeb47c785efa263ba8c78 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 17:15:48 -0700
Subject: [PATCH] [Bounds Safety] Add `-fexperimental-bounds-safety` CC1 and
language option 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 #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
https://github.com/llvm/llvm-project/pull/90786#issuecomment-2118416515)
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
---
clang/include/clang/Basic/LangOptions.def | 3 ++
clang/include/clang/Driver/Options.td | 8
clang/lib/Sema/SemaDeclAttr.cpp | 2 +-
.../Sema/attr-counted-by-bounds-safety-vlas.c | 37 +++
4 files changed, 49 insertions(+), 1 deletion(-)
create mode 100644 clang/test/Sema/attr-counted-by-bounds-safety-vlas.c
diff --git a/clang/include/clang/Basic/LangOptions.def
b/clang/include/clang/Basic/LangOptions.def
index 5ad9c1f24b7c5..a6f36b23f07dc 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -520,6 +520,9 @@ BENIGN_LANGOPT(CheckNew, 1, 0, "Do not assume C++ operator
new may not return NU
BENIGN_LANGOPT(CheckConstexprFunctionBodies, 1, 1,
"Emit diagnostics for a constexpr function body that can never "
"be used in a constant expression.")
+
+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 fecfa2f3c7dba..5229940ec8258 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1911,6 +1911,14 @@ def fapinotes_swift_version : Joined<["-"],
"fapinotes-swift-version=">,
MetaVarName<"">,
HelpText<"Specify the Swift version to use when filtering API notes">;
+defm bounds_safety : BoolFOption<
+ "experimental-bounds-safety",
+ LangOpts<"BoundsSafety">, DefaultFalse,
+ PosFlag,
+ NegFlag,
+ BothFlags<[], [CC1Option],
+ " experimental bounds safety extension for C">>;
+
defm addrsig : BoolFOption<"addrsig",
CodeGenOpts<"Addrsig">, DefaultFalse,
PosFlag,
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 41295bfb3b94f..f2f3bfcd09035 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5948,7 +5948,7 @@ CheckCountedByAttrOnField(Sema , FieldDecl *FD, Expr *E,
} 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 0..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
[clang] [clang][NFC] Move Bounds Safety Sema code to `SemaBoundsSafety.cpp` (PR #99330)
delcypher wrote: @AaronBallman @Endilll Thanks for approving. I'll wait a bit for some of the code owners to chime in before landing this. @rapidsna @hnrklssn @bwendling @kees Please let me know if you have any concerns about this refactor. https://github.com/llvm/llvm-project/pull/99330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
https://github.com/delcypher closed https://github.com/llvm/llvm-project/pull/98954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
delcypher wrote: Thanks all for the clarification. Closing this PR in favor of #99330 https://github.com/llvm/llvm-project/pull/98954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Move Bounds Safety Sema code to `SemaBoundsSafety.cpp` (PR #99330)
https://github.com/delcypher created
https://github.com/llvm/llvm-project/pull/99330
This patch adds a new `SemaBoundsSafety.cpp` source file and moves the existing
`CheckCountedByAttrOnField` function and related helper functions and types
from `SemaDeclAttr.cpp` into the new source file. The
`CheckCountedByAttrOnField` function is now a method on the Sema class and now
has doxygen comments.
The goal behind this refactor is to clearly separate the `-fbounds-safety` Sema
code from everything else.
Although `counted_by(_or_null)` and `sized_by(_or_null)` attributes have a
meaning outside of `-fbounds-safety` it seems reasonable to also have the Sema
logic live in `SemaBoundsSafety.cpp` since the intention is that the attributes
will have the same semantics (but not necessarily the same enforcement).
As `-fbounds-safety` is upstreamed additional Sema checks will be added to
`SemaBoundsSafety.cpp`.
rdar://131777237
>From 050c328e90109c3f8a2c98da8df50f2865d67095 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 17 Jul 2024 14:53:52 +0100
Subject: [PATCH] [Bounds Safety][NFC] Move Bounds Safety Sema code to
`SemaBoundsSafety.cpp`
This patch adds a new `SemaBoundsSafety.cpp` source file and moves the
existing `CheckCountedByAttrOnField` function and related helper
functions and types from `SemaDeclAttr.cpp` into the new source file.
The `CheckCountedByAttrOnField` function is now a method on the Sema
class and now has doxygen comments.
The goal behind this refactor is to clearly separate the
`-fbounds-safety` Sema code from everything else.
Although `counted_by(_or_null)` and `sized_by(_or_null)` attributes have
a meaning outside of `-fbounds-safety` it seems reasonable to also have
the Sema logic live in `SemaBoundsSafety.cpp` since the intention is
that the attributes will have the same semantics (but not necessarily
the same enforcement).
As `-fbounds-safety` is upstreamed additional Sema checks will be
added to `SemaBoundsSafety.cpp`.
rdar://131777237
---
clang/include/clang/Sema/Sema.h | 38 ++
clang/lib/Sema/CMakeLists.txt | 1 +
clang/lib/Sema/SemaBoundsSafety.cpp | 193
clang/lib/Sema/SemaDeclAttr.cpp | 177 +
4 files changed, 233 insertions(+), 176 deletions(-)
create mode 100644 clang/lib/Sema/SemaBoundsSafety.cpp
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 48dff1b76cc57..0850ed456bcf6 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -15046,6 +15046,44 @@ class Sema final : public SemaBase {
void ProcessAPINotes(Decl *D);
///@}
+
+ //
+ //
+ // -
+ //
+ //
+
+ /// \name Bounds Safety
+ /// Implementations are in SemaBoundsSafety.cpp
+ ///@{
+public:
+ /// Check if applying the specified attribute variant from the "counted by"
+ /// family of attributes to FieldDecl \p FD is semantically valid. If
+ /// semantically invalid diagnostics will be emitted explaining the problems.
+ ///
+ /// \param FD The FieldDecl to apply the attribute to
+ /// \param E The count expression on the attribute
+ /// \param[out] Decls If the attribute is semantically valid \p Decls
+ /// is populated with TypeCoupledDeclRefInfo objects, each
+ /// describing Decls referred to in \p E.
+ /// \param CountInBytes If true the attribute is from the "sized_by" family
of
+ /// attributes. If the false the attribute is from
+ /// "counted_by" family of attributes.
+ /// \param OrNull If true the attribute is from the "_or_null" suffixed
family
+ /// of attributes. If false the attribute does not have the
+ /// suffix.
+ ///
+ /// Together \p CountInBytes and \p OrNull decide the attribute variant. E.g.
+ /// \p CountInBytes and \p OrNull both being true indicates the
+ /// `counted_by_or_null` attribute.
+ ///
+ /// \returns false iff semantically valid.
+ bool CheckCountedByAttrOnField(
+ FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl , bool CountInBytes,
+ bool OrNull);
+
+ ///@}
};
DeductionFailureInfo
diff --git a/clang/lib/Sema/CMakeLists.txt b/clang/lib/Sema/CMakeLists.txt
index 5934c8c30daf9..2cee4f5ef6e99 100644
--- a/clang/lib/Sema/CMakeLists.txt
+++ b/clang/lib/Sema/CMakeLists.txt
@@ -36,6 +36,7 @@ add_clang_library(clangSema
SemaAvailability.cpp
SemaBPF.cpp
SemaBase.cpp
+ SemaBoundsSafety.cpp
SemaCXXScopeSpec.cpp
SemaCast.cpp
SemaChecking.cpp
diff --git a/clang/lib/Sema/SemaBoundsSafety.cpp
b/clang/lib/Sema/SemaBoundsSafety.cpp
new file mode 100644
index 0..5bb1c8b276c85
--- /dev/null
+++ b/clang/lib/Sema/SemaBoundsSafety.cpp
@@ -0,0 +1,193 @@
+//===-- SemaBoundsSafety.cpp - Bounds Safety specific routines-*- C++
-*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
delcypher wrote: > The separations we've been making so far in Sema have been at a higher level > of granularity than this proposal. Vlad was calling it "language extensions" > but perhaps a different way to phrase it would be "unique language dialects". > e.g., Objective-C is its own language, but it's technically an extension of > C. Similar for things like OpenMP, etc. I don't think bounds safety is the > same kind of extension in that regard; it's a handful of features allowing > for extra diagnostic checking more than it's a unique language. Thanks for clarifying. I would describe`-fbounds-safety` as a lot more than extra diagnostic checking, it also injects runtime checks and its programming model adds restrictions that make C feel quite different. I'm not really sure that counts as a dialect but I guess it doesn't matter now given that I'll be going with a different approach. Out of curiosity, how does the current division of Sema fit into the "unique language dialect" classification? I've noticed there are a bunch of architecture specific Sema classes (e.g. `SemaRISCV`, `SemaX86`) and those don't really fit the classification. > For example, thread safety analysis can be described in exactly the same > way, so should it be split off too? If we keep doing that, will splitting > semantic object by language feature scale? What's the criteria for when a > language feature should or should not be split? I think we can side step all > of this for right now. Interesting question. I guess it depends on the size of the language feature and given that almost none of `-fbounds-safety` is currently upstream it is very difficult for you to make an informed decision in this particular case. So, yes let's side step this for now. We can always revisit it once a significant portion of `-fbounds-safety` is upstream. > The goal is to provide some layering within Sema to help break it up more. > e.g., the base layer is C and C++ needs, but then there's a layer for > Objective-C needs, a different layer for OpenMP needs, etc. This helps make > it more clear where dependencies lie between the large-scale different > semantic components. Bounds safety isn't really a "layer". Does that make > some sense? Mostly, but I have some doubts about the "Bounds safety isn't really a layer" part but let's delay dealing with that until more of the implementation is upstream. https://github.com/llvm/llvm-project/pull/98954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
delcypher wrote: > > As noted above `-fbounds-safety` **is a C language extension** which makes > > it seem like it would fit nicely into the existing division of Sema into > > multiple objects and relevant source files. > > No, it doesn't fit nicely into the division, which is the reason we're having > this discussion. If we don't agree on this then I must not fully understand what the criteria is for dividing Sema current is. > You can have `SemaBoundsSafety.cpp` and your own "section" inside `Sema.h`, > like C++ features do (like templates or lambdas). There's no reason to make > separation physical by introducing `SemaBoundsSafety` class from the get-go. I'm ok with this. Having the implementation in a separate file is where the main benefit lies. The separation into a separation into a separate class is a nice-to-have and ok to drop doing that. > That's why I propose to follow long-established practice of doing > `SemaBoundsSafety.cpp`, and move that around later. What I'd like to evaluate > before deciding on `SemaBoundsChecking` is how big its interface is (what > would be exposed via `SemaBoundsChecking` class,) Sure. Let's go with that approach then. https://github.com/llvm/llvm-project/pull/98954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
delcypher wrote: > "Extension" is definitely quite broad. What I meant are (basically) languages > that are based off C or C++. Would you argue that `-fbounds-safety` fits into > a set of OpenMP, OpenACC, CUDA, HLSL, Objective-C, and Swift? In my opinion it fits in the set because it is a (pretty large) C language extension. > > It's currently in the process of being upstreamed which is why it's small, > > but it'd be easier to split now than wait until it's reached a certain > > size, if we do want to split it eventually. > > I did that a lot with other parts of `Sema`, and it's not that hard, unless > it grows comparable to `SemaExprCXX.cpp` (which I consider unlikely to > happen). I'd rather see this being upstreamed into the existing `Sema` > structure as it has been planned all along, and use it as an input for the > design of further `Sema` splitting, rather than committing today that > `SemaBoundsSafety` is going to be a thing. I don't agree with this approach. I outlined above why I think it makes a lot of sense to keep the `-fbounds-safety` Sema code in its own source file and blocking doing that on a _potential future refactor_ that might never happen doesn't seem like a good idea to me. If at some point we come up with some new design for Sema we can easily move the `-fbounds-safety` code out of its own source file (and class) and into the relevant locations required by the redesign. If the `-fbounds-safety` code is littered all over the other Sema files the "potential future refactor" could be much harder because finding all `-fbound-safety` code now is much more time consuming. https://github.com/llvm/llvm-project/pull/98954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
delcypher wrote: > Unfortunately, I think this should be held off until we have a bigger picture > for the future of `Sema`. I'll elaborate below. > > If you take a close look at the existing set of `Sema` parts, it's almost > entirely comprised of other languages or language extensions (from C and C++ > standpoint) and backends. They were considered natural enough to be split off > `Sema`, which, among other things, means that it's easy to explain their > place in the big picture, like I did in the previous sentence. As noted above `-fbounds-safety` **is a C language extension** which makes it seem like it would fit nicely into the existing division of Sema into multiple objects and relevant source files. > I don't think this patch makes it easier to explain people where things are > in this codebase, because (I think) `counted_by` is a declaration attribute, > and we already have place for them. Having only one function also makes it a > very small part of `Sema`, which doesn't sound compelling. It doesn't right now because most of `-fbounds-safety` implementation isn't upstream yet. I simply moved what is **currently upstream**. As we (Apple) upstream more and more of the implementation it makes a lot sense to try to keep as much of it in its own source file to: * Create a clean separation between `-fbounds-safety` Sema checks and everything else. This will ultimately make exploring the code easier once a significant amount of our implementation has been upstreamed. * Avoid growing all the existing `Sema*.cpp` files unnecessarily. They are already huge and we don't want to make the problem worse when there's an easy option available to avoid that. > A bigger picture this fits in would be a compelling argument (like it was for > small backend parts), but to my knowledge there's none. The `counted_by` attribute is just one of several attributes that are part of the `-fbounds-safety` language extension which is documented here https://clang.llvm.org/docs/BoundsSafety.html. That's "bigger picture" for the feature, or did you mean something else by "bigger picture this fits in"? > Coming up with one and getting maintainers on board is certainly out of scope > of this PR, so don't feel obligated to do that. Hopefully I'll get back to > this whole topic later. > My end goal is to have "somewhere" to put all of the Sema code that support `-fbounds-safety` as we upstream it. I think it's very much preferable to have this code go into its own source file and header file for the reasons I gave above. We don't have to use the sub-class `SemaBase` mechanism that I've used here if this is really a problem. However, the mechanism does seem like a good fit for `-fbounds-safety` and would be a shame not to use it. https://github.com/llvm/llvm-project/pull/98954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)
https://github.com/delcypher created
https://github.com/llvm/llvm-project/pull/98954
This patch creates a `SemaBoundsSafety` class to create a clean
separation between `-fbounds-safety` Sema checks and
other Sema checks. A `SemaBoundsSafety` object is available via the
`Sema::BoundsSafety()` method, similar to how other Sema checks have
been seperated out (e.g. `SemaSwift`).
The existing `CheckCountedByAttrOnField` function and related helper
functions and types from `SemaDeclAttr.cpp` has been moved into
`SemaBoundsSafety`. Although `counted_by(_or_null)` and
`sized_by(_or_null)` attributes have a meaning outside of
`-fbounds-safety` it seems reasonable to also have the Sema logic live
in `SemaBoundsSafety` since the intention is that the attributes will
have the same semantics (but not necessarily the same enforcement).
As `-fbounds-safety` is upstreamed additional Sema checks will be
added to the `SemaBoundsSafety` class.
rdar://131777237
>From 15e0f8f67a68bc9c36ee476491973d2d50f92c7e Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Mon, 15 Jul 2024 20:45:30 +0100
Subject: [PATCH] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move
existing Sema checks there
This patch creates a `SemaBoundsSafety` class to create a clean
separation between `-fbounds-safety` Sema checks and
other Sema checks. A `SemaBoundsSafety` object is available via the
`Sema::BoundsSafety()` method, similar to how other Sema checks have
been seperated out (e.g. `SemaSwift`).
The existing `CheckCountedByAttrOnField` function and related helper
functions and types from `SemaDeclAttr.cpp` has been moved into
`SemaBoundsSafety`. Although `counted_by(_or_null)` and
`sized_by(_or_null)` attributes have a meaning outside of
`-fbounds-safety` it seems reasonable to also have the Sema logic live
in `SemaBoundsSafety` since the intention is that the attributes will
have the same semantics (but not necessarily the same enforcement).
As `-fbounds-safety` is upstreamed additional Sema checks will be
added to the `SemaBoundsSafety` class.
rdar://131777237
---
clang/include/clang/Sema/Sema.h | 7 +
clang/include/clang/Sema/SemaBoundsSafety.h | 42 +
clang/lib/Sema/CMakeLists.txt | 1 +
clang/lib/Sema/Sema.cpp | 2 +
clang/lib/Sema/SemaBoundsSafety.cpp | 198
clang/lib/Sema/SemaDeclAttr.cpp | 179 +-
6 files changed, 253 insertions(+), 176 deletions(-)
create mode 100644 clang/include/clang/Sema/SemaBoundsSafety.h
create mode 100644 clang/lib/Sema/SemaBoundsSafety.cpp
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 48dff1b76cc57..81e7fed9f3f4c 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -173,6 +173,7 @@ class QualType;
class SemaAMDGPU;
class SemaARM;
class SemaAVR;
+class SemaBoundsSafety;
class SemaBPF;
class SemaCodeCompletion;
class SemaCUDA;
@@ -1151,6 +1152,11 @@ class Sema final : public SemaBase {
return *AVRPtr;
}
+ SemaBoundsSafety () {
+assert(BoundsSafetyPtr);
+return *BoundsSafetyPtr;
+ }
+
SemaBPF () {
assert(BPFPtr);
return *BPFPtr;
@@ -1294,6 +1300,7 @@ class Sema final : public SemaBase {
std::unique_ptr AMDGPUPtr;
std::unique_ptr ARMPtr;
std::unique_ptr AVRPtr;
+ std::unique_ptr BoundsSafetyPtr;
std::unique_ptr BPFPtr;
std::unique_ptr CodeCompletionPtr;
std::unique_ptr CUDAPtr;
diff --git a/clang/include/clang/Sema/SemaBoundsSafety.h
b/clang/include/clang/Sema/SemaBoundsSafety.h
new file mode 100644
index 0..22ac14807e66d
--- /dev/null
+++ b/clang/include/clang/Sema/SemaBoundsSafety.h
@@ -0,0 +1,42 @@
+//=== SemaBoundsSafety.h - Bounds Safety specific routines-*- C++
-*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis functions specific to
`-fbounds-safety`
+/// (Bounds Safety) and also its attributes when used without `-fbounds-safety`
+/// (e.g. `counted_by`)
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMABOUNDSSAFETY_H
+#define LLVM_CLANG_SEMA_SEMABOUNDSSAFETY_H
+
+#include "clang/Sema/SemaBase.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace clang {
+class CountAttributedType;
+class Decl;
+class Expr;
+class FieldDecl;
+class NamedDecl;
+class ParsedAttr;
+class TypeCoupledDeclRefInfo;
+
+class SemaBoundsSafety : public SemaBase {
+public:
+ SemaBoundsSafety(Sema );
+
+ bool CheckCountedByAttrOnField(
+ FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl , bool CountInBytes,
+ bool OrNull);
+};
+
+} // namespace clang
+
+#endif //
[clang] Support `guarded_by` attribute and related attributes inside C structs and support late parsing them (PR #95455)
https://github.com/delcypher closed https://github.com/llvm/llvm-project/pull/95455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support `guarded_by` attribute and related attributes inside C structs and support late parsing them (PR #95455)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/95455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support `guarded_by` attribute and related attributes inside C structs and support late parsing them (PR #95455)
https://github.com/delcypher approved this pull request. @pdherbemont Thanks for working on this LGTM. Please confirm that the comments that @aaronpuchert have been added because I couldn't see them. Once you've done that we can get this merged :) https://github.com/llvm/llvm-project/pull/95455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support `guarded_by` attribute and related attributes inside C structs and support late parsing them (PR #95455)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/95455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support `guarded_by` attribute and related attributes inside C structs and support late parsing them (PR #95455)
@@ -29,6 +30,22 @@ struct LOCKABLE Mutex {};
struct Foo {
struct Mutex *mu_;
+ int a_value GUARDED_BY(mu_);
+
+ struct Bar {
+struct Mutex *other_mu ACQUIRED_AFTER(mu_);
delcypher wrote:
@pdherbemont Did you add the comment? I can't find it in the diff shown on
GitHub
https://github.com/llvm/llvm-project/pull/95455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -0,0 +1,141 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define __counted_by_or_null(f) __attribute__((counted_by_or_null(f)))
+
+// This has been adapted from clang/test/Sema/attr-counted-by-vla.c, but with
VLAs replaced with pointers
+
+struct bar;
+
+struct not_found {
+ int count;
+ struct bar *ptr __counted_by_or_null(bork); // expected-error {{use of
undeclared identifier 'bork'}}
+};
+
+struct no_found_count_not_in_substruct {
+ unsigned long flags;
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct A {
+int dummy;
+int * ptr __counted_by_or_null(count); // expected-error
{{'counted_by_or_null' field 'count' isn't within the same struct as the
annotated pointer}}
delcypher wrote:
@hnrklssn Looks like you just modified the existing wording. I definitely think
it could be improved but we can do that in a separate patch if you prefer.
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -0,0 +1,141 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define __counted_by_or_null(f) __attribute__((counted_by_or_null(f)))
+
+// This has been adapted from clang/test/Sema/attr-counted-by-vla.c, but with
VLAs replaced with pointers
+
+struct bar;
+
+struct not_found {
+ int count;
+ struct bar *ptr __counted_by_or_null(bork); // expected-error {{use of
undeclared identifier 'bork'}}
+};
+
+struct no_found_count_not_in_substruct {
+ unsigned long flags;
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct A {
+int dummy;
+int * ptr __counted_by_or_null(count); // expected-error
{{'counted_by_or_null' field 'count' isn't within the same struct as the
annotated pointer}}
delcypher wrote:
Nit: This wording is a little odd. I would expect something like.
```
'counted_by_or_null' references field 'count' which isn't within the same
struct as the annotated pointer
```
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -425,6 +425,12 @@ Attribute Changes in Clang size_t count; }; +- The attributes ``sized_by``, ``counted_by_or_null`` and ``sized_by_or_null``` + have been added as variants on ``counted_by``, each with slightly different semantics. + ``sized_by`` takes a byte size parameter instead of an element count, allowing pointees + with unknown size. The ``counted_by_or_null`` and ``sized_by_or_null`` variants are equivalent + to their base variants, except the pointer can be null regardless of count/size value. delcypher wrote: Nit: Maybe it's worth calling out (because it's really not obvious) that the only valid count for `__sized_by` when the pointer is `NULL` is `0`, `__sized_by_or_null` allows **any** count value when the pointer is `NULL`. We could also note that this isn't currently enforced. https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8320,6 +8320,15 @@ static const RecordDecl
*GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
return RD;
}
+static CountAttributedType::DynamicCountPointerKind
+getCountAttrKind(bool CountInBytes, bool OrNull) {
delcypher wrote:
Nit. Should this be a class method on `CountAttributedType`?
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
https://github.com/delcypher approved this pull request. Thanks for addressing my comment. I have some nits but I'll leave it to your discretion whether or not you fix them. https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add support for builtin_verbose_trap (PR #79230)
delcypher wrote: @ahatanak Excited to see this finally land https://github.com/llvm/llvm-project/pull/79230 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8697,9 +8708,10 @@ static bool CheckCountedByAttrOnField(
InvalidTypeKind = CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER;
}
- if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID) {
+ if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID &&
+ !CountInBytes) {
delcypher wrote:
> I think that is a reasonable restriction to add in -fbounds-safety, but
I think we should avoid bifurcating the semantics of `sized_by` based on the
`-fbounds-safety` flag where possible. The more differences we have the more
pain we are going to cause during adoption. Consider a project that first
annotates their headers with `sized_by` but doesn't pass `-fbounds-safety`.
They will get one set of semantics, then a project that consumes that header
file builds with `-fbounds-safety`. That project is built with different
semantics. If the semantics differ enough its possible that the header file
will build fine without `-fbound-safety` but fail to build with it.
> but for simply saying "this is the size", is it really an issue?
The issue for me is that it doesn't really make sense to have pointers to
"sizeless types" (not to be confused with "incomplete types") or to "function
types" in the first place and so to me that implies adding `sized_by` to those
pointers doesn't make sense either.
I don't want to delay this PR for too long so if you think my ask is
unreasonable we can file an issue and get back to it.
Ultimately I think @rapidsna should make the call on this issue.
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
https://github.com/delcypher requested changes to this pull request. I'm requesting changes for now but we can change this based on @rapidsna 's opinion. https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
delcypher wrote: @pdherbemont Could you take a look at this? https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
delcypher wrote: Reverted in c9d580033f29196223cd56a0fa238c3ba51d23e4 https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] c9d5800 - Revert "Support `guarded_by` attribute and related attributes inside C structs and support late parsing them (#94216)"
Author: Dan Liew
Date: 2024-06-12T15:43:12-07:00
New Revision: c9d580033f29196223cd56a0fa238c3ba51d23e4
URL:
https://github.com/llvm/llvm-project/commit/c9d580033f29196223cd56a0fa238c3ba51d23e4
DIFF:
https://github.com/llvm/llvm-project/commit/c9d580033f29196223cd56a0fa238c3ba51d23e4.diff
LOG: Revert "Support `guarded_by` attribute and related attributes inside C
structs and support late parsing them (#94216)"
This reverts commit af0d7128c8fd053d3de8af208d7d1682bc7a525a.
Reverting due to likely regression:
https://github.com/llvm/llvm-project/pull/94216#issuecomment-2164013300
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/docs/ThreadSafetyAnalysis.rst
clang/include/clang/Basic/Attr.td
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/Sema.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/AST/ast-dump-color.cpp
clang/test/Sema/warn-thread-safety-analysis.c
clang/test/SemaCXX/warn-thread-safety-parsing.cpp
Removed:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f17603a161668..cf1ba02cbc4b2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -471,10 +471,6 @@ Attribute Changes in Clang
size_t count;
};
-- The ``guarded_by``, ``pt_guarded_by``, ``acquired_after``,
``acquired_before``
- attributes now support referencing struct members in C. The arguments are
also
- now late parsed when ``-fexperimental-late-parse-attributes`` is passed like
- for ``counted_by``.
Improvements to Clang's diagnostics
---
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 6eefa306e3c89..dcde0c706c704 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -764,6 +764,12 @@ doesn't know that munl.mu == mutex. The SCOPED_CAPABILITY
attribute handles
aliasing for MutexLocker, but does so only for that particular pattern.
+ACQUIRED_BEFORE(...) and ACQUIRED_AFTER(...) are currently unimplemented.
+-
+
+To be fixed in a future update.
+
+
.. _mutexheader:
mutex.h
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 6032b934279dd..b70b0c8b836a5 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3633,7 +3633,7 @@ def NoThreadSafetyAnalysis : InheritableAttr {
def GuardedBy : InheritableAttr {
let Spellings = [GNU<"guarded_by">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = LateAttrParseExperimentalExt;
+ let LateParsed = LateAttrParseStandard;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3644,7 +3644,7 @@ def GuardedBy : InheritableAttr {
def PtGuardedBy : InheritableAttr {
let Spellings = [GNU<"pt_guarded_by">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = LateAttrParseExperimentalExt;
+ let LateParsed = LateAttrParseStandard;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3655,7 +3655,7 @@ def PtGuardedBy : InheritableAttr {
def AcquiredAfter : InheritableAttr {
let Spellings = [GNU<"acquired_after">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = LateAttrParseExperimentalExt;
+ let LateParsed = LateAttrParseStandard;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3666,7 +3666,7 @@ def AcquiredAfter : InheritableAttr {
def AcquiredBefore : InheritableAttr {
let Spellings = [GNU<"acquired_before">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = LateAttrParseExperimentalExt;
+ let LateParsed = LateAttrParseStandard;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
diff --git a/clang/include/clang/Parse/Parser.h
b/clang/include/clang/Parse/Parser.h
index 9e11078382756..d054b8cf0d240 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3128,20 +3128,6 @@ class Parser : public CodeCompletionHandler {
IdentifierInfo *ScopeName, SourceLocation ScopeLoc,
ParsedAttr::Form Form);
- void ParseGuardedByAttribute(IdentifierInfo ,
- SourceLocation AttrNameLoc,
- ParsedAttributes ,
- IdentifierInfo *ScopeName,
- SourceLocation ScopeLoc, SourceLocation *EndLoc,
- ParsedAttr::Form Form);
-
- void ParseAcquiredAttribute(IdentifierInfo ,
- SourceLocation AttrNameLoc,
-
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
delcypher wrote: @aeubanks I'll revert. Is this example C or C++? https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
delcypher wrote: @pdherbemont I've landed on your behalf. I tweaked the commit message to be more descriptive of the change we finally landed and noted that you are the author of this patch. Thanks again for working on this :) https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
https://github.com/delcypher closed https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
https://github.com/delcypher approved this pull request. Thanks for fixing my requested changes. LGTM https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -74,6 +83,15 @@ int get_value(int *p) SHARED_LOCKS_REQUIRED(foo_.mu_){
void unlock_scope(struct Mutex *const *mu)
__attribute__((release_capability(**mu)));
+// Verify late parsing:
+#ifdef LATE_PARSING
+struct LateParsing {
+ int a_value_defined_before GUARDED_BY(a_mutex_defined_late);
+ struct Mutex *a_mutex_defined_late ACQUIRED_AFTER(a_mutex_defined_very_late);
+ struct Mutex *a_mutex_defined_very_late;
delcypher wrote:
@pdherbemont Also I can't see any uses of the `LateParsing` struct. It might be
worth testing the attributes were parsed. You could do that as a separate AST
test or use the `LateParsing` struct in a way that will make the thread safety
analysis emit warnings.
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
https://github.com/delcypher requested changes to this pull request. https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -74,6 +83,15 @@ int get_value(int *p) SHARED_LOCKS_REQUIRED(foo_.mu_){
void unlock_scope(struct Mutex *const *mu)
__attribute__((release_capability(**mu)));
+// Verify late parsing:
+#ifdef LATE_PARSING
+struct LateParsing {
+ int a_value_defined_before GUARDED_BY(a_mutex_defined_late);
+ struct Mutex *a_mutex_defined_late ACQUIRED_AFTER(a_mutex_defined_very_late);
+ struct Mutex *a_mutex_defined_very_late;
delcypher wrote:
@pdherbemont I think you're missing a use of `pt_guarded_by` and
`acquired_before` being late parsed here.
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta %s +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -fexperimental-late-parse-attributes %s delcypher wrote: @pdherbemont We should probably have a test case that runs without `-fexperimental-late-parse-attributes` so that we check normal parsing still works, especially given that its the default. https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -3330,6 +3340,112 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
}
}
+/// GuardedBy attributes (e.g., guarded_by):
+/// AttrName '(' expression ')'
+void Parser::ParseGuardedByAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+Parens.consumeClose();
+return;
+ }
+
+ ArgsVector ArgExprs;
+ // Don't evaluate argument when the attribute is ignored.
+ using ExpressionKind =
+ Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+ EnterExpressionEvaluationContext EC(
+ Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
nullptr,
+ ExpressionKind::EK_AttrArgument);
+
+ ExprResult ArgExpr(
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+ if (ArgExpr.isInvalid()) {
+Parens.skipToEnd();
+return;
+ }
+
+ ArgExprs.push_back(ArgExpr.get());
+
+ auto RParens = Tok.getLocation();
+ auto =
+ *Attrs.addNew(, SourceRange(AttrNameLoc, RParens), ScopeName,
+ScopeLoc, ArgExprs.data(), ArgExprs.size(), Form);
+
+ if (EndLoc)
+*EndLoc = RParens;
+
+ if (!Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_attribute_wrong_number_arguments)
delcypher wrote:
@pdherbemont Could you add a test case for this path?
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -3330,6 +3340,112 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
}
}
+/// GuardedBy attributes (e.g., guarded_by):
+/// AttrName '(' expression ')'
+void Parser::ParseGuardedByAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+Parens.consumeClose();
+return;
+ }
+
+ ArgsVector ArgExprs;
+ // Don't evaluate argument when the attribute is ignored.
+ using ExpressionKind =
+ Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+ EnterExpressionEvaluationContext EC(
+ Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
nullptr,
+ ExpressionKind::EK_AttrArgument);
+
+ ExprResult ArgExpr(
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+ if (ArgExpr.isInvalid()) {
+Parens.skipToEnd();
+return;
+ }
+
+ ArgExprs.push_back(ArgExpr.get());
+
+ auto RParens = Tok.getLocation();
+ auto =
+ *Attrs.addNew(, SourceRange(AttrNameLoc, RParens), ScopeName,
+ScopeLoc, ArgExprs.data(), ArgExprs.size(), Form);
+
+ if (EndLoc)
+*EndLoc = RParens;
+
+ if (!Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_attribute_wrong_number_arguments)
+<< AL << 1;
+Parens.skipToEnd();
+return;
+ }
+
+ Parens.consumeClose();
+}
+
+/// Acquired attributes (e.g., acquired_before, acquired_after):
+/// AttrName '(' expression-list ')'
+void Parser::ParseAcquiredAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+Parens.consumeClose();
+return;
+ }
+
+ auto ArgStart = Tok.getLocation();
+
+ ArgsVector ArgExprs;
+
+ do {
+
+// Don't evaluate argument when the attribute is ignored.
+using ExpressionKind =
+Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+EnterExpressionEvaluationContext EC(
+Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
+nullptr, ExpressionKind::EK_AttrArgument);
+
+ExprResult ArgExpr(
+Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+if (ArgExpr.isInvalid()) {
+ Parens.skipToEnd();
delcypher wrote:
@pdherbemont Could you add a test case for this path?
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -3330,6 +3340,112 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
}
}
+/// GuardedBy attributes (e.g., guarded_by):
+/// AttrName '(' expression ')'
+void Parser::ParseGuardedByAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+Parens.consumeClose();
+return;
+ }
+
+ ArgsVector ArgExprs;
+ // Don't evaluate argument when the attribute is ignored.
+ using ExpressionKind =
+ Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+ EnterExpressionEvaluationContext EC(
+ Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
nullptr,
+ ExpressionKind::EK_AttrArgument);
+
+ ExprResult ArgExpr(
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+ if (ArgExpr.isInvalid()) {
+Parens.skipToEnd();
+return;
+ }
+
+ ArgExprs.push_back(ArgExpr.get());
+
+ auto RParens = Tok.getLocation();
+ auto =
+ *Attrs.addNew(, SourceRange(AttrNameLoc, RParens), ScopeName,
+ScopeLoc, ArgExprs.data(), ArgExprs.size(), Form);
+
+ if (EndLoc)
+*EndLoc = RParens;
+
+ if (!Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_attribute_wrong_number_arguments)
+<< AL << 1;
+Parens.skipToEnd();
+return;
+ }
+
+ Parens.consumeClose();
+}
+
+/// Acquired attributes (e.g., acquired_before, acquired_after):
+/// AttrName '(' expression-list ')'
+void Parser::ParseAcquiredAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+Parens.consumeClose();
delcypher wrote:
@pdherbemont Could you add a test case for this path?
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -3330,6 +3340,112 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
}
}
+/// GuardedBy attributes (e.g., guarded_by):
+/// AttrName '(' expression ')'
+void Parser::ParseGuardedByAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+Parens.consumeClose();
+return;
+ }
+
+ ArgsVector ArgExprs;
+ // Don't evaluate argument when the attribute is ignored.
+ using ExpressionKind =
+ Sema::ExpressionEvaluationContextRecord::ExpressionKind;
+ EnterExpressionEvaluationContext EC(
+ Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
nullptr,
+ ExpressionKind::EK_AttrArgument);
+
+ ExprResult ArgExpr(
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+ if (ArgExpr.isInvalid()) {
+Parens.skipToEnd();
delcypher wrote:
@pdherbemont Could you add a test case for this path?
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -3330,6 +3340,112 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
}
}
+/// GuardedBy attributes (e.g., guarded_by):
+/// AttrName '(' expression ')'
+void Parser::ParseGuardedByAttribute(
+IdentifierInfo , SourceLocation AttrNameLoc,
+ParsedAttributes , IdentifierInfo *ScopeName, SourceLocation
ScopeLoc,
+SourceLocation *EndLoc, ParsedAttr::Form Form) {
+ assert(Tok.is(tok::l_paren) && "Attribute arg list not starting with '('");
+
+ BalancedDelimiterTracker Parens(*this, tok::l_paren);
+ Parens.consumeOpen();
+
+ if (Tok.is(tok::r_paren)) {
+Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
delcypher wrote:
@pdherbemont Could you add a test case for this path?
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
@@ -29,6 +29,13 @@ struct LOCKABLE Mutex {};
struct Foo {
struct Mutex *mu_;
+ int a_value GUARDED_BY(mu_);
delcypher wrote:
@pdherbemont I think you should be checking **both** late and regular parsing
for all attributes that you're adding support for.
https://github.com/llvm/llvm-project/pull/94216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Support [[guarded_by(mutex)]] attribute inside C struct (PR #94216)
https://github.com/delcypher requested changes to this pull request. @pdherbemont thanks for working on this. It looks pretty good and its great to see the late parsing support I added gaining new users. I just have some nits about missing test cases. https://github.com/llvm/llvm-project/pull/94216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
delcypher wrote: @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 https://github.com/llvm/llvm-project/pull/90786#issuecomment-2118416515) 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. https://github.com/llvm/llvm-project/pull/92623 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8641,22 +8641,33 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-static bool CheckCountedByAttrOnField(
-Sema , FieldDecl *FD, Expr *E,
-llvm::SmallVectorImpl ) {
+static bool
+CheckCountedByAttrOnField(Sema , FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl ,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
+ unsigned Kind = CountInBytes;
+ if (OrNull)
+Kind += 2;
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
-<< FD->getSourceRange();
+<< Kind << FD->getSourceRange();
return true;
}
const auto FieldTy = FD->getType();
+ if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
delcypher wrote:
> > The diagnostic name is a little misleading here because CountInBytes
> > suggested __sized_by but the diagnostic name has counted_by in its name
>
> I kept it because it's the same family of attributes. Do you have a
> suggestion for a name that would imply that it's not just for `counted_by`,
> but more specific than `bounds_attribute`?
We use `CountAttributedType` to represent both `counted_by` and `sized_by` so
how about `err_count_attr_...` as the diagnostic prefix? I don't have strong
opinions on exactly what the name should be.
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8641,22 +8641,33 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-static bool CheckCountedByAttrOnField(
-Sema , FieldDecl *FD, Expr *E,
-llvm::SmallVectorImpl ) {
+static bool
+CheckCountedByAttrOnField(Sema , FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl ,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
+ unsigned Kind = CountInBytes;
+ if (OrNull)
+Kind += 2;
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
-<< FD->getSourceRange();
+<< Kind << FD->getSourceRange();
return true;
}
const auto FieldTy = FD->getType();
+ if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
delcypher wrote:
> > The OrNull case probably deserves its own special diagnostic because in
> > that case the diagnostic should explain that they cannot use the attribute
> > on arrays and that they need to use __counted_by instead.
>
> That is also true for `CountInBytes`. But I agree that it would be good for
> the diagnostic to suggest `counted_by`.
Hmm looking at it more closely for the
`err_counted_by_attr_not_on_ptr_or_flexible_array_member` means the diagnostic
output for
```
struct Test {
int count;
int fma[] __counted_by_or_null(count)
}
```
would look something like
```
counted_by_or_null only applies to pointers
```
i.e. mentioning flexible array members is dropped (I initially thought it
wasn't). So actually I guess this is fine. Suggesting the right attribute would
be an extra bonus. If you don't want to do it now file an issue (tagged with
clang:bounds-safety).
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8697,9 +8708,10 @@ static bool CheckCountedByAttrOnField(
InvalidTypeKind = CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER;
}
- if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID) {
+ if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID &&
+ !CountInBytes) {
delcypher wrote:
I would have thought we would want to prevent `sized_by` on all of the
conditions above (e.g. pointee being a function type) apart from pointee being
an incomplete type. However, it looks like this implementation allows pointee
types for all the cases we disallow it for `counted_by`.
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -425,6 +425,12 @@ Attribute Changes in Clang size_t count; }; +- The attributes ``sized_by``, ``counted_by_or_null`` and ``sized_by_or_null``` + have been added as variants on ``counted_by``, each with slightly different semantics. + ``sized_by`` takes a byte size parameter instead of an element count, allowing pointees + with unknown size. The ``counted_by_or_null`` and ``sized_by_or_null`` variants are equivalent + to their base variants, except the pointer can be null regardless of count/size value. delcypher wrote: Nit: The description of the behavior of `counted_by` with a `nullptr` is the `-fbound-safety` behavior. I'm not sure if that restriction exists for `counted_by` outside that mode. https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8641,22 +8641,33 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-static bool CheckCountedByAttrOnField(
-Sema , FieldDecl *FD, Expr *E,
-llvm::SmallVectorImpl ) {
+static bool
+CheckCountedByAttrOnField(Sema , FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl ,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
+ unsigned Kind = CountInBytes;
+ if (OrNull)
+Kind += 2;
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
-<< FD->getSourceRange();
+<< Kind << FD->getSourceRange();
return true;
}
const auto FieldTy = FD->getType();
+ if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
delcypher wrote:
Nit: The diagnostic name is a little misleading here because `CountInBytes`
suggested `__sized_by` but the diagnostic name has `counted_by` in its name
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8641,22 +8641,33 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-static bool CheckCountedByAttrOnField(
-Sema , FieldDecl *FD, Expr *E,
-llvm::SmallVectorImpl ) {
+static bool
+CheckCountedByAttrOnField(Sema , FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl ,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
+ unsigned Kind = CountInBytes;
+ if (OrNull)
+Kind += 2;
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
-<< FD->getSourceRange();
+<< Kind << FD->getSourceRange();
return true;
}
const auto FieldTy = FD->getType();
+ if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
delcypher wrote:
The `OrNull` case probably deserves its own special diagnostic because in that
case the diagnostic should explain that they cannot use the attribute on arrays
and that they need to use `__counted_by` instead.
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
@@ -8641,22 +8641,33 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-static bool CheckCountedByAttrOnField(
-Sema , FieldDecl *FD, Expr *E,
-llvm::SmallVectorImpl ) {
+static bool
+CheckCountedByAttrOnField(Sema , FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl ,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
+ unsigned Kind = CountInBytes;
+ if (OrNull)
+Kind += 2;
delcypher wrote:
Rather than magic constants it might be better to use an enum that you refer to
here and in `DiagnosticSemaKinds.td` so that it's more readable.
https://github.com/llvm/llvm-project/pull/93231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
https://github.com/delcypher requested changes to this pull request. Looks pretty good. I have some minor comments. https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null (PR #93231)
delcypher wrote: @hnrklssn #93121 has been landed. Let's hope it sticks this time 爛 https://github.com/llvm/llvm-project/pull/93231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher closed https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From c1f0914b1f8ae8416cebdf7f0573b4ff11f6110d Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH] [BoundsSafety] Reland #93121 Allow 'counted_by' attribute on
pointers in structs in C (#93121)
Fixes #92687.
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
** Differences between #93121 and this patch **
* The memory leak that caused #93121 to be reverted (see #92687) should
now be fixed. See "The Memory Leak".
* The fix to `pragma-attribute-supported-attributes-list.test`
(originally in cef6387) has been incorporated into this patch.
* A relaxation of counted_by semantics (originally in 112eadd) has been
incorporated into this patch.
* The assert in `Parser::DistributeCLateParsedAttrs` has been removed
because that broke downstream code.
* The switch statement in `Parser::ParseLexedCAttribute` has been
removed in favor of using `Parser::ParseGNUAttributeArgs` which does
the same thing but is more feature complete.
* The `EnterScope` parameter has been plumbed through
`Parser::ParseLexedCAttribute` and `Parser::ParseLexedCAttributeList`.
It currently doesn't do anything but it will be needed in future
commits.
** The Memory Leak **
The problem was that these lines parsed the attributes but then did nothing to
free the memory
```
assert(!getLangOpts().CPlusPlus);
for (auto *LateAttr : LateFieldAttrs)
ParseLexedCAttribute(*LateAttr);
```
To fix this this a new `Parser::ParseLexedCAttributeList` method has been
added (based on `Parser::ParseLexedAttributeList`) which does the
necessary memory management. The intention is to merge these two
methods together so there is just one implementation in a future patch
(#93263).
A more principled fixed here would be to fix the ownership of the
`LateParsedAttribute` objects. In principle `LateParsedAttrList` should own
its pointers exclusively and be responsible for deallocating them.
Unfortunately this is complicated by `LateParsedAttribute` objects also
being stored in another data structure (`LateParsedDeclarations`) as
can be seen below (`LA` gets stored in two places).
```
// Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
// Attributes in a class are parsed at the end of the class, along
// with other late-parsed declarations.
if (!ClassStack.empty() && !LateAttrs->parseSoon())
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
delcypher wrote: Filed to #93263 to track the potential refactor. https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From 9e1a9e49b33d4b77d75cee243244dd66a40ea209 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH] [BoundsSafety] Reland #93121 Allow 'counted_by' attribute on
pointers in structs in C (#93121)
Fixes #92687.
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
** Differences between #93121 and this patch **
* The memory leak that caused #93121 to be reverted (see #92687) should
now be fixed. See "The Memory Leak".
* The fix to `pragma-attribute-supported-attributes-list.test`
(originally in cef6387) has been incorporated into this patch.
* A relaxation of counted_by semantics (originally in 112eadd) has been
incorporated into this patch.
* The assert in `Parser::DistributeCLateParsedAttrs` has been removed
because that broke downstream code.
* The switch statement in `Parser::ParseLexedCAttribute` has been
removed in favor of using `Parser::ParseGNUAttributeArgs` which does
the same thing but is more feature complete.
* The `EnterScope` parameter has been plumbed through
`Parser::ParseLexedCAttribute` and `Parser::ParseLexedCAttributeList`.
It currently doesn't do anything but it will be needed in future
commits.
** The Memory Leak **
The problem was that these lines parsed the attributes but then did nothing to
free the memory
```
assert(!getLangOpts().CPlusPlus);
for (auto *LateAttr : LateFieldAttrs)
ParseLexedCAttribute(*LateAttr);
```
To fix this this a new `Parser::ParseLexedCAttributeList` method has been
added (based on `Parser::ParseLexedAttributeList`) which does the
necessary memory management. The intention is to merge these two
methods together so there is just one implementation in a future patch.
A more principled fixed here would be to fix the ownership of the
`LateParsedAttribute` objects. In principle `LateParsedAttrList` should own
its pointers exclusively and be responsible for deallocating them.
Unfortunately this is complicated by `LateParsedAttribute` objects also
being stored in another data structure (`LateParsedDeclarations`) as
can be seen below (`LA` gets stored in two places).
```
// Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
// Attributes in a class are parsed at the end of the class, along
// with other late-parsed declarations.
if (!ClassStack.empty() && !LateAttrs->parseSoon())
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From f9c8abdb243096b6fe0c2ad1a42023039ca7f1f2 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH] [BoundsSafety] Reland #93121 Allow 'counted_by' attribute on
pointers in structs in C (#93121)
Fixes #92687.
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
** Differences between #93121 and this patch **
* The memory leak that caused #93121 to be reverted (see #92687) should
now be fixed. See "The Memory Leak".
* The fix to `pragma-attribute-supported-attributes-list.test`
(originally in cef6387) has been incorporated into this patch.
* A relaxation of counted_by semantics (originally in 112eadd) has been
incorporated into this patch.
* The assert in `Parser::DistributeCLateParsedAttrs` has been removed
because that broke downstream code.
* The switch statement in `Parser::ParseLexedCAttribute` has been
removed in favor of using `Parser::ParseGNUAttributeArgs` which does
the same thing but is more feature complete.
* The `EnterScope` parameter has been plumbed through
`Parser::ParseLexedCAttribute` and `Parser::ParseLexedCAttributeList`.
It currently doesn't do anything but it will be needed in future
commits.
** The Memory Leak **
The problem was that these lines parsed the attributes but then did nothing to
free the memory
```
assert(!getLangOpts().CPlusPlus);
for (auto *LateAttr : LateFieldAttrs)
ParseLexedCAttribute(*LateAttr);
```
To fix this this a new `Parser::ParseLexedCAttributeList` method has been
added (based on Parser::ParseLexedAttributeList) which does the
necessary memory management. The intention is to merge these two
methods together so there is just one implementation in a future patch.
A more principled fixed here would be to fix the ownership of the
LateParsedAttribute objects. In principle LateParsedAttrList should own
its pointers exclusively and be responsible for deallocating them.
Unfortunately this is complicated by LateParsedAttribute objects also
being stored in another data structure (LateParsedDeclarations) as
can be seen below (`LA` gets stored in two places).
```
// Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
// Attributes in a class are parsed at the end of the class, along
// with other late-parsed declarations.
if (!ClassStack.empty() && !LateAttrs->parseSoon())
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From bc5b8bc34b50e2e98d2c6103c7b1c232a3e765b4 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH] [BoundsSafety] Reland #93121 Allow 'counted_by' attribute on
pointers in structs in C (#93121)
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
** Differences between #93121 and this patch **
* The memory leak that caused #93121 to be reverted (see #92687) should now be
fixed
* The fix to `pragma-attribute-supported-attributes-list.test` (originally in
cef6387) has been incorporated into this patch
* A relaxation of counted_by semantics (originally in 112eadd) has been
incorporated into this patch
* The assert in `Parser::DistributeCLateParsedAttrs` has been removed because
that broke
downstream code
* The switch statement in `Parser::ParseLexedCAttribute` has been
removed in favor of using `Parser::ParseGNUAttributeArgs` which
does the same thing but is more feature complete.
* The `EnterScope` parameter has been plumbed through
`Parser::ParseLexedCAttribute` and `Parser::ParseLexedCAttributeList`.
It currently doesn't do anything but it will be needed in future
commits.
rdar://125400257
---
clang/docs/ReleaseNotes.rst | 21 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
clang/include/clang/Basic/DiagnosticGroups.td | 4 +
.../clang/Basic/DiagnosticSemaKinds.td| 23 +-
clang/include/clang/Parse/Parser.h| 9 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 10 +
clang/lib/Parse/ParseDecl.cpp | 106 +++-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 95 +--
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117
...a-attribute-supported-attributes-list.test | 1 -
.../Sema/attr-counted-by-late-parsed-off.c| 26 ++
.../attr-counted-by-late-parsed-struct-ptrs.c | 254 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 224 +++
.../Sema/attr-counted-by-vla-sizeless-types.c | 11 +
clang/test/Sema/attr-counted-by-vla.c | 196 ++
clang/test/Sema/attr-counted-by.c
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
@@ -4944,6 +4944,26 @@ void Parser::ParseStructDeclaration(
}
}
+// TODO: All callers of this function should be moved to
+// `Parser::ParseLexedAttributeList`.
+void Parser::ParseLexedCAttributeList(LateParsedAttrList , bool EnterScope,
+ ParsedAttributes *OutAttrs) {
+ assert(LAs.parseSoon() &&
+ "Attribute list should be marked for immediate parsing.");
+#ifndef NDEBUG
+ auto LangStd = getLangOpts().LangStd;
+ if (LangStd != LangStandard::lang_unspecified) {
+auto Lang = LangStandard::getLangStandardForKind(LangStd).getLanguage();
+assert(Lang == Language::C || Lang == Language::OpenCL);
delcypher wrote:
So parsing a `.m` file without specifying a language seems to result in
```
(lldb) p LangOpts.LangStd
(clang::LangStandard::Kind) lang_gnu11
(lldb) p LangOpts.ObjC
(unsigned int) 1
(lldb) p LangOpts.C99
(unsigned int) 1
(lldb) p LangOpts.C11
(unsigned int) 1
(lldb) p LangOpts.C17
(unsigned int) 0
(lldb) p LangOpts.C23
(unsigned int) 0
(lldb) p LangOpts.CPlusPlus
(unsigned int) 0
(lldb) p
clang::LangStandard::getLangStandardForKind(LangOpts.LangStd).getLanguage()
(clang::Language) C
```
So my assert doesn't do the right thing for Objective-C. For Objective-C my
assert thinks its C 臘♂️. So it seems like my assert would want to be
`assert(!getLangOpts().CPlusPlus && !getLangOpts().ObjC)`. However, I think
I'll remove this assert for now because I intend (provided I can make it to
work with our internal code) to remove this code entirely in favor of re-using
the late parsing code that's used for C++.
https://github.com/llvm/llvm-project/pull/93121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
@@ -4944,6 +4944,26 @@ void Parser::ParseStructDeclaration(
}
}
+// TODO: All callers of this function should be moved to
+// `Parser::ParseLexedAttributeList`.
+void Parser::ParseLexedCAttributeList(LateParsedAttrList , bool EnterScope,
+ ParsedAttributes *OutAttrs) {
+ assert(LAs.parseSoon() &&
+ "Attribute list should be marked for immediate parsing.");
+#ifndef NDEBUG
+ auto LangStd = getLangOpts().LangStd;
+ if (LangStd != LangStandard::lang_unspecified) {
+auto Lang = LangStandard::getLangStandardForKind(LangStd).getLanguage();
+assert(Lang == Language::C || Lang == Language::OpenCL);
delcypher wrote:
> Isn't it? I think getLangOpts().CPlusPlus should set true when it is
> Objective-C++.
I'm not sure if that's the case but I'll check. But what about "Objective-C"? I
didn't want this path taken for that so `!getLangOpts().CPlusPlus` would be
true for Objective-C.
In general I think the approach of thinking that this function should be for C
only is actually flawed and I'm going to look into removing this entirely after
landing this.
https://github.com/llvm/llvm-project/pull/93121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
@@ -4944,6 +4944,26 @@ void Parser::ParseStructDeclaration(
}
}
+// TODO: All callers of this function should be moved to
+// `Parser::ParseLexedAttributeList`.
+void Parser::ParseLexedCAttributeList(LateParsedAttrList , bool EnterScope,
+ ParsedAttributes *OutAttrs) {
+ assert(LAs.parseSoon() &&
+ "Attribute list should be marked for immediate parsing.");
+#ifndef NDEBUG
+ auto LangStd = getLangOpts().LangStd;
+ if (LangStd != LangStandard::lang_unspecified) {
+auto Lang = LangStandard::getLangStandardForKind(LangStd).getLanguage();
+assert(Lang == Language::C || Lang == Language::OpenCL);
delcypher wrote:
@hnrklssn Yes. The original assert didn't check for `Lang == Language::OpenCL`
and that caused test failures due to the assertion failing.
https://github.com/llvm/llvm-project/pull/93121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
@@ -4944,6 +4944,26 @@ void Parser::ParseStructDeclaration(
}
}
+// TODO: All callers of this function should be moved to
+// `Parser::ParseLexedAttributeList`.
+void Parser::ParseLexedCAttributeList(LateParsedAttrList , bool EnterScope,
+ ParsedAttributes *OutAttrs) {
+ assert(LAs.parseSoon() &&
+ "Attribute list should be marked for immediate parsing.");
+#ifndef NDEBUG
+ auto LangStd = getLangOpts().LangStd;
+ if (LangStd != LangStandard::lang_unspecified) {
+auto Lang = LangStandard::getLangStandardForKind(LangStd).getLanguage();
+assert(Lang == Language::C || Lang == Language::OpenCL);
delcypher wrote:
That doesn't sound equivalent. The language could also be objective-C or
objective-C++.
https://github.com/llvm/llvm-project/pull/93121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From 7f5af7cc180825e07b5f5292e3f6b860c8e8591e Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH 1/7] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C (#90786)
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
Co-authored-by: Dan Liew
---
clang/docs/ReleaseNotes.rst | 21 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 17 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 10 +
clang/lib/Parse/ParseDecl.cpp | 104 ++-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 82 --
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117
.../Sema/attr-counted-by-late-parsed-off.c| 26 ++
.../attr-counted-by-late-parsed-struct-ptrs.c | 254 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 224 +++
.../Sema/attr-counted-by-vla-sizeless-types.c | 11 +
clang/test/Sema/attr-counted-by-vla.c | 193 +
clang/test/Sema/attr-counted-by.c | 112
21 files changed, 1117 insertions(+), 148 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-vla-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-vla.c
delete mode 100644 clang/test/Sema/attr-counted-by.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 81e9d0423f96a..5d59b3b053600 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -317,7 +317,8 @@ New Compiler Flags
-
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
delcypher wrote: Hmm looks like I'll have to tweak the assert. Looks like some unit tests don't specify the language. ``` FAIL: Clang-Unit :: CodeGen/./ClangCodeGenTests/5/16 (19428 of 19653) TEST 'Clang-Unit :: CodeGen/./ClangCodeGenTests/5/16' FAILED Script(shard): -- GTEST_OUTPUT=json:/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/tools/clang/unittests/CodeGen/./ClangCodeGenTests-Clang-Unit-78051-5-16.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=16 GTEST_SHARD_INDEX=5 /Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/tools/clang/unittests/CodeGen/./ClangCodeGenTests -- Note: This is test shard 6 of 16. [==] Running 1 test from 1 test suite. [--] Global test environment set-up. [--] 1 test from TBAAMetadataTest [ RUN ] TBAAMetadataTest.CTypedefFields2 LLVM ERROR: getLangStandardForKind() on unspecified kind Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it): 0 ClangCodeGenTests0x000102c186e4 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 88 1 ClangCodeGenTests0x000102c18cc8 PrintStackTraceSignalHandler(void*) + 28 2 ClangCodeGenTests0x000102c1695c llvm::sys::RunSignalHandlers() + 152 3 ClangCodeGenTests0x000102c19e30 SignalHandler(int) + 276 4 libsystem_platform.dylib 0x0001813b9a24 _sigtramp + 56 5 libsystem_pthread.dylib 0x000181389cc0 pthread_kill + 288 6 libsystem_c.dylib0x000181295a40 abort + 180 7 ClangCodeGenTests0x000102ad60bc llvm::report_fatal_error(llvm::Twine const&, bool) + 364 8 ClangCodeGenTests0x000102ad5f50 llvm::report_fatal_error(llvm::Twine const&, bool) + 0 9 ClangCodeGenTests0x00010388c44c clang::LangStandard::getLangStandardForKind(clang::LangStandard::Kind) + 88 10 ClangCodeGenTests0x0001048d565c clang::Parser::ParseLexedCAttributeList(clang::Parser::LateParsedAttrList&, bool, clang::ParsedAttributes*) + 144 11 ClangCodeGenTests0x0001048d6190 clang::Parser::ParseStructUnionBody(clang::SourceLocation, clang::TypeSpecifierType, clang::RecordDecl*) + 1724 12 ClangCodeGenTests0x0001048ff868 clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) + 8916 13 ClangCodeGenTests0x0001048ce33c clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) + 15492 14 ClangCodeGenTests0x0001048c5908 clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*) + 124 15 ClangCodeGenTests0x0001048c54ac clang::Parser::ParseSimpleDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, bool, clang::Parser::ForRangeInit*, clang::SourceLocation*) + 260 16 ClangCodeGenTests0x0001048c5284 clang::Parser::ParseDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, clang::SourceLocation*) + 1060 17 ClangCodeGenTests0x0001049ddbd0 clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) + 2196 18 ClangCodeGenTests0x0001049dc10c clang::Parser::ParseTopLevelDecl(clang::OpaquePtr&, clang::Sema::ModuleImportState&) + 2024 19 ClangCodeGenTests0x0001049db880 clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr&, clang::Sema::ModuleImportState&) + 64 20 ClangCodeGenTests0x0001048ac1c8 clang::ParseAST(clang::Sema&, bool, bool) + 476 21 ClangCodeGenTests0x0001024b1824 llvm::TestCompiler::compile() + 48 22 ClangCodeGenTests0x0001024c1024 (anonymous namespace)::TBAAMetadataTest_CTypedefFields2_Test::TestBody() + 236 23 ClangCodeGenTests0x000102c9cf40 void testing::internal::HandleSehExceptionsInMethodIfSupported(testing::Test*, void (testing::Test::*)(), char const*) + 132 24 ClangCodeGenTests0x000102c6be7c void testing::internal::HandleExceptionsInMethodIfSupported(testing::Test*, void (testing::Test::*)(), char const*) + 96 25 ClangCodeGenTests0x000102c6bdcc testing::Test::Run() + 192 26 ClangCodeGenTests0x000102c6c894 testing::TestInfo::Run() + 304 27 ClangCodeGenTests0x000102c6d768 testing::TestSuite::Run() + 412 28 ClangCodeGenTests0x000102c77ee4 testing::internal::UnitTestImpl::RunAllTests() + 1024 29
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
@@ -4993,20 +4993,8 @@ void Parser::ParseLexedCAttribute(LateParsedAttribute , "late field attribute expects to have at most one declaration."); // Dispatch based on the attribute and parse it - const AttributeCommonInfo::Form ParsedForm = ParsedAttr::Form::GNU(); delcypher wrote: @hnrklssn @rapidsna Heads up I plan to land this change too. I discovered that this this switch statement doesn't need to exist because `ParseGNUAttributeArgs` effectively already does what we need. https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From 7f5af7cc180825e07b5f5292e3f6b860c8e8591e Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH 1/7] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C (#90786)
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
Co-authored-by: Dan Liew
---
clang/docs/ReleaseNotes.rst | 21 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 17 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 10 +
clang/lib/Parse/ParseDecl.cpp | 104 ++-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 82 --
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117
.../Sema/attr-counted-by-late-parsed-off.c| 26 ++
.../attr-counted-by-late-parsed-struct-ptrs.c | 254 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 224 +++
.../Sema/attr-counted-by-vla-sizeless-types.c | 11 +
clang/test/Sema/attr-counted-by-vla.c | 193 +
clang/test/Sema/attr-counted-by.c | 112
21 files changed, 1117 insertions(+), 148 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-vla-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-vla.c
delete mode 100644 clang/test/Sema/attr-counted-by.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 81e9d0423f96a..5d59b3b053600 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -317,7 +317,8 @@ New Compiler Flags
-
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/93121
>From 7f5af7cc180825e07b5f5292e3f6b860c8e8591e Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH 1/6] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C (#90786)
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
Co-authored-by: Dan Liew
---
clang/docs/ReleaseNotes.rst | 21 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 17 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 10 +
clang/lib/Parse/ParseDecl.cpp | 104 ++-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 82 --
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117
.../Sema/attr-counted-by-late-parsed-off.c| 26 ++
.../attr-counted-by-late-parsed-struct-ptrs.c | 254 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 224 +++
.../Sema/attr-counted-by-vla-sizeless-types.c | 11 +
clang/test/Sema/attr-counted-by-vla.c | 193 +
clang/test/Sema/attr-counted-by.c | 112
21 files changed, 1117 insertions(+), 148 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-vla-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-vla.c
delete mode 100644 clang/test/Sema/attr-counted-by.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 81e9d0423f96a..5d59b3b053600 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -317,7 +317,8 @@ New Compiler Flags
-
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
delcypher wrote:
Okay this is interesting...
```
FAIL: Clang :: SemaObjC/warn-thread-safety-analysis.m (15515 of 19369)
TEST 'Clang :: SemaObjC/warn-thread-safety-analysis.m'
FAILED
Exit Code: 134
Command Output (stderr):
--
RUN: at line 1:
/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang
-cc1 -internal-isystem
/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/lib/clang/19/include
-nostdsysteminc -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta
-Wno-objc-root-class
/Volumes/user_data/dev/llvm/llvm.org/main/src/llvm-project/clang/test/SemaObjC/warn-thread-safety-analysis.m
+ /Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang
-cc1 -internal-isystem
/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/lib/clang/19/include
-nostdsysteminc -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta
-Wno-objc-root-class
/Volumes/user_data/dev/llvm/llvm.org/main/src/llvm-project/clang/test/SemaObjC/warn-thread-safety-analysis.m
Unhandled late parsed attribute
UNREACHABLE executed at
/Volumes/user_data/dev/llvm/llvm.org/main/src/llvm-project/clang/lib/Parse/ParseDecl.cpp:4990!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and
include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0. Program arguments:
/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang
-cc1 -internal-isystem
/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/lib/clang/19/include
-nostdsysteminc -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta
-Wno-objc-root-class
/Volumes/user_data/dev/llvm/llvm.org/main/src/llvm-project/clang/test/SemaObjC/warn-thread-safety-analysis.m
1.
/Volumes/user_data/dev/llvm/llvm.org/main/src/llvm-project/clang/test/SemaObjC/warn-thread-safety-analysis.m:7:61:
current parser token '('
#0 0x000107d741b8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x1035d41b8)
#1 0x000107d7479c PrintStackTraceSignalHandler(void*)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x1035d479c)
#2 0x000107d72454 llvm::sys::RunSignalHandlers()
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x1035d2454)
#3 0x000107d758b8 SignalHandler(int)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x1035d58b8)
#4 0x0001813b9a24 (/usr/lib/system/libsystem_platform.dylib+0x18046da24)
#5 0x000181389cc0 (/usr/lib/system/libsystem_pthread.dylib+0x18043dcc0)
#6 0x000181295a40 (/usr/lib/system/libsystem_c.dylib+0x180349a40)
#7 0x000107c4f0dc llvm::install_out_of_memory_new_handler()
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x1034af0dc)
#8 0x00010c40fc90
clang::Parser::ParseLexedCAttribute(clang::Parser::LateParsedAttribute&,
clang::ParsedAttributes*)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107c6fc90)
#9 0x00010c3ed5ec
clang::Parser::ParseLexedAttributeList(clang::Parser::LateParsedAttrList&,
clang::Decl*, bool, bool)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107c4d5ec)
#10 0x00010c51a05c
clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&,
clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107d7a05c)
#11 0x00010c400bb8 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&,
clang::DeclaratorContext, clang::ParsedAttributes&,
clang::Parser::ParsedTemplateInfo&, clang::SourceLocation*,
clang::Parser::ForRangeInit*)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107c60bb8)
#12 0x00010c5190bc
clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&,
clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107d790bc)
#13 0x00010c5185f4
clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&,
clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107d785f4)
#14 0x00010c5177f0
clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&,
clang::ParsedAttributes&, clang::ParsingDeclSpec*)
(/Volumes/user_data/dev/llvm/llvm.org/main/builds/Debug_xc___sccache/bin/clang-19+0x107d777f0)
#15 0x00010c5158cc
clang::Parser::ParseTopLevelDecl(clang::OpaquePtr&,
clang::Sema::ModuleImportState&)
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
delcypher wrote: Hmm looks like I broke ``` Clang :: Sema/attr-capabilities.c Clang :: Sema/diagnose_if.c Clang :: Sema/warn-thread-safety-analysis.c Clang :: SemaObjC/warn-thread-safety-analysis.m ``` Let's see if I can figure out what I did... https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
delcypher wrote: This is blocked by #93121 https://github.com/llvm/llvm-project/pull/92623 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
delcypher wrote: @rapidsna Please check 604e274a5bc37b18c3bc89eed5749c8617de36e7 to check you are happy with the way I'm fixing the memory leak. https://github.com/llvm/llvm-project/pull/93121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
@@ -744,7 +744,13 @@ void Parser::ParseLexedAttributeList(LateParsedAttrList
, Decl *D,
for (unsigned i = 0, ni = LAs.size(); i < ni; ++i) {
if (D)
LAs[i]->addDecl(D);
-ParseLexedAttribute(*LAs[i], EnterScope, OnDefinition);
+// FIXME: There has to be a better way to ask "is this C?""
+if (LangStandard::getLangStandardForKind(getLangOpts().LangStd)
delcypher wrote:
@AaronBallman @erichkeane Is there a better way to check that we're building
for C?
https://github.com/llvm/llvm-project/pull/93121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) (PR #93121)
https://github.com/delcypher created
https://github.com/llvm/llvm-project/pull/93121
This PR attempts to reland https://github.com/llvm/llvm-project/pull/90786 that
was reverted due to a memory leak.
This PR includes the following commits
1. The original commit in #90786 (originally
0ec3b972e58bcbcdc1bebe1696ea37f2931287c3)
2. A follow up fix to `pragma-attribute-supported-attributes-list.test`
(originally in cef6387e52578366c2332275dad88b9953b55336)
3. A relaxation of `counted_by` semantics (originally in
112eadd55f06bee15caadff688ea0b45acbfa804)
4. A fix to the memory leak (in 604e274a5bc37b18c3bc89eed5749c8617de36e7)
The intention will be squash all the changes into a single commit once we've
agreed to the memory leak fix.
## The Memory leak
The problem was that these lines parsed the attributes but then did nothing to
free the memory
```c++
assert(!getLangOpts().CPlusPlus);
for (auto *LateAttr : LateFieldAttrs)
ParseLexedCAttribute(*LateAttr);
```
To fix this the existing `Parser::ParseLexedAttributeList` method has been
tweaked for our use case and has been re-used because it does the necessary
memory management.
A more principled fixed here would be to fix the ownership of the
`LateParsedAttribute` objects. In principle `LateParsedAttrList` should own its
pointers and be responsible for deallocating them. Unfortunately this is
complicated by `LateParsedAttribute` objects also being stored in another data
structure (`LateParsedDeclarations`).
```c++
// Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
// Attributes in a class are parsed at the end of the class, along
// with other late-parsed declarations.
if (!ClassStack.empty() && !LateAttrs->parseSoon())
getCurrentClass().LateParsedDeclarations.push_back(LA);
```
this means the ownership `LateParsedAttribute` objects isn't clear :(
>From 7f5af7cc180825e07b5f5292e3f6b860c8e8591e Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 12:07:40 -0700
Subject: [PATCH 1/4] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C (#90786)
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
Co-authored-by: Dan Liew
---
clang/docs/ReleaseNotes.rst | 21 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 17 +-
[clang] [Attributes] Support Attributes being declared as supporting an experimental late parsing mode "extension" (PR #88596)
delcypher wrote: @erichkeane I posted the comment on the wrong PR. The memory leak is in #90786 not this PR so I deleted the comment i made here. #90786 has already been reverted. https://github.com/llvm/llvm-project/pull/88596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: I'll put up a new version of this PR with the memory leak fixed soon. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote:
Ok. Now I see what's happening.
These lines here are basically giving ownership of `LateParsedAttribute` to the
`LateParsedAttrList`
```
// Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
```
However `LateParsedAttrList` is basically just a
```
class LateParsedAttrList: public SmallVector {
...
}
```
so it won't free the pointers when the list is destroyed. If we look at the C++
code that handles this we can see a bunch of manual memory management.
```
/// Parse all attributes in LAs, and attach them to Decl D.
void Parser::ParseLexedAttributeList(LateParsedAttrList , Decl *D,
bool EnterScope, bool OnDefinition) {
assert(LAs.parseSoon() &&
"Attribute list should be marked for immediate parsing.");
for (unsigned i = 0, ni = LAs.size(); i < ni; ++i) {
if (D)
LAs[i]->addDecl(D);
ParseLexedAttribute(*LAs[i], EnterScope, OnDefinition);
delete LAs[i];
}
LAs.clear();
}
```
This is a silly footgun. `LateParsedAttrList` should properly take ownership of
its pointers by deleting them when the list is destroyed.
For now we should just replicate what is done for C++ but once we're confident
we've fixed things we should make `LateParsedAttrList` properly own its
pointers.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: The leak via `clang::Parser::ParseLexedCAttribute` is ```c++ LA.Toks.push_back(AttrEnd); ``` and the leak via `clang::Parser::ParseGNUAttributes` is ``` LateParsedAttribute *LA = new LateParsedAttribute(this, *AttrName, AttrNameLoc); ``` which is really old code so I doubt this is directly responsible for the leak. I'm a little confused because LSan is telling us that these are indirect leaks which means "reachable from other leaked blocks" but LSan isn't showing any direct leaks (not reachable from anywhere). This might suggest the leak is some sort of cycle (multiple objects leaked that point to each other). https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: Hmm. Apparently there's a memory leak. https://lab.llvm.org/buildbot/#/builders/239/builds/7043 ``` -- Testing: 79948 of 79949 tests, 48 workers -- Testing: FAIL: Clang :: AST/attr-counted-by-late-parsed-struct-ptrs.c (480 of 79948) TEST 'Clang :: AST/attr-counted-by-late-parsed-struct-ptrs.c' FAILED Exit Code: 1 Command Output (stderr): -- RUN: at line 1: /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/lib/clang/19/include -nostdsysteminc -fexperimental-late-parse-attributes /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c -ast-dump | /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c + /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/lib/clang/19/include -nostdsysteminc -fexperimental-late-parse-attributes /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c -ast-dump + /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c = ==1477834==ERROR: LeakSanitizer: detected memory leaks Indirect leak of 432 byte(s) in 2 object(s) allocated from: #0 0xbdec8684 in malloc /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:68:3 #1 0xc36532a8 in safe_malloc /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26:18 #2 0xc36532a8 in llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/SmallVector.cpp:143:15 #3 0xc82d9960 in grow_pod /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:151:11 #4 0xc82d9960 in grow /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:538:41 #5 0xc82d9960 in reserveForParamAndGetAddressImpl > /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:256:11 #6 0xc82d9960 in reserveForParamAndGetAddress /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:543:12 #7 0xc82d9960 in push_back /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:575:23 #8 0xc82d9960 in clang::Parser::ParseLexedCAttribute(clang::Parser::LateParsedAttribute&, clang::ParsedAttributes*) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:4943:11 #9 0xc82db118 in clang::Parser::ParseStructUnionBody(clang::SourceLocation, clang::TypeSpecifierType, clang::RecordDecl*) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:5128:5 #10 0xc827170c in clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:2275:7 #11 0xc82c4734 in clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:4598:7 #12 0xc81b99dc in ParseDeclarationSpecifiers /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/include/clang/Parse/Parser.h:2495:12 #13 0xc81b99dc in clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:1153:3 #14 0xc81b8ffc in clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:1271:12 #15 0xc81b63e4 in clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*)
[clang] [Attributes] Support Attributes being declared as supporting an experimental late parsing mode "extension" (PR #88596)
delcypher wrote: Hmm. Apparently there's a memory leak. https://lab.llvm.org/buildbot/#/builders/239/builds/7043 ``` -- Testing: 79948 of 79949 tests, 48 workers -- Testing: FAIL: Clang :: AST/attr-counted-by-late-parsed-struct-ptrs.c (480 of 79948) TEST 'Clang :: AST/attr-counted-by-late-parsed-struct-ptrs.c' FAILED Exit Code: 1 Command Output (stderr): -- RUN: at line 1: /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/lib/clang/19/include -nostdsysteminc -fexperimental-late-parse-attributes /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c -ast-dump | /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c + /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/lib/clang/19/include -nostdsysteminc -fexperimental-late-parse-attributes /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c -ast-dump + /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c = ==1477834==ERROR: LeakSanitizer: detected memory leaks Indirect leak of 432 byte(s) in 2 object(s) allocated from: #0 0xbdec8684 in malloc /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:68:3 #1 0xc36532a8 in safe_malloc /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26:18 #2 0xc36532a8 in llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/SmallVector.cpp:143:15 #3 0xc82d9960 in grow_pod /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:151:11 #4 0xc82d9960 in grow /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:538:41 #5 0xc82d9960 in reserveForParamAndGetAddressImpl > /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:256:11 #6 0xc82d9960 in reserveForParamAndGetAddress /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:543:12 #7 0xc82d9960 in push_back /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:575:23 #8 0xc82d9960 in clang::Parser::ParseLexedCAttribute(clang::Parser::LateParsedAttribute&, clang::ParsedAttributes*) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:4943:11 #9 0xc82db118 in clang::Parser::ParseStructUnionBody(clang::SourceLocation, clang::TypeSpecifierType, clang::RecordDecl*) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:5128:5 #10 0xc827170c in clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:2275:7 #11 0xc82c4734 in clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:4598:7 #12 0xc81b99dc in ParseDeclarationSpecifiers /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/include/clang/Parse/Parser.h:2495:12 #13 0xc81b99dc in clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:1153:3 #14 0xc81b8ffc in clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:1271:12 #15 0xc81b63e4 in clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*)
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
https://github.com/delcypher edited https://github.com/llvm/llvm-project/pull/92623 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/92623
>From ef46dd51c5c54cf5a76d83b9c15f8f3aee052e42 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 17:15:48 -0700
Subject: [PATCH] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and
language option 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);
};
```
rdar://125400392
---
clang/include/clang/Basic/LangOptions.def | 2 +
clang/include/clang/Driver/Options.td | 8
clang/lib/Sema/SemaDeclAttr.cpp | 2 +-
.../Sema/attr-counted-by-bounds-safety-vlas.c | 37 +++
4 files changed, 48 insertions(+), 1 deletion(-)
create mode 100644 clang/test/Sema/attr-counted-by-bounds-safety-vlas.c
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<"">,
HelpText<"Specify the Swift version to use when filtering API notes">;
+defm bounds_safety : BoolFOption<
+ "experimental-bounds-safety",
+ LangOpts<"BoundsSafety">, DefaultFalse,
+ PosFlag,
+ NegFlag,
+ BothFlags<[], [CC1Option],
+ " experimental bounds safety extension for C">>;
+
defm addrsig : BoolFOption<"addrsig",
CodeGenOpts<"Addrsig">, DefaultFalse,
PosFlag,
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 0..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
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
delcypher wrote: This is based on #70480 but removes the driver part of the change and makes the flag a CC1 flag only. https://github.com/llvm/llvm-project/pull/92623 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and language option and use it to tweak `counted_by`'s semantics (PR #92623)
https://github.com/delcypher created
https://github.com/llvm/llvm-project/pull/92623
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);
};
```
>From cc6adf2e6473ab1dc008ddad5d925cad5e6646cf Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Fri, 17 May 2024 17:15:48 -0700
Subject: [PATCH] [BoundsSafety] Add `-fexperimental-bounds-safety` CC1 and
language option 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);
};
```
---
clang/include/clang/Basic/LangOptions.def | 2 +
clang/include/clang/Driver/Options.td | 8
clang/lib/Sema/SemaDeclAttr.cpp | 2 +-
.../Sema/attr-counted-by-bounds-safety-vlas.c | 37 +++
4 files changed, 48 insertions(+), 1 deletion(-)
create mode 100644 clang/test/Sema/attr-counted-by-bounds-safety-vlas.c
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<"">,
HelpText<"Specify the Swift version to use when filtering API notes">;
+defm bounds_safety : BoolFOption<
+ "experimental-bounds-safety",
+ LangOpts<"BoundsSafety">, DefaultFalse,
+ PosFlag,
+ NegFlag,
+ BothFlags<[], [CC1Option],
+ " experimental bounds safety extension for C">>;
+
defm addrsig : BoolFOption<"addrsig",
CodeGenOpts<"Addrsig">, DefaultFalse,
PosFlag,
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 0..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
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: @kees @bwendling @rapidsna The workaround to downgrade this error to a warning has landed https://github.com/llvm/llvm-project/commit/cef6387e52578366c2332275dad88b9953b55336 https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] cef6387 - [Bounds-Safety] Temporarily relax a `counted_by` attribute restriction on flexible array members
Author: Dan Liew
Date: 2024-05-17T16:23:24-07:00
New Revision: cef6387e52578366c2332275dad88b9953b55336
URL:
https://github.com/llvm/llvm-project/commit/cef6387e52578366c2332275dad88b9953b55336
DIFF:
https://github.com/llvm/llvm-project/commit/cef6387e52578366c2332275dad88b9953b55336.diff
LOG: [Bounds-Safety] Temporarily relax a `counted_by` attribute restriction on
flexible array members
In 0ec3b972e58bcbcdc1bebe1696ea37f2931287c3 an additional restriction
was added when applying the `counted_by` attribute to flexible array
members in structs. The restriction prevented the element type being
a struct that itself had a flexible array member. E.g.:
```
struct has_unannotated_VLA {
int count;
char buffer[];
};
struct buffer_of_structs_with_unnannotated_vla {
int count;
struct has_unannotated_VLA Arr[] __counted_by(count);
};
```
In this example assuming the size of `Arr` is `sizeof(struct
has_unannotated_VLA)*count` (which is what the attribute says) is wrong
because it doesn't account for the size of
`has_unannotated_VLA::buffer`. This is why this kind of code construct
was treated as an error.
However, it turns out existing Linux kernel code used the attribute
on a flexible array member in this way
(https://github.com/llvm/llvm-project/pull/90786#issuecomment-2118416515).
To unbreak the build this restriction is downgraded to a warning with
the plan to make it an error again once the errornous use of the
attribute in the Linux kernel is resolved.
Added:
Modified:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaDeclAttr.cpp
clang/test/Sema/attr-counted-by-vla.c
Removed:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 4cb4f3d999f7a..4fad4d1a0eca7 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1447,6 +1447,10 @@ def FunctionMultiVersioning
def NoDeref : DiagGroup<"noderef">;
+// -fbounds-safety and bounds annotation related warnings
+def BoundsSafetyCountedByEltTyUnknownSize :
+ DiagGroup<"bounds-safety-counted-by-elt-type-unknown-size">;
+
// A group for cross translation unit static analysis related warnings.
def CrossTU : DiagGroup<"ctu">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e6596410c5d0..1efa3af121c10 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6552,7 +6552,7 @@ def err_counted_by_attr_refer_to_union : Error<
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
def err_counted_by_attr_pointee_unknown_size : Error<
- "'counted_by' cannot be applied to %select{"
+ "'counted_by' %select{cannot|should not}3 be applied to %select{"
"a pointer with pointee|" // pointer
"an array with element}0" // array
" of unknown size because %1 is %select{"
@@ -6561,8 +6561,14 @@ def err_counted_by_attr_pointee_unknown_size : Error<
"a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
// CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER
"a struct type with a flexible array member"
+"%select{|. This will be an error in a future compiler version}3"
+""
"}2">;
+def warn_counted_by_attr_elt_type_unknown_size :
+ Warning,
+ InGroup;
+
let CategoryName = "ARC Semantic Issue" in {
// ARC-mode diagnostics.
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index c8b71631076ba..e816ea3647a7c 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8687,6 +8687,7 @@ static bool CheckCountedByAttrOnField(
// Note: The `Decl::isFlexibleArrayMemberLike` check earlier on means
// only `PointeeTy->isStructureTypeWithFlexibleArrayMember()` is reachable
// when `FieldTy->isArrayType()`.
+ bool ShouldWarn = false;
if (PointeeTy->isIncompleteType()) {
InvalidTypeKind = CountedByInvalidPointeeTypeKind::INCOMPLETE;
} else if (PointeeTy->isSizelessType()) {
@@ -8694,13 +8695,25 @@ static bool CheckCountedByAttrOnField(
} else if (PointeeTy->isFunctionType()) {
InvalidTypeKind = CountedByInvalidPointeeTypeKind::FUNCTION;
} else if (PointeeTy->isStructureTypeWithFlexibleArrayMember()) {
+if (FieldTy->isArrayType()) {
+ // 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
+ // done correctly without manually traversing every struct object in the
+ // array at runtime. To allow the code to be built this error is
+ // downgraded to a warning.
+ ShouldWarn =
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: @bwendling This is unfortunate ``` drivers/gpu/drm/radeon/pptable.h:442:5: error: 'counted_by' cannot be applied to an array with element of unknown size because 'ATOM_PPLIB_STATE_V2' (aka 'struct _ATOM_PPLIB_STATE_V2') is a struct type with a flexible array member 442 | ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries); | ^~~~ 1 error generated. ``` Based on our previous discussion (https://github.com/llvm/llvm-project/pull/90786#issuecomment-2108855275) I hope we can agree using the attribute this way is problematic. I don't see how `states` could be have its bounds correctly computed when the compiler doesn't know how big `struct _ATOM_PPLIB_STATE_V2` actually is. To unbreak this particular use quickly I'll downgrade the struct with FAM case to a warning and from there we'll need to decide how to proceed. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: Test fixed by `112eadd55f06bee15caadff688ea0b45acbfa804`. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 112eadd - [Bounds-Safety] Fix `pragma-attribute-supported-attributes-list.test`
Author: Dan Liew Date: 2024-05-17T13:09:22-07:00 New Revision: 112eadd55f06bee15caadff688ea0b45acbfa804 URL: https://github.com/llvm/llvm-project/commit/112eadd55f06bee15caadff688ea0b45acbfa804 DIFF: https://github.com/llvm/llvm-project/commit/112eadd55f06bee15caadff688ea0b45acbfa804.diff LOG: [Bounds-Safety] Fix `pragma-attribute-supported-attributes-list.test` 0ec3b972e58bcbcdc1bebe1696ea37f2931287c3 changed the `counted_by` attribute to be `LateAttrParseExperimentalExt`. This means the attribute is no longer supported by `#pragma clang attribute`. However, the `pragma-attribute-supported-attributes-list.test` wasn't updated to account for that. rdar://125400257 Added: Modified: clang/test/Misc/pragma-attribute-supported-attributes-list.test Removed: diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test index fd0e6d71baa80..99732694f72a5 100644 --- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test +++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test @@ -63,7 +63,6 @@ // CHECK-NEXT: CoroOnlyDestroyWhenComplete (SubjectMatchRule_record) // CHECK-NEXT: CoroReturnType (SubjectMatchRule_record) // CHECK-NEXT: CoroWrapper (SubjectMatchRule_function) -// CHECK-NEXT: CountedBy (SubjectMatchRule_field) // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface) // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface) // CHECK-NEXT: Destructor (SubjectMatchRule_function) ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: Looks like I broke the `clang/test/Misc/pragma-attribute-supported-attributes-list.test` test. I'll push a follow up fix to that test once I've confirmed I've fixed it. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
