[RFC] Initial implementation of P2719#113510
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ojhunt
force-pushed
the
users/ojhunt/P2719
branch
2 times, most recently
from
341b6f2 to
24c8b4a
Compare
|
Thanks for working on this. FYI most reviewers at the LLVM conference, do not expect a lot of feedback this week |
|
@cor3ntin oh yeah, I know, I'm also there :D |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Oliver Hunt (ojhunt) ChangesThis is a basic implementation of P2719: "Type-aware allocation and deallocation functions" described at http://wg21.link/P2719 The proposal includes some more details but the basic change in functionality is the addition of support for an additional implicit parameter in operators void operator new(std::type_identity<int>, size_t); However this becomes more powerful by specifying templated declarations, for example template <typename T> void operator new(std::type_identity<T>, size_t); Where the operators being resolved will be the concrete type being operated over (NB. A completely unconstrained global definition as above is not recommended as it triggers many problems similar to a general override of the global operators). These type aware operators can be declared as either free functions or in class, and can be specified with or without the other implicit parameters, with overload resolution performed according to the existing standard parameter prioritisation, only with type parameterised operators having higher precedence than non-type aware operators. The only exception is destroying_delete which for reasons discussed in the paper we do not support type-aware variants by default. In order to ensure interchangability with existing operators, this implementation does extend the definition of a The implementation adds a new This also adds a separate flag to support the application of the proposal to the destroying delete, The implementation includes some error checking and warnings to try to detect basic errors, but the problem of detecting mismatch type-polymorphic new and delete is a problem to be explored in future. Patch is 138.06 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/113510.diff 37 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 7ff35d73df5997..5235ace380bd9e 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -2530,6 +2530,8 @@ class FunctionDecl : public DeclaratorDecl,
/// Determine whether this is a destroying operator delete.
bool isDestroyingOperatorDelete() const;
+ bool IsTypeAwareOperatorNewOrDelete() const;
+
/// Compute the language linkage.
LanguageLinkage getLanguageLinkage() const;
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index cfe3938f83847b..65696ee57822ed 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -2234,6 +2234,17 @@ enum class CXXNewInitializationStyle {
Braces
};
+struct ImplicitAllocationParameters {
+ bool PassTypeIdentity;
+ bool PassAlignment;
+};
+
+struct ImplicitDeallocationParameters {
+ bool PassTypeIdentity;
+ bool PassAlignment;
+ bool PassSize;
+};
+
/// Represents a new-expression for memory allocation and constructor
/// calls, e.g: "new CXXNewExpr(foo)".
class CXXNewExpr final
@@ -2289,7 +2300,7 @@ class CXXNewExpr final
/// Build a c++ new expression.
CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
- FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
+ FunctionDecl *OperatorDelete, ImplicitAllocationParameters IAP,
bool UsualArrayDeleteWantsSize, ArrayRef<Expr *> PlacementArgs,
SourceRange TypeIdParens, std::optional<Expr *> ArraySize,
CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
@@ -2304,7 +2315,7 @@ class CXXNewExpr final
/// Create a c++ new expression.
static CXXNewExpr *
Create(const ASTContext &Ctx, bool IsGlobalNew, FunctionDecl *OperatorNew,
- FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
+ FunctionDecl *OperatorDelete, ImplicitAllocationParameters IAP,
bool UsualArrayDeleteWantsSize, ArrayRef<Expr *> PlacementArgs,
SourceRange TypeIdParens, std::optional<Expr *> ArraySize,
CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
@@ -2393,6 +2404,13 @@ class CXXNewExpr final
return const_cast<CXXNewExpr *>(this)->getPlacementArg(I);
}
+ unsigned getNumImplicitArgs() const {
+ unsigned ImplicitArgCount = 1; // Size
+ ImplicitArgCount += passAlignment();
+ ImplicitArgCount += passTypeIdentity();
+ return ImplicitArgCount;
+ }
+
bool isParenTypeId() const { return CXXNewExprBits.IsParenTypeId; }
SourceRange getTypeIdParens() const {
return isParenTypeId() ? getTrailingObjects<SourceRange>()[0]
@@ -2431,6 +2449,12 @@ class CXXNewExpr final
/// the allocation function.
bool passAlignment() const { return CXXNewExprBits.ShouldPassAlignment; }
+ /// Indicates whether a type_identity tag should be implicitly passed to
+ /// the allocation function.
+ bool passTypeIdentity() const {
+ return CXXNewExprBits.ShouldPassTypeIdentity;
+ }
+
/// Answers whether the usual array deallocation function for the
/// allocated type expects the size of the allocation as a
/// parameter.
@@ -2438,6 +2462,13 @@ class CXXNewExpr final
return CXXNewExprBits.UsualArrayDeleteWantsSize;
}
+ /// Provides the full set of information about expected implicit
+ /// parameters in this call
+ ImplicitAllocationParameters implicitAllocationParameters() const {
+ return ImplicitAllocationParameters{.PassTypeIdentity = passTypeIdentity(),
+ .PassAlignment = passAlignment()};
+ }
+
using arg_iterator = ExprIterator;
using const_arg_iterator = ConstExprIterator;
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 83fafbabb1d460..8717b2b3e84846 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -890,6 +890,10 @@ class alignas(void *) Stmt {
LLVM_PREFERRED_TYPE(bool)
unsigned ShouldPassAlignment : 1;
+ /// Should the type identity be passed to the allocation function?
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned ShouldPassTypeIdentity : 1;
+
/// If this is an array allocation, does the usual deallocation
/// function for the allocated type want to know the allocated size?
LLVM_PREFERRED_TYPE(bool)
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index ba3161c366f4d9..8af03e2afdd455 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2635,6 +2635,9 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
// C++14 decltype(auto)
bool isTypedefNameType() const; // typedef or alias template
+ bool isTypeIdentitySpecialization() const; // std::type_identity<X> for any X
+ bool isDestroyingDeleteT() const; // std::destroying_delete_t
+
#define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix) \
bool is##Id##Type() const;
#include "clang/Basic/OpenCLImageTypes.def"
@@ -2699,6 +2702,8 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
return static_cast<TypeDependence>(TypeBits.Dependence);
}
+ TemplateDecl *getSpecializedTemplateDecl() const;
+
/// Whether this type is an error type.
bool containsErrors() const {
return getDependence() & TypeDependence::Error;
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 72eada50a56cc9..7fa46023593466 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1524,6 +1524,10 @@ in addition with the pragmas or -fmax-tokens flag to get any warnings.
}];
}
+// Warning group for type aware allocators
+def TypeAwareAllocatorMismatch :
+ DiagGroup<"type-aware-allocator-mismatch">;
+
def WebAssemblyExceptionSpec : DiagGroup<"wasm-exception-spec">;
def RTTI : DiagGroup<"rtti">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e4718008ece72..90fad76c4af112 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9688,6 +9688,18 @@ def err_operator_delete_param_type : Error<
def err_destroying_operator_delete_not_usual : Error<
"destroying operator delete can have only an optional size and optional "
"alignment parameter">;
+def err_type_aware_destroying_operator_delete : Error<
+ "type aware destroying delete is not permitted">;
+def err_unsupported_type_aware_allocator : Error<
+ "type aware allocation operators are disabled">;
+def err_no_matching_type_aware_cleanup_deallocator_mismatch : Error<
+ "type aware %0 requires matching %1 in %2">;
+def err_type_aware_operator_found : Note<
+ "type aware %0 found in %1">;
+def warn_mismatching_type_aware_cleanup_deallocator : Warning<
+ "mismatching type aware allocation operators for constructor cleanup">, InGroup<TypeAwareAllocatorMismatch>;
+def note_type_aware_operator_declared : Note<
+ "%select{|non-}0type aware %1 declared here">;
def note_implicit_delete_this_in_destructor_here : Note<
"while checking implicit 'delete this' for virtual destructor">;
def err_builtin_operator_new_delete_not_usual : Error<
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index 7f5d26118bdc71..a9ff58fb94ebc6 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -307,6 +307,10 @@ EXTENSION(datasizeof, LangOpts.CPlusPlus)
FEATURE(cxx_abi_relative_vtable, LangOpts.CPlusPlus && LangOpts.RelativeCXXABIVTables)
+// Type aware allocators
+FEATURE(cxx_type_aware_allocators, LangOpts.TypeAwareAllocators)
+FEATURE(cxx_type_aware_destroying_delete, LangOpts.TypeAwareDestroyingDelete)
+
// CUDA/HIP Features
FEATURE(cuda_noinline_keyword, LangOpts.CUDA)
EXTENSION(cuda_implicit_host_device_templates, LangOpts.CUDA && LangOpts.OffloadImplicitHostDeviceTemplates)
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 68db400c22e6c1..768e8a683babd0 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -313,6 +313,8 @@ LANGOPT(OpenACC , 1, 0, "OpenACC Enabled")
LANGOPT(MSVCEnableStdcMacro , 1, 0, "Define __STDC__ with '-fms-compatibility'")
LANGOPT(SizedDeallocation , 1, 0, "sized deallocation")
+LANGOPT(TypeAwareAllocators , 1, 1, "type aware C++ allocation operators")
+LANGOPT(TypeAwareDestroyingDelete , 1, 1, "type aware C++ destroying delete")
LANGOPT(AlignedAllocation , 1, 0, "aligned allocation")
LANGOPT(AlignedAllocationUnavailable, 1, 0, "aligned allocation functions are unavailable")
LANGOPT(NewAlignOverride , 32, 0, "maximum alignment guaranteed by '::operator new(size_t)'")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 2ddb2f5312148e..c88fb6c948c54a 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3533,7 +3533,14 @@ def : Separate<["-"], "fnew-alignment">, Alias<fnew_alignment_EQ>;
def : Flag<["-"], "faligned-new">, Alias<faligned_allocation>;
def : Flag<["-"], "fno-aligned-new">, Alias<fno_aligned_allocation>;
def faligned_new_EQ : Joined<["-"], "faligned-new=">;
-
+defm cxx_type_aware_allocators : BoolFOption<"cxx-type-aware-allocators",
+ LangOpts<"TypeAwareAllocators">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption], "Enable C++YY type aware allocator operators">,
+ NegFlag<SetFalse>, BothFlags<[], [ClangOption, CC1Option]>>;
+defm cxx_type_aware_destroying_delete : BoolFOption<"cxx-type-aware-destroying-delete",
+ LangOpts<"TypeAwareDestroyingDelete">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption], "Enable C++YY type aware allocator operators">,
+ NegFlag<SetFalse>, BothFlags<[], [ClangOption, CC1Option]>>;
def fobjc_legacy_dispatch : Flag<["-"], "fobjc-legacy-dispatch">, Group<f_Group>;
def fobjc_new_property : Flag<["-"], "fobjc-new-property">, Group<clang_ignored_f_Group>;
defm objc_infer_related_result_type : BoolFOption<"objc-infer-related-result-type",
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 9e6b04bc3f8f7c..05d416cdf7b3fd 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2135,6 +2135,10 @@ class Sema final : public SemaBase {
isConstantEvaluatedOverride;
}
+ bool AllowTypeAwareAllocators() const {
+ return getLangOpts().TypeAwareAllocators && !isConstantEvaluatedContext();
+ }
+
SourceLocation getLocationOfStringLiteralByte(const StringLiteral *SL,
unsigned ByteNo) const;
@@ -4749,6 +4753,15 @@ class Sema final : public SemaBase {
CXXRecordDecl *getStdBadAlloc() const;
EnumDecl *getStdAlignValT() const;
+ ClassTemplateDecl *getStdTypeIdentity() const;
+ std::optional<QualType> InstantiateSpecializedTypeIdentity(QualType Subject);
+ bool IsTypeIdentitySpecialization(QualType Type) const;
+ bool IsTypeAwareOperatorNewOrDelete(const FunctionDecl *FnDecl) const;
+ bool IsTypeAwareOperatorNewOrDelete(const FunctionTemplateDecl *FnDecl) const;
+ bool IsTypeAwareOperatorNewOrDelete(const NamedDecl *FnDecl) const;
+ std::optional<FunctionDecl *>
+ InstantiateTypeAwareUsualDelete(FunctionTemplateDecl *FnDecl,
+ QualType AllocType);
ValueDecl *tryLookupUnambiguousFieldDecl(RecordDecl *ClassDecl,
const IdentifierInfo *MemberOrBase);
@@ -7933,6 +7946,10 @@ class Sema final : public SemaBase {
/// The C++ "type_info" declaration, which is defined in \<typeinfo>.
RecordDecl *CXXTypeInfoDecl;
+ /// The C++ "std::type_identity" template class, which is defined by the C++
+ /// standard library.
+ LazyDeclPtr StdTypeIdentity;
+
/// A flag to remember whether the implicit forms of operator new and delete
/// have been declared.
bool GlobalNewDeleteDeclared;
@@ -8126,7 +8143,7 @@ class Sema final : public SemaBase {
/// The scope in which to find allocation functions.
enum AllocationFunctionScope {
- /// Only look for allocation functions in the global scope.
+ /// Only look for allocation functions in the global scope
AFS_Global,
/// Only look for allocation functions in the scope of the
/// allocated class.
@@ -8138,14 +8155,12 @@ class Sema final : public SemaBase {
/// Finds the overloads of operator new and delete that are appropriate
/// for the allocation.
- bool FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
- AllocationFunctionScope NewScope,
- AllocationFunctionScope DeleteScope,
- QualType AllocType, bool IsArray,
- bool &PassAlignment, MultiExprArg PlaceArgs,
- FunctionDecl *&OperatorNew,
- FunctionDecl *&OperatorDelete,
- bool Diagnose = true);
+ bool FindAllocationFunctions(
+ SourceLocation StartLoc, SourceRange Range,
+ AllocationFunctionScope NewScope, AllocationFunctionScope DeleteScope,
+ QualType AllocType, bool IsArray, ImplicitAllocationParameters &IAP,
+ MultiExprArg PlaceArgs, FunctionDecl *&OperatorNew,
+ FunctionDecl *&OperatorDelete, bool Diagnose = true);
/// DeclareGlobalNewDelete - Declare the global forms of operator new and
/// delete. These are:
@@ -8176,11 +8191,11 @@ class Sema final : public SemaBase {
bool FindDeallocationFunction(SourceLocation StartLoc, CXXRecordDecl *RD,
DeclarationName Name, FunctionDecl *&Operator,
- bool Diagnose = true, bool WantSize = false,
- bool WantAligned = false);
- FunctionDecl *FindUsualDeallocationFunction(SourceLocation StartLoc,
- bool CanProvideSize,
- bool Overaligned,
+ QualType DeallocType,
+ ImplicitDeallocationParameters,
+ bool Diagnose = true);
+ FunctionDecl *FindUsualDeallocationFunction(QualType, SourceLocation StartLoc,
+ ImplicitDeallocationParameters,
DeclarationName Name);
FunctionDecl *FindDeallocationFunctionForDestructor(SourceLocation StartLoc,
CXXRecordDecl *RD);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 549c864dc91ef2..c1f0f87ec0aeba 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -1141,7 +1141,7 @@ class CXXAllocatorCall : public AnyFunctionCall {
/// C++17 aligned operator new() calls that have alignment implicitly
/// passed as the second argument, and to 1 for other operator new() calls.
unsigned getNumImplicitArgs() const {
- return getOriginExpr()->passAlignment() ? 2 : 1;
+ return getOriginExpr()->getNumImplicitArgs();
}
unsigned getNumArgs() const override {
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index e7a6509167f0a0..5bddd0741b17dc 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -8313,10 +8313,10 @@ ExpectedStmt ASTNodeImporter::VisitCXXNewExpr(CXXNewExpr *E) {
return CXXNewExpr::Create(
Importer.getToContext(), E->isGlobalNew(), ToOperatorNew,
- ToOperatorDelete, E->passAlignment(), E->doesUsualArrayDeleteWantSize(),
- ToPlacementArgs, ToTypeIdParens, ToArraySize, E->getInitializationStyle(),
- ToInitializer, ToType, ToAllocatedTypeSourceInfo, ToSourceRange,
- ToDirectInitRange);
+ ToOperatorDelete, E->implicitAllocationParameters(),
+ E->doesUsualArrayDeleteWantSize(), ToPlacementArgs, ToTypeIdParens,
+ ToArraySize, E->getInitializationStyle(), ToInitializer, ToType,
+ ToAllocatedTypeSourceInfo, ToSourceRange, ToDirectInitRange);
}
ExpectedStmt ASTNodeImporter::VisitCXXDeleteExpr(CXXDeleteExpr *E) {
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 8321cee0e0bc94..08618f6b38d3cd 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3358,6 +3358,12 @@ bool FunctionDecl::isReservedGlobalPlacementOperator() const {
return false;
const auto *proto = getType()->castAs<FunctionProtoType>();
+ if (proto->getNumParams() < 2)
+ return false;
+ bool IsTypeAwareAllocator =
+ proto->getParamType(0)->isTypeIdentitySpecialization();
+ if (IsTypeAwareAllocator)
+ return false;
if (proto->getNumParams() != 2 || proto->isVariadic())
return false;
@@ -3483,15 +3489,40 @@ bool FunctionDecl::isDestroyingOperatorDelete() const {
// Within a class C, a single object deallocation function with signature
// (T, std::destroying_delete_t, <more params>)
// is a destroying operator delete.
- if (!isa<CXXMethodDecl>(this) || getOverloadedOperator() != OO_Delete ||
- getNumParams() < 2)
+ if (!isa<CXXMethodDecl>(this) || getOverloadedOperator() != OO_Delete)
+ return false;
+
+ auto NumParams = getNumParams();
+ unsigned DestroyingDeleteTagParam = 1;
+ bool IsTypeAware = false;
+ if (NumParams > 0)
+ IsTypeAware = getParamDecl(0)->getType()->isTypeIdentitySpecialization();
+
+ if (IsTypeAware)
+ ++DestroyingDeleteTagParam;
+
+ if (getNumParams() <= DestroyingDeleteTagParam)
return false;
- auto *RD = getParamDecl(1)->getType()->getAsCXXRecordDecl();
+ auto *RD =
+ getParamDecl(DestroyingDeleteTagParam)->getType()->getAsCXXRecordDecl();
return RD && RD->isInStdNamespace() && RD->getIdentifier() &&
RD->getIdentifier()->isStr("destroying_delete_t");
}
+bool FunctionDecl::IsTypeAwareOperatorNewOrDelete() const {
+ if (getDeclName().getNameKind() != DeclarationName::CXXOperatorName)
+ return false;
+ if (getDeclName().getCXXOverloadedOperator() != OO_New &&
+ getDeclName().getCXXOverloadedOperator() != OO_Delete &&
+ getDeclName().getCXXOverloadedOperator() != OO_Array_New &&
+ getDeclName().getCXXOverloadedOperator() != OO_Array_Delete)
+ return false;
+ if (getNumParams() < 2)
+ return false;
+ return getParamDecl(0)->getType()->isTypeIdentitySpecialization();
+}
+
LanguageLinkage FunctionDecl::getLanguageLinkage() const {
return getDeclLanguageLinkage(*this);
}
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 17ebee0d6b22fa..5d88f92fc89379 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -2474,11 +2474,39 @@ bool CXXMethodDecl::isUsualDeallocationFunction(
getOverloadedOperator() != OO_Array_Delete)
return false;
+ auto NumParams = getNumParams();
+ bool IsTypeAware = IsTypeAwareOperatorNewOrDelete();
+
// C++ [basic.stc.dynamic.deallocation]p2:
// A template instance is never a usual deallocation function,
// regardless of its signature.
- if (getPrimaryTemplate())
- return false;
+ if (auto *PrimaryTemplate = getPrimaryTemplate()) {
+ // Addendum: a template instance is a usual deallocation function if there
+ // is a single template parameter, that parameter is a type, only the first
+ // parameter is dependent, and that parameter is a specialization of
+ // std::type_identity
+ if (!IsTypeAware) {
+ // Stop early on if the specialization is not explicitly type aware
+ return false;
+ }
+
+ auto *SpecializedDecl = PrimaryTemplate->getTemplatedDecl();
+ if (!SpecializedDecl->IsTypeAwareOperatorNewOrDelete()) {
+ // The underlying template decl must also be explicitly type aware
+ // e.g. template <typename T> void operator delete(T, ...)
+ // specialized on T=type_identity<X> is not usual
+ return false;
+ }
+
+ for (unsigned Idx = 1; Idx < NumParams; ++Idx) {
+ if (SpecializedDecl->getParamDecl(Idx)-...
[truncated]
|
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
I cant find my comment, but I see the args structure isn't actually stored anywhere except as params, so feel free to disregard that comment.
| Previous.clear(); | ||
| } | ||
|
|
||
| // I think DC check should be DC->isStdNamespace()? |
There was a problem hiding this comment.
These were questions for reviewers regarding the existing code, not something I wanted to change in this PR :D
There was a problem hiding this comment.
No description provided.
There was a problem hiding this comment.
@erichkeane so I think this was a case where the test can fail incorrectly (albeit not in practice) and I was wanting confirmation of the fix, though I think the change should not be part of this PR :D
cor3ntin
changed the title
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Thank you for working on this!
The changes should come with release notes in clang/docs/ReleaseNotes.rst so users know about the new functionality.
Unless this is slated for inclusion in C++26, it should probably also come with user-facing documentation in the language extensions file.
| // once MSVC implements it. | ||
| if (R.getLookupName().getCXXOverloadedOperator() == OO_Array_New && | ||
| S.Context.getLangOpts().MSVCCompat) { | ||
| S.Context.getLangOpts().MSVCCompat && Mode != ResolveMode::Typed) { |
There was a problem hiding this comment.
Hmmmm it's a bit weird that we're changing MSVC compat mode when I don't think MSVC implements this functionality at all. I'm not opposed, but do we want to support this in MSVC compat mode?
There was a problem hiding this comment.
The intent here was to not involve MSVC compat mode at all while performing a typed look up, on the assumption that looking up a typed allocator would imply a modern enough msvc+codebase to not warrant it? I really don't know what the correct thing to do is as I think you could make equal arguments in both directions?
There was a problem hiding this comment.
Oh right, it does also raise the question of whether you should change the allocation type, e.g should we go from T to T[] for instance - I think that question was why I came down on this side.
There was a problem hiding this comment.
@AaronBallman you ok with me closing this?
This is a basic implementation of P2719: "Type-aware allocation and deallocation functions" described at http://wg21.link/P2719 The proposal includes some more details but the basic change in functionality is the addition of support for an additional implicit parameter in operators `new` and `delete` to act as a type tag. Tag is of type `std::type_identity<T>` where T is the concrete type being allocated. So for example, a custom type specific allocator for `int` say can be provided by the declaration of void *operator new(std::type_identity<int>, size_t); void operator delete(std::type_identity<int>, void*); However this becomes more powerful by specifying templated declarations, for example template <typename T> void *operator new(std::type_identity<T>, size_t); template <typename T> void operator delete(std::type_identity<T>, void*); Where the operators being resolved will be the concrete type being operated over (NB. A completely unconstrained global definition as above is not recommended as it triggers many problems similar to a general override of the global operators). These type aware operators can be declared as either free functions or in class, and can be specified with or without the other implicit parameters, with overload resolution performed according to the existing standard parameter prioritisation, only with type parameterised operators having higher precedence than non-type aware operators. The only exception is destroying_delete which for reasons discussed in the paper we do not support type-aware variants by default. In order to ensure interchangability with existing operators, this implementation does extend the definition of a `usual deallocation function` as suggested in the proposal to support templated operators, as long as the only dependent parameter is the type_identity tag type, and the tag is explicitly a specialization of std::type_identity The implementation adds a new `-fexperimental-cxx-type-aware-allocators` flag to enable the feature. This is detectable in source via the `cxx_type_aware_allocators` feature check. This also adds a separate flag to support the application of the proposal to the destroying delete, `-fexperimental-cxx-type-aware-destroying-delete`. The proposal currently does not support destroying delete as it adds some significant hazards, but we've received sufficient requests for the behavior that we included it so people can see if it is actually useful. This can be detected in source via the `cxx_type_aware_destroying_delete` feature flag. The implementation includes some error checking and warnings to try to detect basic errors, but the problem of detecting mismatch type-polymorphic new and delete is a problem to be explored in future.
ojhunt
force-pushed
the
users/ojhunt/P2719
branch
from
24c8b4a to
9730efc
Compare
Co-authored-by: Erich Keane <ekeane@nvidia.com>
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
|
Ok, so I've gone through all my GitHub settings, and am hoping for a comment or something, to see if GH will actually ping me this time :D |
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Just 1 suggestion, otherwise nothing the othres didnt come up with. I'm happy when everyone else is.
| SemaRef, MD, /*WasMalformed=*/nullptr)) { | ||
| QualType AddressParamType = | ||
| SemaRef.Context.getCanonicalType(MD->getParamDecl(1)->getType()); | ||
| if (AddressParamType != SemaRef.Context.VoidPtrTy && |
There was a problem hiding this comment.
I actually only went this route because it was not awfully hard and the improvement in diagnostics seemed worth it, and GCC does diagnose these incorrect definitions consistently (whereas we silently produce incorrect codegen in a bunch of cases :-/ )
| // once MSVC implements it. | ||
| if (R.getLookupName().getCXXOverloadedOperator() == OO_Array_New && | ||
| S.Context.getLangOpts().MSVCCompat) { | ||
| S.Context.getLangOpts().MSVCCompat && Mode != ResolveMode::Typed) { |
There was a problem hiding this comment.
@AaronBallman you ok with me closing this?
| "%select{|type aware }0%select{|destroying }1%2 takes type %5 (%4) as %select{first|second|third|fourth|fifth}3 parameter">; | ||
| def err_operator_new_default_arg: Error< | ||
| "parameter of %0 cannot have a default argument">; | ||
| def err_operator_delete_dependent_param_type : Error< | ||
| "%0 cannot take a dependent type as first parameter; use %1 instead">; | ||
| "%select{|type aware }0%select{|destroying }1%2 cannot take a dependent type as its %select{first|second|third|fourth|fifth}3 parameter; " | ||
| "use %4 instead">; | ||
| def err_operator_delete_param_type : Error< | ||
| "first parameter of %0 must have type %1">; | ||
| "%select{first|second|third|fourth|fifth}3 parameter of%select{| type aware}0%select{| destroying}1 %2 must have type %4">; |
There was a problem hiding this comment.
@AaronBallman so I think we need to"parameter X must be Y" for the error message to be meaningful, as otherwise the error message is just "this parameter must be of type Y", and you can only see which parameter is involved if you can see the pointer.
I will switch to %ordinal, but there are numerous cases of first|second|third etc in the diagnostics which should be switched over, but that also takes us to %ordinal using 1st, 2nd, 3rd, etc instead of first, second, third, which was why I did not use ordinal in the first place.
| return SemaRef.Diag(FnDecl->getLocation(), | ||
| diag::err_operator_new_delete_too_few_parameters) | ||
| << IsPotentiallyTypeAware << IsPotentiallyDestroyingDelete | ||
| << FnDecl->getDeclName() << MinimumMandatoryArgumentCount; |
| for (unsigned Idx = 0; Idx < MinimumMandatoryArgumentCount; ++Idx) { | ||
| const ParmVarDecl *ParamDecl = FnDecl->getParamDecl(Idx); | ||
| if (ParamDecl->hasDefaultArg()) | ||
| return SemaRef.Diag(FnDecl->getLocation(), |
| int ComparisonResult = Best.Compare(S, Info, IDP); | ||
| if (ComparisonResult > 0) |
There was a problem hiding this comment.
@cor3ntin where should it go? Sema.h seems like it runs the risk of copious namespace pollution?
cor3ntin
left a comment
cor3ntin
left a comment
There was a problem hiding this comment.
I think the PR is in pretty good shape and it's becoming counter productive to nitpick this giant piece of work.
Thanks a lot for sticking with it. Hopefully the GitHub workflow can be less surprising in the future.
Looks Good To Me. Ship it.
|
We have tracked down a new and spurious "This variable is used uninitialized" to this PR. I'm working on a reduced test case now. It looks like this: There is no control flow on line XXX+1. |
hmmmm while you minimize I'll see if I can see any obvious path that would lead to this |
Does FileMode have custom operator new or delete? |
|
Yes it does. Here is a reduced repro: |
|
@Sterling-Augustine thank you! |
|
What is the time frame for a fix? Should we revert this change and until then? |
|
@Sterling-Augustine Oliver is looking into it. Given it's a relatively large patch, I'd rather wait a day until reverting to avoid churn. Will that work for you? |
|
I have a very immediate fix, but I'd rather a slightly nicer one - at some point refactoring exposed a problem in access control diagnostics, in a way that is very very weird and implies something else broken in the existing tree as well. If I can't work out the correct systemic fix in the next half hour or so I'll introduce a trivial behavioral fix and then work out the correct fix. Basically an access check that should pass fails, the pre-p2719 code path ignored that failure, so worked, p2719 unified a pile of diagnostics which means that the failing access check causes an error, however at the same time the failing access check does not actually report an error, so we get sadness. The underlying issues are: the access check fails erroneously, and the failed access check does not report an error, the trivial fix is restoring the "ignore the return value of the access check function" but if possible I'd like to just make sure that the access check does the right thing, I'll land "ignore the access check failure like we did in the past" in half an hour or so if I haven't worked out what the actual error is. |
|
That would be very helpful. Thank you. |
|
It's even more dumb, but I have this fixed and am writing tests |
|
We have tracked two more issues to this commit. Both only manifest when using Clang header modules, which likely means that AST serialization is somehow incorrect after this patch. I'm reducing one of the test cases now, but it's taking a lot of time. In the meantime, please take a look at the AST serialization in case you can spot any issue there. |
Will do, are you seeing a crash or codegen error? There were a bunch of serialization changes at the end so it's probable that I messed it up there (will be going through the serialization this evening) |
A lot of back and forth happened with tracking type awareness but also destroying delete, I'm currently wondering if the issue is that deserialization of a destroying delete failing to ensure correct ASTContext state |
|
There are two issues:
Both are only manifesting with header modules. |
Thanks for the prompt fix! #137102 resolves both the assertion failure and the bogus compilation error. |
| LLVM_PREFERRED_TYPE(AlignedAllocationMode) | ||
| unsigned PassAlignmentToPlacementDelete : 1; | ||
| LLVM_PREFERRED_TYPE(TypeAwareAllocationMode) | ||
| unsigned PassTypeToPlacementDelete : 1; |
There was a problem hiding this comment.
So this was flagged by static analysis b/c it is not initialized during construction. Is this intended for future use only? A comment would have been useful for future readers.
There was a problem hiding this comment.
I believe through refactoring and semantic changes to the feature this became irrelevant and I removed the uses but not the field. I've verified that we have the correct behavior, but before I remove the field I want to verify through code inspection as well.
There was a problem hiding this comment.
sorry this slipped my mind once I had verified it was just dead code instead of UB lying in wait.
Will push a PR shortly
11 participants
This is a basic implementation of P2719: "Type-aware allocation and deallocation functions" described at http://wg21.link/P2719
The proposal includes some more details but the basic change in functionality is the addition of support for an additional implicit parameter in operators
newanddeleteto act as a type tag. Tag is of typestd::type_identity<T>where T is the concrete type being allocated. So for example, a custom type specific allocator forintsay can be provided by the declaration ofvoid operator new(std::type_identity, size_t);
void operator delete(std::type_identity, void);
However this becomes more powerful by specifying templated declarations, for example
template void operator new(std::type_identity, size_t);
template void operator delete(std::type_identity, void);
Where the operators being resolved will be the concrete type being operated over (NB. A completely unconstrained global definition as above is not recommended as it triggers many problems similar to a general override of the global operators).
These type aware operators can be declared as either free functions or in class, and can be specified with or without the other implicit parameters, with overload resolution performed according to the existing standard parameter prioritisation, only with type parameterised operators having higher precedence than non-type aware operators. The only exception is destroying_delete which for reasons discussed in the paper we do not support type-aware variants by default.
In order to ensure interchangability with existing operators, this implementation does extend the definition of a
usual deallocation functionas suggested in the proposal to support templated operators, as long as the only dependent parameter is the type_identity tag type, and the tag is explicitly a specialization of std::type_identityThe implementation adds a new
-fexperimental-cxx-type-aware-allocatorsflag to enable the feature. This is detectable in source via thecxx_type_aware_allocatorsfeature check.This also adds a separate flag to support the application of the proposal to the destroying delete,
-fexperimental-cxx-type-aware-destroying-delete. The proposal currently does not support destroying delete as it adds some significant hazards, but we've received sufficient requests for the behavior that we included it so people can see if it is actually useful. This can be detected in source via thecxx_type_aware_destroying_deletefeature flag.The implementation includes some error checking and warnings to try to detect basic errors, but the problem of detecting mismatch type-polymorphic new and delete is a problem to be explored in future.
This change is