nonblocking/nonallocating attributes (was: nolock/noalloc) #84983
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules Author: Doug Wyatt (dougsonos) ChangesRough first draft. This is an early PR to solicit comments on the overall approach and a number of outstanding questions. Some of the larger ones, "earlier" in the flow (from parse -> Sema):
Patch is 116.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84983.diff 26 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index a5879591f4c659..0460f30ce8a8b4 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -3008,6 +3008,13 @@ class FunctionDecl : public DeclaratorDecl,
/// computed and stored.
unsigned getODRHash() const;
+ FunctionEffectSet getFunctionEffects() const {
+ const auto *FPT = getType()->getAs<FunctionProtoType>();
+ if (FPT)
+ return FPT->getFunctionEffects();
+ return {};
+ }
+
// Implement isa/cast/dyncast/etc.
static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) {
@@ -4633,6 +4640,8 @@ class BlockDecl : public Decl, public DeclContext {
SourceRange getSourceRange() const override LLVM_READONLY;
+ FunctionEffectSet getFunctionEffects() const;
+
// Implement isa/cast/dyncast/etc.
static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) { return K == Block; }
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1942b0e67f65a3..de21561ce5dcad 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -4047,8 +4047,12 @@ class FunctionType : public Type {
LLVM_PREFERRED_TYPE(bool)
unsigned HasArmTypeAttributes : 1;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned HasFunctionEffects : 1;
+
FunctionTypeExtraBitfields()
- : NumExceptionType(0), HasArmTypeAttributes(false) {}
+ : NumExceptionType(0), HasArmTypeAttributes(false),
+ HasFunctionEffects(false) {}
};
/// The AArch64 SME ACLE (Arm C/C++ Language Extensions) define a number
@@ -4181,6 +4185,131 @@ class FunctionNoProtoType : public FunctionType, public llvm::FoldingSetNode {
}
};
+class FunctionEffect;
+class FunctionEffectSet;
+
+// It is the user's responsibility to keep this in set form: elements are
+// ordered and unique.
+// We could hide the mutating methods which are capable of breaking the
+// invariant, but they're needed and safe when used with STL set algorithms.
+class MutableFunctionEffectSet : public SmallVector<const FunctionEffect *, 4> {
+public:
+ using SmallVector::insert;
+ using SmallVector::SmallVector;
+
+ /// Maintains order/uniquenesss.
+ void insert(const FunctionEffect *effect);
+
+ MutableFunctionEffectSet &operator|=(FunctionEffectSet rhs);
+};
+
+class FunctionEffectSet {
+public:
+ // These sets will tend to be very small (1 element), so represent them as
+ // sorted vectors, which are compatible with the STL set algorithms. Using an
+ // array or vector also means the elements are contiguous, keeping iterators
+ // simple.
+
+private:
+ // 'Uniqued' refers to the set itself being uniqued. Storage is allocated
+ // separately. Use ArrayRef for its iterators. Subclass so as to be able to
+ // compare (it seems ArrayRef would silently convert itself to a vector for
+ // comparison?!).
+ class UniquedAndSortedFX : public llvm::ArrayRef<const FunctionEffect *> {
+ public:
+ using Base = llvm::ArrayRef<const FunctionEffect *>;
+
+ UniquedAndSortedFX(Base Array) : Base(Array) {}
+ UniquedAndSortedFX(const FunctionEffect **Ptr, size_t Len)
+ : Base(Ptr, Len) {}
+
+ bool operator<(const UniquedAndSortedFX &rhs) const;
+ };
+
+ // Could have used a TinyPtrVector if it were unique-able.
+ // Empty set has a null Impl.
+ llvm::PointerUnion<const FunctionEffect *, const UniquedAndSortedFX *> Impl;
+
+ explicit FunctionEffectSet(const FunctionEffect *Single) : Impl(Single) {}
+ explicit FunctionEffectSet(const UniquedAndSortedFX *Multi) : Impl(Multi) {}
+
+public:
+ using Differences =
+ SmallVector<std::pair<const FunctionEffect *, bool /*added*/>>;
+
+ FunctionEffectSet() : Impl(nullptr) {}
+
+ void *getOpaqueValue() const { return Impl.getOpaqueValue(); }
+
+ explicit operator bool() const { return !empty(); }
+ bool empty() const {
+ if (Impl.isNull())
+ return true;
+ if (const UniquedAndSortedFX *Vec =
+ dyn_cast_if_present<const UniquedAndSortedFX *>(Impl))
+ return Vec->empty();
+ return false;
+ }
+ size_t size() const {
+ if (empty())
+ return 0;
+ if (isa<const FunctionEffect *>(Impl))
+ return 1;
+ return cast<const UniquedAndSortedFX *>(Impl)->size();
+ }
+
+ using iterator = const FunctionEffect *const *;
+
+ iterator begin() const {
+ if (isa<const FunctionEffect *>(Impl))
+ return Impl.getAddrOfPtr1();
+ return cast<const UniquedAndSortedFX *>(Impl)->begin();
+ }
+
+ iterator end() const {
+ if (isa<const FunctionEffect *>(Impl))
+ return begin() + (Impl.isNull() ? 0 : 1);
+ return cast<const UniquedAndSortedFX *>(Impl)->end();
+ }
+
+ ArrayRef<const FunctionEffect *> items() const { return {begin(), end()}; }
+
+ // Since iterators are non-trivial and sets are very often empty,
+ // encourage short-circuiting loops for the empty set.
+ // void for_each(llvm::function_ref<void(const FunctionEffect*)> func) const;
+
+ bool operator==(const FunctionEffectSet &other) const {
+ return Impl == other.Impl;
+ }
+ bool operator!=(const FunctionEffectSet &other) const {
+ return Impl != other.Impl;
+ }
+ bool operator<(const FunctionEffectSet &other) const;
+
+ void dump(llvm::raw_ostream &OS) const;
+
+ /// Factory functions: return instances with uniqued implementations.
+ static FunctionEffectSet create(const FunctionEffect &single) {
+ return FunctionEffectSet{&single};
+ }
+ static FunctionEffectSet create(llvm::ArrayRef<const FunctionEffect *> items);
+
+ /// Union. Caller should check for incompatible effects.
+ FunctionEffectSet operator|(const FunctionEffectSet &rhs) const;
+ /// Intersection.
+ MutableFunctionEffectSet operator&(const FunctionEffectSet &rhs) const;
+ /// Difference.
+ MutableFunctionEffectSet operator-(const FunctionEffectSet &rhs) const;
+
+ /// Caller should short-circuit by checking for equality first.
+ static Differences differences(const FunctionEffectSet &Old,
+ const FunctionEffectSet &New);
+
+ /// Extract the effects from a Type if it is a BlockType or FunctionProtoType,
+ /// or pointer to one.
+ static FunctionEffectSet get(const Type &TyRef);
+};
+
/// Represents a prototype with parameter type info, e.g.
/// 'int foo(int)' or 'int foo(void)'. 'void' is represented as having no
/// parameters, not as having a single void parameter. Such a type can have
@@ -4195,7 +4324,8 @@ class FunctionProtoType final
FunctionProtoType, QualType, SourceLocation,
FunctionType::FunctionTypeExtraBitfields,
FunctionType::FunctionTypeArmAttributes, FunctionType::ExceptionType,
- Expr *, FunctionDecl *, FunctionType::ExtParameterInfo, Qualifiers> {
+ Expr *, FunctionDecl *, FunctionType::ExtParameterInfo,
+ FunctionEffectSet, Qualifiers> {
friend class ASTContext; // ASTContext creates these.
friend TrailingObjects;
@@ -4226,6 +4356,9 @@ class FunctionProtoType final
// an ExtParameterInfo for each of the parameters. Present if and
// only if hasExtParameterInfos() is true.
//
+ // * Optionally, a FunctionEffectSet. Present if and only if
+ // hasFunctionEffects() is true.
+ //
// * Optionally a Qualifiers object to represent extra qualifiers that can't
// be represented by FunctionTypeBitfields.FastTypeQuals. Present if and only
// if hasExtQualifiers() is true.
@@ -4284,6 +4417,7 @@ class FunctionProtoType final
ExceptionSpecInfo ExceptionSpec;
const ExtParameterInfo *ExtParameterInfos = nullptr;
SourceLocation EllipsisLoc;
+ FunctionEffectSet FunctionEffects;
ExtProtoInfo()
: Variadic(false), HasTrailingReturn(false),
@@ -4301,7 +4435,8 @@ class FunctionProtoType final
bool requiresFunctionProtoTypeExtraBitfields() const {
return ExceptionSpec.Type == EST_Dynamic ||
- requiresFunctionProtoTypeArmAttributes();
+ requiresFunctionProtoTypeArmAttributes() ||
+ FunctionEffects;
}
bool requiresFunctionProtoTypeArmAttributes() const {
@@ -4349,6 +4484,10 @@ class FunctionProtoType final
return hasExtParameterInfos() ? getNumParams() : 0;
}
+ unsigned numTrailingObjects(OverloadToken<FunctionEffectSet>) const {
+ return hasFunctionEffects();
+ }
+
/// Determine whether there are any argument types that
/// contain an unexpanded parameter pack.
static bool containsAnyUnexpandedParameterPack(const QualType *ArgArray,
@@ -4450,6 +4589,7 @@ class FunctionProtoType final
EPI.RefQualifier = getRefQualifier();
EPI.ExtParameterInfos = getExtParameterInfosOrNull();
EPI.AArch64SMEAttributes = getAArch64SMEAttributes();
+ EPI.FunctionEffects = getFunctionEffects();
return EPI;
}
@@ -4661,6 +4801,18 @@ class FunctionProtoType final
return false;
}
+ bool hasFunctionEffects() const {
+ if (!hasExtraBitfields())
+ return false;
+ return getTrailingObjects<FunctionTypeExtraBitfields>()->HasFunctionEffects;
+ }
+
+ FunctionEffectSet getFunctionEffects() const {
+ if (hasFunctionEffects())
+ return *getTrailingObjects<FunctionEffectSet>();
+ return {};
+ }
+
bool isSugared() const { return false; }
QualType desugar() const { return QualType(this, 0); }
@@ -7754,6 +7906,145 @@ QualType DecayedType::getPointeeType() const {
void FixedPointValueToString(SmallVectorImpl<char> &Str, llvm::APSInt Val,
unsigned Scale);
+// ------------------------------------------------------------------------------
+
+// TODO: Should FunctionEffect be located elsewhere, where Decl is not
+// forward-declared?
+class Decl;
+class CXXMethodDecl;
+
+/// Represents an abstract function effect.
+class FunctionEffect {
+public:
+ enum EffectType {
+ kGeneric,
+ kNoLockTrue,
+ kNoAllocTrue,
+ };
+
+ /// Flags describing behaviors of the effect.
+ using Flags = unsigned;
+ enum FlagBit : unsigned {
+ // Some effects require verification, e.g. nolock(true); others might not?
+ // (no example yet)
+ kRequiresVerification = 0x1,
+
+ // Does this effect want to verify all function calls originating in
+ // functions having this effect?
+ kVerifyCalls = 0x2,
+
+ // Can verification inspect callees' implementations? (e.g. nolock: yes,
+ // tcb+types: no)
+ kInferrableOnCallees = 0x4,
+
+ // Language constructs which effects can diagnose as disallowed.
+ kExcludeThrow = 0x8,
+ kExcludeCatch = 0x10,
+ kExcludeObjCMessageSend = 0x20,
+ kExcludeStaticLocalVars = 0x40,
+ kExcludeThreadLocalVars = 0x80
+ };
+
+private:
+ const EffectType Type_;
+ const Flags Flags_;
+ const char *Name;
+
+public:
+ using CalleeDeclOrType =
+ llvm::PointerUnion<const Decl *, const FunctionProtoType *>;
+
+ FunctionEffect(EffectType T, Flags F, const char *Name)
+ : Type_(T), Flags_(F), Name(Name) {}
+ virtual ~FunctionEffect();
+
+ /// The type of the effect.
+ EffectType type() const { return Type_; }
+
+ /// Flags describing behaviors of the effect.
+ Flags flags() const { return Flags_; }
+
+ /// The description printed in diagnostics, e.g. 'nolock'.
+ StringRef name() const { return Name; }
+
+ /// The description used by TypePrinter, e.g. __attribute__((clang_nolock))
+ virtual std::string attribute() const = 0;
+
+ /// Return true if adding or removing the effect as part of a type conversion
+ /// should generate a diagnostic.
+ virtual bool diagnoseConversion(bool Adding, QualType OldType,
+ FunctionEffectSet OldFX, QualType NewType,
+ FunctionEffectSet NewFX) const;
+
+ /// Return true if adding or removing the effect in a redeclaration should
+ /// generate a diagnostic.
+ virtual bool diagnoseRedeclaration(bool Adding,
+ const FunctionDecl &OldFunction,
+ FunctionEffectSet OldFX,
+ const FunctionDecl &NewFunction,
+ FunctionEffectSet NewFX) const;
+
+ /// Return true if adding or removing the effect in a C++ virtual method
+ /// override should generate a diagnostic.
+ virtual bool diagnoseMethodOverride(bool Adding,
+ const CXXMethodDecl &OldMethod,
+ FunctionEffectSet OldFX,
+ const CXXMethodDecl &NewMethod,
+ FunctionEffectSet NewFX) const;
+
+ /// Return true if the effect is allowed to be inferred on the specified Decl
+ /// (may be a FunctionDecl or BlockDecl). Only used if the effect has
+ /// kInferrableOnCallees flag set. Example: This allows nolock(false) to
+ /// prevent inference for the function.
+ virtual bool canInferOnDecl(const Decl *Caller,
+ FunctionEffectSet CallerFX) const;
+
+ // Called if kVerifyCalls flag is set; return false for success. When true is
+ // returned for a direct call, then the kInferrableOnCallees flag may trigger
+ // inference rather than an immediate diagnostic. Caller should be assumed to
+ // have the effect (it may not have it explicitly when inferring).
+ virtual bool diagnoseFunctionCall(bool Direct, const Decl *Caller,
+ FunctionEffectSet CallerFX,
+ CalleeDeclOrType Callee,
+ FunctionEffectSet CalleeFX) const;
+};
+
+/// FunctionEffect subclass for nolock and noalloc (whose behaviors are close
+/// to identical).
+class NoLockNoAllocEffect : public FunctionEffect {
+ bool isNoLock() const { return type() == kNoLockTrue; }
+
+public:
+ static const NoLockNoAllocEffect &nolock_instance();
+ static const NoLockNoAllocEffect &noalloc_instance();
+
+ NoLockNoAllocEffect(EffectType Type, const char *Name);
+ ~NoLockNoAllocEffect() override;
+
+ std::string attribute() const override;
+
+ bool diagnoseConversion(bool Adding, QualType OldType,
+ FunctionEffectSet OldFX, QualType NewType,
+ FunctionEffectSet NewFX) const override;
+
+ bool diagnoseRedeclaration(bool Adding, const FunctionDecl &OldFunction,
+ FunctionEffectSet OldFX,
+ const FunctionDecl &NewFunction,
+ FunctionEffectSet NewFX) const override;
+
+ bool diagnoseMethodOverride(bool Adding, const CXXMethodDecl &OldMethod,
+ FunctionEffectSet OldFX,
+ const CXXMethodDecl &NewMethod,
+ FunctionEffectSet NewFX) const override;
+
+ bool canInferOnDecl(const Decl *Caller,
+ FunctionEffectSet CallerFX) const override;
+
+ bool diagnoseFunctionCall(bool Direct, const Decl *Caller,
+ FunctionEffectSet CallerFX, CalleeDeclOrType Callee,
+ FunctionEffectSet CalleeFX) const override;
+};
+
} // namespace clang
#endif // LLVM_CLANG_AST_TYPE_H
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 0ba172a4035fdb..1d5d8f696977e7 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -326,6 +326,10 @@ let Class = FunctionProtoType in {
def : Property<"AArch64SMEAttributes", UInt32> {
let Read = [{ node->getAArch64SMEAttributes() }];
}
+ /* TODO: How to serialize FunctionEffect / FunctionEffectSet?
+ def : Property<"functionEffects", FunctionEffectSet> {
+ let Read = [{ node->getFunctionEffects() }];
+ }*/
def : Creator<[{
auto extInfo = FunctionType::ExtInfo(noReturn, hasRegParm, regParm,
@@ -342,6 +346,7 @@ let Class = FunctionProtoType in {
epi.ExtParameterInfos =
extParameterInfo.empty() ? nullptr : extParameterInfo.data();
epi.AArch64SMEAttributes = AArch64SMEAttributes;
+ //epi.FunctionEffects = functionEffects;
return ctx.getFunctionType(returnType, parameters, epi);
}]>;
}
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index fd7970d0451acd..4033b9efb86f39 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1402,6 +1402,29 @@ def CXX11NoReturn : InheritableAttr {
let Documentation = [CXX11NoReturnDocs];
}
+def NoLock : DeclOrTypeAttr {
+ let Spellings = [CXX11<"clang", "nolock">,
+ C23<"clang", "nolock">,
+ GNU<"clang_nolock">];
+
+ // Subjects - not needed?
+ //let Subjects = SubjectList<[FunctionLike, Block, TypedefName], ErrorDiag>;
+ let Args = [DefaultBoolArgument<"Cond", /*default*/1>];
+ let HasCustomParsing = 0;
+ let Documentation = [NoLockNoAllocDocs];
+}
+
+def NoAlloc : DeclOrTypeAttr {
+ let Spellings = [CXX11<"clang", "noalloc">,
+ C23<"clang", "noalloc">,
+ GNU<"clang_noalloc">];
+ // Subjects - not needed?
+ //let Subjects = SubjectList<[FunctionLike, Block, TypedefName], ErrorDiag>;
+ let Args = [DefaultBoolArgument<"Cond", /*default*/1>];
+ let HasCustomParsing = 0;
+ let Documentation = [NoLockNoAllocDocs];
+}
+
// Similar to CUDA, OpenCL attributes do not receive a [[]] spelling because
// the specification does not expose them with one currently.
def OpenCLKernel : InheritableAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 2c07cd09b0d5b7..bb897c708ebbea 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7973,3 +7973,20 @@ requirement:
}
}];
}
+
+def NoLockNoAllocDocs : Documentation {
+ let Category = DocCatType;
+ let Content = [{
+The ``nolock`` and ``noalloc`` attributes can be attached to functions, blocks,
+function pointers, lambdas, and member functions. The attributes identify code
+which must not allocate memory or lock, and the compiler uses the attributes to
+verify these requirements.
+
+Like ``noexcept``, ``nolock`` and ``noalloc`` have an optional argument, a
+compile-time constant boolean expression. By default, the argument is true, so
+``[[clang::nolock(true)]]`` is equivalent to ``[[clang::nolock]]``, and declares
+the function type as never locking.
+
+TODO: how much of the RFC to include here? Make it a separate page?
+ }];
+}
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 3f14167d6b8469..0d6e9f0289f6bc 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1507,3 +1507,7 @@ def ReadOnlyPlacementChecks : DiagGroup<"read-only-types">;
// Warnings and fixes to support the "safe buffers" programming model.
def UnsafeBufferUsageInContainer : DiagGroup<"unsafe-buffer-usage-in-container">;
def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer]>;
+
+// Warnings and notes related to the function effects system underlying
+// the nolock and noalloc attributes.
+def FunctionEffects : DiagGroup<"function-effects">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c54105507753eb..df553a47a8dbe7 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10778,6 +10778,101 @@ def warn_imp_cast_drops_unaligned : Warning<
"implicit cast from type %0 to type %1 drops __unaligned qualifier">,
InGroup<DiagGroup<"unaligned-qualifier-implicit-cast">>;
+def warn_func_effect_allocates : Warning<
+ "'%0' function '%1' must not allocate or deallocate memory">,
+ InGroup<FunctionEffects>;
+
+def note_func_effect_allocates : Note<
+ "'%1' cannot be inferred '%0' because it allocates/deallocates memory">;
+
+def warn_func_effect_throws_or_catches : Warning<
+ "'%0' function '%1' must not throw or catch exceptions">,
+ InGroup<FunctionEffects>;
+
+def note_func_effect_throws_or_catches : Note<
+ "'%1' cannot be inferred '%0' because it throws or catches exceptions">;
+
+def warn_func_effect_has_static_local : Warning<
+ "'%0' function '%1' must not have sta...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Doug Wyatt (dougsonos) ChangesRough first draft. This is an early PR to solicit comments on the overall approach and a number of outstanding questions. Some of the larger ones, "earlier" in the flow (from parse -> Sema):
Patch is 116.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84983.diff 26 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index a5879591f4c659..0460f30ce8a8b4 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -3008,6 +3008,13 @@ class FunctionDecl : public DeclaratorDecl,
/// computed and stored.
unsigned getODRHash() const;
+ FunctionEffectSet getFunctionEffects() const {
+ const auto *FPT = getType()->getAs<FunctionProtoType>();
+ if (FPT)
+ return FPT->getFunctionEffects();
+ return {};
+ }
+
// Implement isa/cast/dyncast/etc.
static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) {
@@ -4633,6 +4640,8 @@ class BlockDecl : public Decl, public DeclContext {
SourceRange getSourceRange() const override LLVM_READONLY;
+ FunctionEffectSet getFunctionEffects() const;
+
// Implement isa/cast/dyncast/etc.
static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) { return K == Block; }
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1942b0e67f65a3..de21561ce5dcad 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -4047,8 +4047,12 @@ class FunctionType : public Type {
LLVM_PREFERRED_TYPE(bool)
unsigned HasArmTypeAttributes : 1;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned HasFunctionEffects : 1;
+
FunctionTypeExtraBitfields()
- : NumExceptionType(0), HasArmTypeAttributes(false) {}
+ : NumExceptionType(0), HasArmTypeAttributes(false),
+ HasFunctionEffects(false) {}
};
/// The AArch64 SME ACLE (Arm C/C++ Language Extensions) define a number
@@ -4181,6 +4185,131 @@ class FunctionNoProtoType : public FunctionType, public llvm::FoldingSetNode {
}
};
+class FunctionEffect;
+class FunctionEffectSet;
+
+// It is the user's responsibility to keep this in set form: elements are
+// ordered and unique.
+// We could hide the mutating methods which are capable of breaking the
+// invariant, but they're needed and safe when used with STL set algorithms.
+class MutableFunctionEffectSet : public SmallVector<const FunctionEffect *, 4> {
+public:
+ using SmallVector::insert;
+ using SmallVector::SmallVector;
+
+ /// Maintains order/uniquenesss.
+ void insert(const FunctionEffect *effect);
+
+ MutableFunctionEffectSet &operator|=(FunctionEffectSet rhs);
+};
+
+class FunctionEffectSet {
+public:
+ // These sets will tend to be very small (1 element), so represent them as
+ // sorted vectors, which are compatible with the STL set algorithms. Using an
+ // array or vector also means the elements are contiguous, keeping iterators
+ // simple.
+
+private:
+ // 'Uniqued' refers to the set itself being uniqued. Storage is allocated
+ // separately. Use ArrayRef for its iterators. Subclass so as to be able to
+ // compare (it seems ArrayRef would silently convert itself to a vector for
+ // comparison?!).
+ class UniquedAndSortedFX : public llvm::ArrayRef<const FunctionEffect *> {
+ public:
+ using Base = llvm::ArrayRef<const FunctionEffect *>;
+
+ UniquedAndSortedFX(Base Array) : Base(Array) {}
+ UniquedAndSortedFX(const FunctionEffect **Ptr, size_t Len)
+ : Base(Ptr, Len) {}
+
+ bool operator<(const UniquedAndSortedFX &rhs) const;
+ };
+
+ // Could have used a TinyPtrVector if it were unique-able.
+ // Empty set has a null Impl.
+ llvm::PointerUnion<const FunctionEffect *, const UniquedAndSortedFX *> Impl;
+
+ explicit FunctionEffectSet(const FunctionEffect *Single) : Impl(Single) {}
+ explicit FunctionEffectSet(const UniquedAndSortedFX *Multi) : Impl(Multi) {}
+
+public:
+ using Differences =
+ SmallVector<std::pair<const FunctionEffect *, bool /*added*/>>;
+
+ FunctionEffectSet() : Impl(nullptr) {}
+
+ void *getOpaqueValue() const { return Impl.getOpaqueValue(); }
+
+ explicit operator bool() const { return !empty(); }
+ bool empty() const {
+ if (Impl.isNull())
+ return true;
+ if (const UniquedAndSortedFX *Vec =
+ dyn_cast_if_present<const UniquedAndSortedFX *>(Impl))
+ return Vec->empty();
+ return false;
+ }
+ size_t size() const {
+ if (empty())
+ return 0;
+ if (isa<const FunctionEffect *>(Impl))
+ return 1;
+ return cast<const UniquedAndSortedFX *>(Impl)->size();
+ }
+
+ using iterator = const FunctionEffect *const *;
+
+ iterator begin() const {
+ if (isa<const FunctionEffect *>(Impl))
+ return Impl.getAddrOfPtr1();
+ return cast<const UniquedAndSortedFX *>(Impl)->begin();
+ }
+
+ iterator end() const {
+ if (isa<const FunctionEffect *>(Impl))
+ return begin() + (Impl.isNull() ? 0 : 1);
+ return cast<const UniquedAndSortedFX *>(Impl)->end();
+ }
+
+ ArrayRef<const FunctionEffect *> items() const { return {begin(), end()}; }
+
+ // Since iterators are non-trivial and sets are very often empty,
+ // encourage short-circuiting loops for the empty set.
+ // void for_each(llvm::function_ref<void(const FunctionEffect*)> func) const;
+
+ bool operator==(const FunctionEffectSet &other) const {
+ return Impl == other.Impl;
+ }
+ bool operator!=(const FunctionEffectSet &other) const {
+ return Impl != other.Impl;
+ }
+ bool operator<(const FunctionEffectSet &other) const;
+
+ void dump(llvm::raw_ostream &OS) const;
+
+ /// Factory functions: return instances with uniqued implementations.
+ static FunctionEffectSet create(const FunctionEffect &single) {
+ return FunctionEffectSet{&single};
+ }
+ static FunctionEffectSet create(llvm::ArrayRef<const FunctionEffect *> items);
+
+ /// Union. Caller should check for incompatible effects.
+ FunctionEffectSet operator|(const FunctionEffectSet &rhs) const;
+ /// Intersection.
+ MutableFunctionEffectSet operator&(const FunctionEffectSet &rhs) const;
+ /// Difference.
+ MutableFunctionEffectSet operator-(const FunctionEffectSet &rhs) const;
+
+ /// Caller should short-circuit by checking for equality first.
+ static Differences differences(const FunctionEffectSet &Old,
+ const FunctionEffectSet &New);
+
+ /// Extract the effects from a Type if it is a BlockType or FunctionProtoType,
+ /// or pointer to one.
+ static FunctionEffectSet get(const Type &TyRef);
+};
+
/// Represents a prototype with parameter type info, e.g.
/// 'int foo(int)' or 'int foo(void)'. 'void' is represented as having no
/// parameters, not as having a single void parameter. Such a type can have
@@ -4195,7 +4324,8 @@ class FunctionProtoType final
FunctionProtoType, QualType, SourceLocation,
FunctionType::FunctionTypeExtraBitfields,
FunctionType::FunctionTypeArmAttributes, FunctionType::ExceptionType,
- Expr *, FunctionDecl *, FunctionType::ExtParameterInfo, Qualifiers> {
+ Expr *, FunctionDecl *, FunctionType::ExtParameterInfo,
+ FunctionEffectSet, Qualifiers> {
friend class ASTContext; // ASTContext creates these.
friend TrailingObjects;
@@ -4226,6 +4356,9 @@ class FunctionProtoType final
// an ExtParameterInfo for each of the parameters. Present if and
// only if hasExtParameterInfos() is true.
//
+ // * Optionally, a FunctionEffectSet. Present if and only if
+ // hasFunctionEffects() is true.
+ //
// * Optionally a Qualifiers object to represent extra qualifiers that can't
// be represented by FunctionTypeBitfields.FastTypeQuals. Present if and only
// if hasExtQualifiers() is true.
@@ -4284,6 +4417,7 @@ class FunctionProtoType final
ExceptionSpecInfo ExceptionSpec;
const ExtParameterInfo *ExtParameterInfos = nullptr;
SourceLocation EllipsisLoc;
+ FunctionEffectSet FunctionEffects;
ExtProtoInfo()
: Variadic(false), HasTrailingReturn(false),
@@ -4301,7 +4435,8 @@ class FunctionProtoType final
bool requiresFunctionProtoTypeExtraBitfields() const {
return ExceptionSpec.Type == EST_Dynamic ||
- requiresFunctionProtoTypeArmAttributes();
+ requiresFunctionProtoTypeArmAttributes() ||
+ FunctionEffects;
}
bool requiresFunctionProtoTypeArmAttributes() const {
@@ -4349,6 +4484,10 @@ class FunctionProtoType final
return hasExtParameterInfos() ? getNumParams() : 0;
}
+ unsigned numTrailingObjects(OverloadToken<FunctionEffectSet>) const {
+ return hasFunctionEffects();
+ }
+
/// Determine whether there are any argument types that
/// contain an unexpanded parameter pack.
static bool containsAnyUnexpandedParameterPack(const QualType *ArgArray,
@@ -4450,6 +4589,7 @@ class FunctionProtoType final
EPI.RefQualifier = getRefQualifier();
EPI.ExtParameterInfos = getExtParameterInfosOrNull();
EPI.AArch64SMEAttributes = getAArch64SMEAttributes();
+ EPI.FunctionEffects = getFunctionEffects();
return EPI;
}
@@ -4661,6 +4801,18 @@ class FunctionProtoType final
return false;
}
+ bool hasFunctionEffects() const {
+ if (!hasExtraBitfields())
+ return false;
+ return getTrailingObjects<FunctionTypeExtraBitfields>()->HasFunctionEffects;
+ }
+
+ FunctionEffectSet getFunctionEffects() const {
+ if (hasFunctionEffects())
+ return *getTrailingObjects<FunctionEffectSet>();
+ return {};
+ }
+
bool isSugared() const { return false; }
QualType desugar() const { return QualType(this, 0); }
@@ -7754,6 +7906,145 @@ QualType DecayedType::getPointeeType() const {
void FixedPointValueToString(SmallVectorImpl<char> &Str, llvm::APSInt Val,
unsigned Scale);
+// ------------------------------------------------------------------------------
+
+// TODO: Should FunctionEffect be located elsewhere, where Decl is not
+// forward-declared?
+class Decl;
+class CXXMethodDecl;
+
+/// Represents an abstract function effect.
+class FunctionEffect {
+public:
+ enum EffectType {
+ kGeneric,
+ kNoLockTrue,
+ kNoAllocTrue,
+ };
+
+ /// Flags describing behaviors of the effect.
+ using Flags = unsigned;
+ enum FlagBit : unsigned {
+ // Some effects require verification, e.g. nolock(true); others might not?
+ // (no example yet)
+ kRequiresVerification = 0x1,
+
+ // Does this effect want to verify all function calls originating in
+ // functions having this effect?
+ kVerifyCalls = 0x2,
+
+ // Can verification inspect callees' implementations? (e.g. nolock: yes,
+ // tcb+types: no)
+ kInferrableOnCallees = 0x4,
+
+ // Language constructs which effects can diagnose as disallowed.
+ kExcludeThrow = 0x8,
+ kExcludeCatch = 0x10,
+ kExcludeObjCMessageSend = 0x20,
+ kExcludeStaticLocalVars = 0x40,
+ kExcludeThreadLocalVars = 0x80
+ };
+
+private:
+ const EffectType Type_;
+ const Flags Flags_;
+ const char *Name;
+
+public:
+ using CalleeDeclOrType =
+ llvm::PointerUnion<const Decl *, const FunctionProtoType *>;
+
+ FunctionEffect(EffectType T, Flags F, const char *Name)
+ : Type_(T), Flags_(F), Name(Name) {}
+ virtual ~FunctionEffect();
+
+ /// The type of the effect.
+ EffectType type() const { return Type_; }
+
+ /// Flags describing behaviors of the effect.
+ Flags flags() const { return Flags_; }
+
+ /// The description printed in diagnostics, e.g. 'nolock'.
+ StringRef name() const { return Name; }
+
+ /// The description used by TypePrinter, e.g. __attribute__((clang_nolock))
+ virtual std::string attribute() const = 0;
+
+ /// Return true if adding or removing the effect as part of a type conversion
+ /// should generate a diagnostic.
+ virtual bool diagnoseConversion(bool Adding, QualType OldType,
+ FunctionEffectSet OldFX, QualType NewType,
+ FunctionEffectSet NewFX) const;
+
+ /// Return true if adding or removing the effect in a redeclaration should
+ /// generate a diagnostic.
+ virtual bool diagnoseRedeclaration(bool Adding,
+ const FunctionDecl &OldFunction,
+ FunctionEffectSet OldFX,
+ const FunctionDecl &NewFunction,
+ FunctionEffectSet NewFX) const;
+
+ /// Return true if adding or removing the effect in a C++ virtual method
+ /// override should generate a diagnostic.
+ virtual bool diagnoseMethodOverride(bool Adding,
+ const CXXMethodDecl &OldMethod,
+ FunctionEffectSet OldFX,
+ const CXXMethodDecl &NewMethod,
+ FunctionEffectSet NewFX) const;
+
+ /// Return true if the effect is allowed to be inferred on the specified Decl
+ /// (may be a FunctionDecl or BlockDecl). Only used if the effect has
+ /// kInferrableOnCallees flag set. Example: This allows nolock(false) to
+ /// prevent inference for the function.
+ virtual bool canInferOnDecl(const Decl *Caller,
+ FunctionEffectSet CallerFX) const;
+
+ // Called if kVerifyCalls flag is set; return false for success. When true is
+ // returned for a direct call, then the kInferrableOnCallees flag may trigger
+ // inference rather than an immediate diagnostic. Caller should be assumed to
+ // have the effect (it may not have it explicitly when inferring).
+ virtual bool diagnoseFunctionCall(bool Direct, const Decl *Caller,
+ FunctionEffectSet CallerFX,
+ CalleeDeclOrType Callee,
+ FunctionEffectSet CalleeFX) const;
+};
+
+/// FunctionEffect subclass for nolock and noalloc (whose behaviors are close
+/// to identical).
+class NoLockNoAllocEffect : public FunctionEffect {
+ bool isNoLock() const { return type() == kNoLockTrue; }
+
+public:
+ static const NoLockNoAllocEffect &nolock_instance();
+ static const NoLockNoAllocEffect &noalloc_instance();
+
+ NoLockNoAllocEffect(EffectType Type, const char *Name);
+ ~NoLockNoAllocEffect() override;
+
+ std::string attribute() const override;
+
+ bool diagnoseConversion(bool Adding, QualType OldType,
+ FunctionEffectSet OldFX, QualType NewType,
+ FunctionEffectSet NewFX) const override;
+
+ bool diagnoseRedeclaration(bool Adding, const FunctionDecl &OldFunction,
+ FunctionEffectSet OldFX,
+ const FunctionDecl &NewFunction,
+ FunctionEffectSet NewFX) const override;
+
+ bool diagnoseMethodOverride(bool Adding, const CXXMethodDecl &OldMethod,
+ FunctionEffectSet OldFX,
+ const CXXMethodDecl &NewMethod,
+ FunctionEffectSet NewFX) const override;
+
+ bool canInferOnDecl(const Decl *Caller,
+ FunctionEffectSet CallerFX) const override;
+
+ bool diagnoseFunctionCall(bool Direct, const Decl *Caller,
+ FunctionEffectSet CallerFX, CalleeDeclOrType Callee,
+ FunctionEffectSet CalleeFX) const override;
+};
+
} // namespace clang
#endif // LLVM_CLANG_AST_TYPE_H
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 0ba172a4035fdb..1d5d8f696977e7 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -326,6 +326,10 @@ let Class = FunctionProtoType in {
def : Property<"AArch64SMEAttributes", UInt32> {
let Read = [{ node->getAArch64SMEAttributes() }];
}
+ /* TODO: How to serialize FunctionEffect / FunctionEffectSet?
+ def : Property<"functionEffects", FunctionEffectSet> {
+ let Read = [{ node->getFunctionEffects() }];
+ }*/
def : Creator<[{
auto extInfo = FunctionType::ExtInfo(noReturn, hasRegParm, regParm,
@@ -342,6 +346,7 @@ let Class = FunctionProtoType in {
epi.ExtParameterInfos =
extParameterInfo.empty() ? nullptr : extParameterInfo.data();
epi.AArch64SMEAttributes = AArch64SMEAttributes;
+ //epi.FunctionEffects = functionEffects;
return ctx.getFunctionType(returnType, parameters, epi);
}]>;
}
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index fd7970d0451acd..4033b9efb86f39 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1402,6 +1402,29 @@ def CXX11NoReturn : InheritableAttr {
let Documentation = [CXX11NoReturnDocs];
}
+def NoLock : DeclOrTypeAttr {
+ let Spellings = [CXX11<"clang", "nolock">,
+ C23<"clang", "nolock">,
+ GNU<"clang_nolock">];
+
+ // Subjects - not needed?
+ //let Subjects = SubjectList<[FunctionLike, Block, TypedefName], ErrorDiag>;
+ let Args = [DefaultBoolArgument<"Cond", /*default*/1>];
+ let HasCustomParsing = 0;
+ let Documentation = [NoLockNoAllocDocs];
+}
+
+def NoAlloc : DeclOrTypeAttr {
+ let Spellings = [CXX11<"clang", "noalloc">,
+ C23<"clang", "noalloc">,
+ GNU<"clang_noalloc">];
+ // Subjects - not needed?
+ //let Subjects = SubjectList<[FunctionLike, Block, TypedefName], ErrorDiag>;
+ let Args = [DefaultBoolArgument<"Cond", /*default*/1>];
+ let HasCustomParsing = 0;
+ let Documentation = [NoLockNoAllocDocs];
+}
+
// Similar to CUDA, OpenCL attributes do not receive a [[]] spelling because
// the specification does not expose them with one currently.
def OpenCLKernel : InheritableAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 2c07cd09b0d5b7..bb897c708ebbea 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7973,3 +7973,20 @@ requirement:
}
}];
}
+
+def NoLockNoAllocDocs : Documentation {
+ let Category = DocCatType;
+ let Content = [{
+The ``nolock`` and ``noalloc`` attributes can be attached to functions, blocks,
+function pointers, lambdas, and member functions. The attributes identify code
+which must not allocate memory or lock, and the compiler uses the attributes to
+verify these requirements.
+
+Like ``noexcept``, ``nolock`` and ``noalloc`` have an optional argument, a
+compile-time constant boolean expression. By default, the argument is true, so
+``[[clang::nolock(true)]]`` is equivalent to ``[[clang::nolock]]``, and declares
+the function type as never locking.
+
+TODO: how much of the RFC to include here? Make it a separate page?
+ }];
+}
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 3f14167d6b8469..0d6e9f0289f6bc 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1507,3 +1507,7 @@ def ReadOnlyPlacementChecks : DiagGroup<"read-only-types">;
// Warnings and fixes to support the "safe buffers" programming model.
def UnsafeBufferUsageInContainer : DiagGroup<"unsafe-buffer-usage-in-container">;
def UnsafeBufferUsage : DiagGroup<"unsafe-buffer-usage", [UnsafeBufferUsageInContainer]>;
+
+// Warnings and notes related to the function effects system underlying
+// the nolock and noalloc attributes.
+def FunctionEffects : DiagGroup<"function-effects">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c54105507753eb..df553a47a8dbe7 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10778,6 +10778,101 @@ def warn_imp_cast_drops_unaligned : Warning<
"implicit cast from type %0 to type %1 drops __unaligned qualifier">,
InGroup<DiagGroup<"unaligned-qualifier-implicit-cast">>;
+def warn_func_effect_allocates : Warning<
+ "'%0' function '%1' must not allocate or deallocate memory">,
+ InGroup<FunctionEffects>;
+
+def note_func_effect_allocates : Note<
+ "'%1' cannot be inferred '%0' because it allocates/deallocates memory">;
+
+def warn_func_effect_throws_or_catches : Warning<
+ "'%0' function '%1' must not throw or catch exceptions">,
+ InGroup<FunctionEffects>;
+
+def note_func_effect_throws_or_catches : Note<
+ "'%1' cannot be inferred '%0' because it throws or catches exceptions">;
+
+def warn_func_effect_has_static_local : Warning<
+ "'%0' function '%1' must not have sta...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
You should advertise this elsewhere (e.g. in RFC thread), or remove the draft status so that reviewers can see this as something they should provide feedback on. |
…system's local one)
There was a problem hiding this comment.
I was following this on Discourse, so I thought I’d take a look; overall, this seems like a very interesting addition to Clang to me.
This is by no means a complete review; this pr is pretty big, so looking at everything in detail would take me a lot longer than just an hour or two, so I’ve just pointed out things that stood out to me. I’m also not particularly familiar with the analysis-based-warnings part of Clang, which is where most of the code for this pr seems to live, so I won’t be of much help there.
|
Does it make sense to have some C testcases too? Likewise some testcases testing the attribute style attribute? |
That’s in |
Hmm, this is a difficult question. Generally, I would probably prefer to just hard-code whatever effects we need right now, perhaps as something like a bitmask; from what I recall, none of the effects take arguments or anything like that, so that would be possible in that case (these are just my personal observations, though; I don’t have every last detail of this feature memorised, so if I’m missing something obvious, please let me know); my main concern is that the current implementation might be a bit too general. In particular, this seems like a lot of—if not too much—machinery for implementing a grand total of two function attributes. Because, sure, making it extensible doesn’t sound like a bad idea on paper, but adding any effects would likely require significant modifications to other parts of the compiler as well, so if a situation should arise where we do need to handle more complex effects, we should be able to refactor it whenever that comes up. For the time being, a simpler effect system may serve us better (and should also be more straight-forward to refactor if need be). Perhaps, let’s say that, unless it’s likely that we may end up adding effects that require more storage than a single bit or two in the foreseeable future, then I’d rather just have it be a bitmask. The way this is implemented currently does remind me at least in part of attributes, but attributes are very varied and do take arguments, so I’m not sure the two systems are really comparable. @AaronBallman I recall you having some opinions on adding data to |
Er, I’ll see if I can find some time to take a look at this, but I’m not sure what the problem is off the top of my head. |
|
One question since the attribute is applied to types is there a way to get the nolock/noalloc from type?. Will the above work or is there no way to implement that currently? Since you mention it is attached to the type, is it mangled then differently. e.g.: Does the above f calls to 2 different functions? |
This example is problematic because in this construct, But let's try this: This showed:
That bug defeats
I added a test to the one that checks an AST dump to make sure attributes are parsed into the right places - |
Opened an issue for this (#85120) because it really does seem like a bug to me. |
Co-authored-by: Shafik Yaghmour <[email protected]>
…ndent contexts. - Tweaking tests - things that came up in review.
- DiagnoseUnexpandedParameterPack() now handled generically elsewhere - err_attribute_argument_type: the name of the attribute shouldn't be manually quoted; there is an overall consistency problem to be solved later.
|
ping |
@dougsonos I've had good success when I ping specific people to re-review by tagging them with their github user names! The people on the reviewers list may be a good place to start next time you're allowed to ping |
|
Just for the record, @AaronBallman said at our last meeting that he’ll try and find some time to help finish up this pr, so you hopefully won’t have to wait much longer ;Þ |
|
Aaron is attending WG14 meeting that is happening this week, so you shouldn't expect him until next week. |
…ments appear in AST dump)
|
@dougsonos Alright, I think it’s finally time for us to try and see if we can merge this. I see you’ve merged main recently, so I’m not expecting Clang to explode after we do this or anything, but it is a fairly large and substantial change; if you get any emails about buildbot failures (because iirc only the pr author gets those), then please post them here if any of them seem related to your changes. |
|
@dougsonos Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
This change has some compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=5a997c1d83845cd891c1e4662cb7ddb02c1eaecc&to=f03cb005eb4ba3c6fb645aca2228e907db8cd452&stat=instructions:u There is a regression of about 0.25% for unoptimized builds. It also regresses time to build clang by 0.5%. Given that this is for a clang extension and not a conformance issue, I'm inclined to revert. |
I was not expecting that. Hmm, I wonder what’s causing this.
It might make sense to do that, yeah. Either way, we should investigate what’s going on here. @AaronBallman wdyt? |
I’m probably just stating the obvious here, but seeing as existing code obviously does not make use of effects, for compiling it to become slower, that means that we’re probably unconditionally executing code somewhere irrespective of whether effects are present or not, so what I’m saying is we should probably investigate any places where this pr checks for the presence of effects and does something based on that and move the checks for whether a function even has effects attached to it as far up as possible and try and also optimise that check (is the function that checks for that defined in a header? If not, we should maybe move the definition there so it can be inlined). The only other option I can think of is that this adds some more data to |
Definitely worth investigating, unsure whether this is sufficiently disruptive to warrant a revert as opposed to a fix forward. I don't oppose a revert if @nikic would like to see one, but I realize now that we have no wording in our revert policy regarding incremental compile time performance regressions (if it was a huge regression, I think it falls under correctness, but this is a relatively small change in performance and no bots are red as a result either). So if we think this warrants a revert, should we consider updating the policy to more clearly state when to revert for performance reasons? |
That makes sense. I remember thinking a couple of times that I was working on a function that might be better inlined, but changing Type.h triggered long rebuilds.
Well, it adds two more types of trailing objects so nothing more gets allocated in the case of no effects. Possibly the trailing objects mechanism itself gets a bit slower though. I'll dig in and investigate today. |
|
I've got fully optimized builds of the commit before this one and this one, and I do see a small but repeatable performance difference when compiling clang's own SemaDecl.cpp. That difference goes away if I completely remove the new trailing objects from FunctionProtoType (and make everything trying to access them a no-op): There's some big X factor in the reproducibility of my at-desk testing :-/ Here's the same statistics for the nonblocking commit vs. the previous one: Those baselines should be identical! |
|
Thank you for investigating! So it sounds like the act of adding an additional trailing object will slow down the performance regardless of whether we use the new trailing object? If so, there's not much we can do about that as part of this patch -- we might want to explore improving performance as a follow-up though, if possible, because we'd like those trailing objects to be (approximately) "free" if they're not being used. |
I was confused by some comments on I also found a hot method to inline. Aiming to open another PR today. |
Yeah, intuitively at least, moving more commonly used stuff further to the front does sound like it should improve performance, because even if the additional trailing objects are not there, we still have to check whether they’re there every time we want to get to the qualifiers. |
…) backend (#92460) Introducing the main runtime of realtime sanitizer. For more information, please see the [discourse thread](https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837). We have also put together a [reviewer support document](https://github.com/realtime-sanitizer/radsan/blob/doc/review-support/doc/review.md) to show what our intention is. This review introduces the sanitizer backend. This includes: * CMake build files (largely adapted from asan). * Main RTSan architecture (the external API, thread local context, stack). * Interceptors. * Many unit tests. Please see the [reviewer support document](https://github.com/realtime-sanitizer/radsan/blob/doc/review-support/doc/review.md) for what our next steps are. We are moving in lockstep with this PR #84983 for the codegen coming up next. Note to reviewers: If you see support documentation mention "RADSan", this was the "old acronym" for the realtime sanitizer, they refer to the same thing. If you see it let us know and we can correct it (especially in the llvm codebase) --------- Co-authored-by: David Trevelyan <[email protected]>
Introduce `nonblocking` and `nonallocating` attributes. RFC is here: https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837 This PR introduces the attributes, with some changes in Sema to deal with them as extensions to function (proto)types. There are some basic type checks, most importantly, a warning when trying to spoof the attribute (implicitly convert a function without the attribute to one that has it). A second, follow-on pull request will introduce new caller/callee verification. --------- Co-authored-by: Doug Wyatt <[email protected]> Co-authored-by: Shafik Yaghmour <[email protected]> Co-authored-by: Aaron Ballman <[email protected]> Co-authored-by: Sirraide <[email protected]>
…) backend (llvm#92460) Introducing the main runtime of realtime sanitizer. For more information, please see the [discourse thread](https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837). We have also put together a [reviewer support document](https://github.com/realtime-sanitizer/radsan/blob/doc/review-support/doc/review.md) to show what our intention is. This review introduces the sanitizer backend. This includes: * CMake build files (largely adapted from asan). * Main RTSan architecture (the external API, thread local context, stack). * Interceptors. * Many unit tests. Please see the [reviewer support document](https://github.com/realtime-sanitizer/radsan/blob/doc/review-support/doc/review.md) for what our next steps are. We are moving in lockstep with this PR llvm#84983 for the codegen coming up next. Note to reviewers: If you see support documentation mention "RADSan", this was the "old acronym" for the realtime sanitizer, they refer to the same thing. If you see it let us know and we can correct it (especially in the llvm codebase) --------- Co-authored-by: David Trevelyan <[email protected]>
(updated 3 May 2024)
Introduce
nonblockingandnonallocatingattributes. RFC is here: https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837This PR introduces the attributes, with some changes in Sema to deal with them as extensions to function (proto)types.
There are some basic type checks, most importantly, a warning when trying to spoof the attribute (implicitly convert a function without the attribute to one that has it).
A second, follow-on pull request will introduce new caller/callee verification.
This is an early PR to solicit comments on the overall approach and a number of outstanding questions. Some of the larger ones, "earlier" in the flow (from parse -> Sema):- Does the FunctionEffect / FunctionEffectSet abstraction make sense (Type.h)? The idea is that an abstract effect system for functions could easily support things like "TCB with types" or adding "nowait" to the "nolock/noalloc" group of effects.- FunctionEffect/FunctionEffectSet need to be serialized as part of FunctionProtoType- [x] Conversions between functions / function pointers with and without the attribute work in C++ but not in C and I'm a bit lost (there's a bunch of debug logging around this e.g. Sema::IsFunctionConversion and Sema::ImpCastExprToType)