[cfe-commits] [PATCH] Instrument allocation to skim allocated types
Dai MIKURUBE
dmikurube at acm.org
Tue Jan 15 21:36:09 PST 2013
Ah, I can't submit my replying comments after I update the patch? Sorry about that. I think I have fixed almost all points that you commented.
For
> const FunctionDecl *getInterceptor() const { return Interceptor; }
> "Why 'const' in the return type here?"
I just followed getOperatorDelete(), but it may not be required. I'll remove it if needed.
================
Comment at: include/clang/AST/DeclCXX.h:2224
@@ -2223,2 +2223,3 @@
FunctionDecl *OperatorDelete;
+ FunctionDecl *Interceptor;
----------------
Dmitri Gribenko wrote:
> Please add a short documentation comment.
Done in the next upload.
================
Comment at: include/clang/AST/DeclCXX.h:2268
@@ -2266,1 +2267,3 @@
+ void setInterceptor(FunctionDecl *I) { Interceptor = I; }
+ const FunctionDecl *getInterceptor() const { return Interceptor; }
----------------
Dmitri Gribenko wrote:
> Why 'const' in the return type here?
Hmm, it just followed getOperatorDelete(), but it may not be required. I'll remove it if needed.
================
Comment at: include/clang/AST/ExprCXX.h:1614
@@ +1613,3 @@
+ /// allocation as a parameter.
+ bool doesInterceptorWantSize() const {
+ return InterceptorWantsSize;
----------------
Dmitri Gribenko wrote:
> Maybe rename to doesDeleteInterceptorWantSize() ? (And rename all similarly-named members, too.)
Done.
================
Comment at: include/clang/Sema/Sema.h:3849-3850
@@ +3848,4 @@
+
+ /// DeclareAllocationInterceptor - Declares a single implicit allocation
+ /// interceptor if it doesn't already exist.
+ void DeclareAllocationInterceptor(DeclarationName Name, bool UseSize);
----------------
Dmitri Gribenko wrote:
> Please don't duplicate function names in comments when adding new code.
Removed.
================
Comment at: lib/CodeGen/CGCXXABI.cpp:170
@@ -169,4 +169,3 @@
QualType elementType) {
- // If the class's usual deallocation function takes two arguments,
- // it needs a cookie.
- if (expr->doesUsualArrayDeleteWantSize())
+ // If the class's usual deallocation function or the allocation interceptor
+ // takes two arguments, it needs a cookie.
----------------
Dmitri Gribenko wrote:
> s/allocation interceptor/deallocation interceptor/ ?
Thank you for the good catch. Fixed.
================
Comment at: lib/CodeGen/CGCXXABI.cpp:180
@@ -178,4 +179,3 @@
bool CGCXXABI::requiresArrayCookie(const CXXNewExpr *expr) {
- // If the class's usual deallocation function takes two arguments,
- // it needs a cookie.
- if (expr->doesUsualArrayDeleteWantSize())
+ // If the class's usual deallocation function or the allocation interceptor
+ // takes two arguments, it needs a cookie.
----------------
Dmitri Gribenko wrote:
> Same here.
Thanks. Fixed.
================
Comment at: lib/CodeGen/CodeGenFunction.h:1848-1851
@@ -1839,1 +1847,6 @@
+ // Returns true if allocation should be intercepted.
+ bool ShouldInterceptAllocation(const FunctionDecl *Intercept);
+
+ // Emits the allocation interceptor.
+ RValue EmitInterceptAllocation(RValue PtrRV, const FunctionDecl *Interceptor,
----------------
Dmitri Gribenko wrote:
> Sorry, but these two comments are not really helpful -- they just rephrase the function name. If you have something to add to these comments, please do (and use three slashes then), otherwise I don't think there's a reason to keep these comments.
Removed the comments. Thanks.
================
Comment at: lib/Driver/Tools.cpp:2499-2500
@@ +2498,4 @@
+ CmdArgs.push_back("-fintercept-allocation");
+ CmdArgs.push_back("-include");
+ CmdArgs.push_back("typeinfo");
+ }
----------------
Dmitri Gribenko wrote:
> This is unfortunate. Is this required to ensure that type_info is declared? But Sema should take of this?
>
> If this is still required, please document the reason here.
I'm not completely sure, but #include <typeinfo> is required to use type_info even if we declare it in Sema. Added a comment.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:6186
@@ +6185,3 @@
+RecordDecl *Sema::getOrCreateCXXTypeInfoDecl() {
+ if (!CXXTypeInfoDecl) {
+ IdentifierInfo *TypeInfoII = &PP.getIdentifierTable().get("type_info");
----------------
Dmitri Gribenko wrote:
> Please reverse this condition to de-indent the code.
>
> if (CXXTypeInfoDecl)
> return CXXTypeInfoDecl;
> // Create it
Done.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:6189
@@ +6188,3 @@
+ LookupResult R(*this, TypeInfoII, SourceLocation(), LookupTagName);
+ NamespaceDecl* StdNamespace = getStdNamespace();
+ if (!StdNamespace)
----------------
Dmitri Gribenko wrote:
> Star should be next to the variable name.
Fixed.
================
Comment at: lib/Sema/SemaExprCXX.cpp:374
@@ -373,15 +373,3 @@
- if (!CXXTypeInfoDecl) {
- IdentifierInfo *TypeInfoII = &PP.getIdentifierTable().get("type_info");
- LookupResult R(*this, TypeInfoII, SourceLocation(), LookupTagName);
- LookupQualifiedName(R, getStdNamespace());
- CXXTypeInfoDecl = R.getAsSingle<RecordDecl>();
- // Microsoft's typeinfo doesn't have type_info in std but in the global
- // namespace if _HAS_EXCEPTIONS is defined to 0. See PR13153.
- if (!CXXTypeInfoDecl && LangOpts.MicrosoftMode) {
- LookupQualifiedName(R, Context.getTranslationUnitDecl());
- CXXTypeInfoDecl = R.getAsSingle<RecordDecl>();
- }
- if (!CXXTypeInfoDecl)
- return ExprError(Diag(OpLoc, diag::err_need_header_before_typeid));
- }
+ RecordDecl* TypeInfoDecl = getOrCreateCXXTypeInfoDecl();
+ if (!TypeInfoDecl)
----------------
Dmitri Gribenko wrote:
> Star should be next to the variable name.
Fixed.
================
Comment at: lib/Sema/SemaExprCXX.cpp:2017
@@ +2016,3 @@
+
+ RecordDecl* TypeInfoDecl = getOrCreateCXXTypeInfoDecl();
+ QualType TypeInfoType = Context.getTypeDeclType(TypeInfoDecl);
----------------
Dmitri Gribenko wrote:
> Star should be next to the variable name.
Fixed.
================
Comment at: lib/Sema/SemaExprCXX.cpp:2021
@@ +2020,3 @@
+ TypeInfoType.withConst());
+
+ // Check if this function is already declared.
----------------
Dmitri Gribenko wrote:
> You can declare
>
> unsigned NumParams = UseSize ? 3 : 2;
>
> here and simplify some conditionals below using NumParams.
Reasonable. Done it.
================
Comment at: lib/Sema/SemaExprCXX.cpp:2066
@@ +2065,3 @@
+
+ SmallVector<ParmVarDecl*, 16> Params;
+ ParmVarDecl *Param1 = ParmVarDecl::Create(Context, Interceptor,
----------------
Dmitri Gribenko wrote:
> Why 16 here? 3 is always sufficient.
Right. Fixed it.
================
Comment at: lib/Sema/SemaExprCXX.cpp:2364
@@ -2209,5 +2363,3 @@
- return Owned(new (Context) CXXDeleteExpr(Context.VoidTy, UseGlobal, ArrayForm,
- ArrayFormAsWritten,
- UsualArrayDeleteWantsSize,
- OperatorDelete, Ex.take(), StartLoc));
+ CXXDeleteExpr* Result =
+ new (Context) CXXDeleteExpr(Context.VoidTy, UseGlobal, ArrayForm,
----------------
Dmitri Gribenko wrote:
> Star should be next to the variable name.
Fixed.
================
Comment at: lib/Sema/SemaExprCXX.cpp:2097
@@ +2096,3 @@
+
+FunctionDecl* Sema::LookupAllocationInterceptor(char *InterceptorName,
+ SourceLocation StartLoc,
----------------
Dmitri Gribenko wrote:
> const char *InterceptorName or even a StringRef?
Fixed into const char*.
http://llvm-reviews.chandlerc.com/D298
More information about the cfe-commits
mailing list