[Bounds Safety][NFC] Add SemaBoundsSafety class and move existing Sema checks there
#98954
Conversation
…ema 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
delcypher
added
the
clang:bounds-safety
delcypher
requested review from
kees,
AaronBallman,
bwendling,
erichkeane,
hnrklssn,
rapidsna,
patrykstefanski and
apple-fcloutier
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Dan Liew (delcypher) ChangesThis patch creates a The existing As rdar://131777237 Full diff: https://github.com/llvm/llvm-project/pull/98954.diff 6 Files Affected:
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 &BoundsSafety() {
+ assert(BoundsSafetyPtr);
+ return *BoundsSafetyPtr;
+ }
+
SemaBPF &BPF() {
assert(BPFPtr);
return *BPFPtr;
@@ -1294,6 +1300,7 @@ class Sema final : public SemaBase {
std::unique_ptr<SemaAMDGPU> AMDGPUPtr;
std::unique_ptr<SemaARM> ARMPtr;
std::unique_ptr<SemaAVR> AVRPtr;
+ std::unique_ptr<SemaBoundsSafety> BoundsSafetyPtr;
std::unique_ptr<SemaBPF> BPFPtr;
std::unique_ptr<SemaCodeCompletion> CodeCompletionPtr;
std::unique_ptr<SemaCUDA> CUDAPtr;
diff --git a/clang/include/clang/Sema/SemaBoundsSafety.h b/clang/include/clang/Sema/SemaBoundsSafety.h
new file mode 100644
index 0000000000000..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 &S);
+
+ bool CheckCountedByAttrOnField(
+ FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls, bool CountInBytes,
+ bool OrNull);
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_SEMA_SEMABOUNDSSAFETY_H
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/Sema.cpp b/clang/lib/Sema/Sema.cpp
index d6228718d53ae..9f6e1a887e40d 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/SemaARM.h"
#include "clang/Sema/SemaAVR.h"
#include "clang/Sema/SemaBPF.h"
+#include "clang/Sema/SemaBoundsSafety.h"
#include "clang/Sema/SemaCUDA.h"
#include "clang/Sema/SemaCodeCompletion.h"
#include "clang/Sema/SemaConsumer.h"
@@ -224,6 +225,7 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
AMDGPUPtr(std::make_unique<SemaAMDGPU>(*this)),
ARMPtr(std::make_unique<SemaARM>(*this)),
AVRPtr(std::make_unique<SemaAVR>(*this)),
+ BoundsSafetyPtr(std::make_unique<SemaBoundsSafety>(*this)),
BPFPtr(std::make_unique<SemaBPF>(*this)),
CodeCompletionPtr(
std::make_unique<SemaCodeCompletion>(*this, CodeCompleter)),
diff --git a/clang/lib/Sema/SemaBoundsSafety.cpp b/clang/lib/Sema/SemaBoundsSafety.cpp
new file mode 100644
index 0000000000000..a7d88cc5073c6
--- /dev/null
+++ b/clang/lib/Sema/SemaBoundsSafety.cpp
@@ -0,0 +1,198 @@
+//===---- 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`)
+///
+//===----------------------------------------------------------------------===//
+
+#include "clang/Sema/SemaBoundsSafety.h"
+#include "clang/Basic/DiagnosticSema.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Sema/Sema.h"
+
+namespace clang {
+SemaBoundsSafety::SemaBoundsSafety(Sema &S) : SemaBase(S) {}
+
+static CountAttributedType::DynamicCountPointerKind
+getCountAttrKind(bool CountInBytes, bool OrNull) {
+ if (CountInBytes)
+ return OrNull ? CountAttributedType::SizedByOrNull
+ : CountAttributedType::SizedBy;
+ return OrNull ? CountAttributedType::CountedByOrNull
+ : CountAttributedType::CountedBy;
+}
+
+static const RecordDecl *GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
+ const auto *RD = FD->getParent();
+ // An unnamed struct is anonymous struct only if it's not instantiated.
+ // However, the struct may not be fully processed yet to determine
+ // whether it's anonymous or not. In that case, this function treats it as
+ // an anonymous struct and tries to find a named parent.
+ while (RD && (RD->isAnonymousStructOrUnion() ||
+ (!RD->isCompleteDefinition() && RD->getName().empty()))) {
+ const auto *Parent = dyn_cast<RecordDecl>(RD->getParent());
+ if (!Parent)
+ break;
+ RD = Parent;
+ }
+ return RD;
+}
+
+enum class CountedByInvalidPointeeTypeKind {
+ INCOMPLETE,
+ SIZELESS,
+ FUNCTION,
+ FLEXIBLE_ARRAY_MEMBER,
+ VALID,
+};
+
+bool SemaBoundsSafety::CheckCountedByAttrOnField(
+ FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls, bool CountInBytes,
+ bool OrNull) {
+ // Check the context the attribute is used in
+
+ unsigned Kind = getCountAttrKind(CountInBytes, OrNull);
+
+ if (FD->getParent()->isUnion()) {
+ Diag(FD->getBeginLoc(), diag::err_count_attr_in_union)
+ << Kind << FD->getSourceRange();
+ return true;
+ }
+
+ const auto FieldTy = FD->getType();
+ if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
+ Diag(FD->getBeginLoc(),
+ diag::err_count_attr_not_on_ptr_or_flexible_array_member)
+ << Kind << FD->getLocation() << /* suggest counted_by */ 1;
+ return true;
+ }
+ if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
+ Diag(FD->getBeginLoc(),
+ diag::err_count_attr_not_on_ptr_or_flexible_array_member)
+ << Kind << FD->getLocation() << /* do not suggest counted_by */ 0;
+ return true;
+ }
+
+ LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
+ LangOptions::StrictFlexArraysLevelKind::IncompleteOnly;
+ if (FieldTy->isArrayType() &&
+ !Decl::isFlexibleArrayMemberLike(getASTContext(), FD, FieldTy,
+ StrictFlexArraysLevel, true)) {
+ Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_on_array_not_flexible_array_member)
+ << Kind << FD->getLocation();
+ return true;
+ }
+
+ CountedByInvalidPointeeTypeKind InvalidTypeKind =
+ CountedByInvalidPointeeTypeKind::VALID;
+ QualType PointeeTy;
+ int SelectPtrOrArr = 0;
+ if (FieldTy->isPointerType()) {
+ PointeeTy = FieldTy->getPointeeType();
+ SelectPtrOrArr = 0;
+ } else {
+ assert(FieldTy->isArrayType());
+ const ArrayType *AT = getASTContext().getAsArrayType(FieldTy);
+ PointeeTy = AT->getElementType();
+ SelectPtrOrArr = 1;
+ }
+ // Note: The `Decl::isFlexibleArrayMemberLike` check earlier on means
+ // only `PointeeTy->isStructureTypeWithFlexibleArrayMember()` is reachable
+ // when `FieldTy->isArrayType()`.
+ bool ShouldWarn = false;
+ if (PointeeTy->isIncompleteType() && !CountInBytes) {
+ InvalidTypeKind = CountedByInvalidPointeeTypeKind::INCOMPLETE;
+ } else if (PointeeTy->isSizelessType()) {
+ InvalidTypeKind = CountedByInvalidPointeeTypeKind::SIZELESS;
+ } 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 = true;
+ }
+ InvalidTypeKind = CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER;
+ }
+
+ if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID) {
+ unsigned DiagID = ShouldWarn
+ ? diag::warn_counted_by_attr_elt_type_unknown_size
+ : diag::err_counted_by_attr_pointee_unknown_size;
+ Diag(FD->getBeginLoc(), DiagID)
+ << SelectPtrOrArr << PointeeTy << (int)InvalidTypeKind
+ << (ShouldWarn ? 1 : 0) << Kind << FD->getSourceRange();
+ return true;
+ }
+
+ // Check the expression
+
+ if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
+ Diag(E->getBeginLoc(), diag::err_count_attr_argument_not_integer)
+ << Kind << E->getSourceRange();
+ return true;
+ }
+
+ auto *DRE = dyn_cast<DeclRefExpr>(E);
+ if (!DRE) {
+ Diag(E->getBeginLoc(),
+ diag::err_count_attr_only_support_simple_decl_reference)
+ << Kind << E->getSourceRange();
+ return true;
+ }
+
+ auto *CountDecl = DRE->getDecl();
+ FieldDecl *CountFD = dyn_cast<FieldDecl>(CountDecl);
+ if (auto *IFD = dyn_cast<IndirectFieldDecl>(CountDecl)) {
+ CountFD = IFD->getAnonField();
+ }
+ if (!CountFD) {
+ Diag(E->getBeginLoc(), diag::err_count_attr_must_be_in_structure)
+ << CountDecl << Kind << E->getSourceRange();
+
+ Diag(CountDecl->getBeginLoc(),
+ diag::note_flexible_array_counted_by_attr_field)
+ << CountDecl << CountDecl->getSourceRange();
+ return true;
+ }
+
+ if (FD->getParent() != CountFD->getParent()) {
+ if (CountFD->getParent()->isUnion()) {
+ Diag(CountFD->getBeginLoc(), diag::err_count_attr_refer_to_union)
+ << Kind << CountFD->getSourceRange();
+ return true;
+ }
+ // Whether CountRD is an anonymous struct is not determined at this
+ // point. Thus, an additional diagnostic in case it's not anonymous struct
+ // is done later in `Parser::ParseStructDeclaration`.
+ auto *RD = GetEnclosingNamedOrTopAnonRecord(FD);
+ auto *CountRD = GetEnclosingNamedOrTopAnonRecord(CountFD);
+
+ if (RD != CountRD) {
+ Diag(E->getBeginLoc(), diag::err_count_attr_param_not_in_same_struct)
+ << CountFD << Kind << FieldTy->isArrayType() << E->getSourceRange();
+ Diag(CountFD->getBeginLoc(),
+ diag::note_flexible_array_counted_by_attr_field)
+ << CountFD << CountFD->getSourceRange();
+ return true;
+ }
+ }
+
+ Decls.push_back(TypeCoupledDeclRefInfo(CountFD, /*IsDref*/ false));
+ return false;
+}
+
+} // namespace clang
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 20f46c003a464..b371a2009f2e0 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/SemaARM.h"
#include "clang/Sema/SemaAVR.h"
#include "clang/Sema/SemaBPF.h"
+#include "clang/Sema/SemaBoundsSafety.h"
#include "clang/Sema/SemaCUDA.h"
#include "clang/Sema/SemaHLSL.h"
#include "clang/Sema/SemaInternal.h"
@@ -5852,181 +5853,6 @@ static void handleZeroCallUsedRegsAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
D->addAttr(ZeroCallUsedRegsAttr::Create(S.Context, Kind, AL));
}
-static const RecordDecl *GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
- const auto *RD = FD->getParent();
- // An unnamed struct is anonymous struct only if it's not instantiated.
- // However, the struct may not be fully processed yet to determine
- // whether it's anonymous or not. In that case, this function treats it as
- // an anonymous struct and tries to find a named parent.
- while (RD && (RD->isAnonymousStructOrUnion() ||
- (!RD->isCompleteDefinition() && RD->getName().empty()))) {
- const auto *Parent = dyn_cast<RecordDecl>(RD->getParent());
- if (!Parent)
- break;
- RD = Parent;
- }
- return RD;
-}
-
-static CountAttributedType::DynamicCountPointerKind
-getCountAttrKind(bool CountInBytes, bool OrNull) {
- if (CountInBytes)
- return OrNull ? CountAttributedType::SizedByOrNull
- : CountAttributedType::SizedBy;
- return OrNull ? CountAttributedType::CountedByOrNull
- : CountAttributedType::CountedBy;
-}
-
-enum class CountedByInvalidPointeeTypeKind {
- INCOMPLETE,
- SIZELESS,
- FUNCTION,
- FLEXIBLE_ARRAY_MEMBER,
- VALID,
-};
-
-static bool
-CheckCountedByAttrOnField(Sema &S, FieldDecl *FD, Expr *E,
- llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls,
- bool CountInBytes, bool OrNull) {
- // Check the context the attribute is used in
-
- unsigned Kind = getCountAttrKind(CountInBytes, OrNull);
-
- if (FD->getParent()->isUnion()) {
- S.Diag(FD->getBeginLoc(), diag::err_count_attr_in_union)
- << Kind << FD->getSourceRange();
- return true;
- }
-
- const auto FieldTy = FD->getType();
- if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
- S.Diag(FD->getBeginLoc(),
- diag::err_count_attr_not_on_ptr_or_flexible_array_member)
- << Kind << FD->getLocation() << /* suggest counted_by */ 1;
- return true;
- }
- if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
- S.Diag(FD->getBeginLoc(),
- diag::err_count_attr_not_on_ptr_or_flexible_array_member)
- << Kind << FD->getLocation() << /* do not suggest counted_by */ 0;
- return true;
- }
-
- LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
- LangOptions::StrictFlexArraysLevelKind::IncompleteOnly;
- if (FieldTy->isArrayType() &&
- !Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FieldTy,
- StrictFlexArraysLevel, true)) {
- S.Diag(FD->getBeginLoc(),
- diag::err_counted_by_attr_on_array_not_flexible_array_member)
- << Kind << FD->getLocation();
- return true;
- }
-
- CountedByInvalidPointeeTypeKind InvalidTypeKind =
- CountedByInvalidPointeeTypeKind::VALID;
- QualType PointeeTy;
- int SelectPtrOrArr = 0;
- if (FieldTy->isPointerType()) {
- PointeeTy = FieldTy->getPointeeType();
- SelectPtrOrArr = 0;
- } else {
- assert(FieldTy->isArrayType());
- const ArrayType *AT = S.getASTContext().getAsArrayType(FieldTy);
- PointeeTy = AT->getElementType();
- SelectPtrOrArr = 1;
- }
- // Note: The `Decl::isFlexibleArrayMemberLike` check earlier on means
- // only `PointeeTy->isStructureTypeWithFlexibleArrayMember()` is reachable
- // when `FieldTy->isArrayType()`.
- bool ShouldWarn = false;
- if (PointeeTy->isIncompleteType() && !CountInBytes) {
- InvalidTypeKind = CountedByInvalidPointeeTypeKind::INCOMPLETE;
- } else if (PointeeTy->isSizelessType()) {
- InvalidTypeKind = CountedByInvalidPointeeTypeKind::SIZELESS;
- } 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 = true;
- }
- InvalidTypeKind = CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER;
- }
-
- if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID) {
- unsigned DiagID = ShouldWarn
- ? diag::warn_counted_by_attr_elt_type_unknown_size
- : diag::err_counted_by_attr_pointee_unknown_size;
- S.Diag(FD->getBeginLoc(), DiagID)
- << SelectPtrOrArr << PointeeTy << (int)InvalidTypeKind
- << (ShouldWarn ? 1 : 0) << Kind << FD->getSourceRange();
- return true;
- }
-
- // Check the expression
-
- if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
- S.Diag(E->getBeginLoc(), diag::err_count_attr_argument_not_integer)
- << Kind << E->getSourceRange();
- return true;
- }
-
- auto *DRE = dyn_cast<DeclRefExpr>(E);
- if (!DRE) {
- S.Diag(E->getBeginLoc(),
- diag::err_count_attr_only_support_simple_decl_reference)
- << Kind << E->getSourceRange();
- return true;
- }
-
- auto *CountDecl = DRE->getDecl();
- FieldDecl *CountFD = dyn_cast<FieldDecl>(CountDecl);
- if (auto *IFD = dyn_cast<IndirectFieldDecl>(CountDecl)) {
- CountFD = IFD->getAnonField();
- }
- if (!CountFD) {
- S.Diag(E->getBeginLoc(), diag::err_count_attr_must_be_in_structure)
- << CountDecl << Kind << E->getSourceRange();
-
- S.Diag(CountDecl->getBeginLoc(),
- diag::note_flexible_array_counted_by_attr_field)
- << CountDecl << CountDecl->getSourceRange();
- return true;
- }
-
- if (FD->getParent() != CountFD->getParent()) {
- if (CountFD->getParent()->isUnion()) {
- S.Diag(CountFD->getBeginLoc(), diag::err_count_attr_refer_to_union)
- << Kind << CountFD->getSourceRange();
- return true;
- }
- // Whether CountRD is an anonymous struct is not determined at this
- // point. Thus, an additional diagnostic in case it's not anonymous struct
- // is done later in `Parser::ParseStructDeclaration`.
- auto *RD = GetEnclosingNamedOrTopAnonRecord(FD);
- auto *CountRD = GetEnclosingNamedOrTopAnonRecord(CountFD);
-
- if (RD != CountRD) {
- S.Diag(E->getBeginLoc(), diag::err_count_attr_param_not_in_same_struct)
- << CountFD << Kind << FieldTy->isArrayType() << E->getSourceRange();
- S.Diag(CountFD->getBeginLoc(),
- diag::note_flexible_array_counted_by_attr_field)
- << CountFD << CountFD->getSourceRange();
- return true;
- }
- }
-
- Decls.push_back(TypeCoupledDeclRefInfo(CountFD, /*IsDref*/ false));
- return false;
-}
-
static void handleCountedByAttrField(Sema &S, Decl *D, const ParsedAttr &AL) {
auto *FD = dyn_cast<FieldDecl>(D);
assert(FD);
@@ -6059,7 +5885,8 @@ static void handleCountedByAttrField(Sema &S, Decl *D, const ParsedAttr &AL) {
}
llvm::SmallVector<TypeCoupledDeclRefInfo, 1> Decls;
- if (CheckCountedByAttrOnField(S, FD, CountExpr, Decls, CountInBytes, OrNull))
+ if (S.BoundsSafety().CheckCountedByAttrOnField(FD, CountExpr, Decls,
+ CountInBytes, OrNull))
return;
QualType CAT = S.BuildCountAttributedArrayOrPointerType(
|
There was a problem hiding this comment.
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.
I drove the effort to split Sema, and it has stalled, because it's not clear what to do with what's left there. We had several offline discussions, but we didn't find a solution that would substantially improve status quo.
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. 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. 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.
| #include "llvm/ADT/SmallVector.h" | ||
|
|
||
| namespace clang { | ||
| class CountAttributedType; |
There was a problem hiding this comment.
I think only half of those forward declarations are really needed in the header.
I don't have a strong opinion on whether this should be split up, but I just wanted to point out that |
"Extension" is definitely quite broad. What I meant are (basically) languages that are based off C or C++. Would you argue that
I did that a lot with other parts of |
As noted above
It doesn't right now because most of
The
My end goal is to have "somewhere" to put all of the Sema code that supports We don't have to use the sub-class |
In my opinion it fits in the set because it is a (pretty large) C language extension.
I don't agree with this approach. I outlined above why I think it makes a lot of sense to keep the If at some point we come up with some new design for Sema we can easily move the |
No, it doesn't fit nicely into the division, which is the reason we're having this discussion.
You can have
A bigger picture is "what are we going to do with the rest of the attributes in
I have to say that
That's why I propose to follow long-established practice of doing |
|
The separations we've been making so far in That said, I think grouping the bounds safety semantic bits together in a single source file is a totally reasonable thing to do (Vlad's suggestion of |
If we don't agree on this then I must not fully understand what the criteria is for dividing Sema current is.
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.
Sure. Let's go with that approach then. |
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?
Excellent, thank you! |
Thanks for clarifying. I would describe 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.
Interesting question. I guess it depends on the size of the language feature and given that almost none of
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. |
They are a different group of |
Yup, and they're an example of where it helps to have layering (if parts of |
|
Thanks all for the clarification. Closing this PR in favor of #99330 |
This patch creates a
SemaBoundsSafetyclass to create a cleanseparation between
-fbounds-safetySema checks andother Sema checks. A
SemaBoundsSafetyobject is available via theSema::BoundsSafety()method, similar to how other Sema checks havebeen seperated out (e.g.
SemaSwift).The existing
CheckCountedByAttrOnFieldfunction and related helperfunctions and types from
SemaDeclAttr.cpphas been moved intoSemaBoundsSafety. Althoughcounted_by(_or_null)andsized_by(_or_null)attributes have a meaning outside of-fbounds-safetyit seems reasonable to also have the Sema logic livein
SemaBoundsSafetysince the intention is that the attributes willhave the same semantics (but not necessarily the same enforcement).
As
-fbounds-safetyis upstreamed additional Sema checks will beadded to the
SemaBoundsSafetyclass.rdar://131777237