Frontend doesn't detect mismatching uses of 'new' and 'delete'. Analyzer catches this case.
Emit warning, and recover, when:
int *p = new int[1]; delete p; // treat as 'delete[]' int *l = new int(1); delete[] l; // treat as 'delete'
rtrieu | |
jordan_rose | |
rsmith |
Frontend doesn't detect mismatching uses of 'new' and 'delete'. Analyzer catches this case.
Emit warning, and recover, when:
int *p = new int[1]; delete p; // treat as 'delete[]' int *l = new int(1); delete[] l; // treat as 'delete'
This seems like a really nice idea.
lib/Sema/SemaExprCXX.cpp | ||
---|---|---|
2246–2247 | You should probably delay this until the end of the TU, so you can do the check based on knowledge of the full set of constructors (in the common case where there exists a file that defines them all). Take a look at the way we implement the 'unused private field` warning for inspiration. | |
2249–2250 | I'm not sure that breaking out once you see any CXXNewExpr is the right approach. Consider: struct S { bool Array; T *P; S() : Array(false), P(new T) {} S(int N) : Array(true), P(new T[N]) {} ~S() { if (Array) delete [] P; else delete P; } }; Here, whether we get a diagnostic, and which one we get, will depend on which constructor we happen to visit first. | |
2251 | You need to map to the definition of CD here. | |
2253 | You should walk over single-element InitListExprs here, for cases like : p{new T}, and IgnoreParenImpCasts, for cases like Base *p; [...] : p(new Derived). | |
2405 | This is not OK as recovery from a warning, especially not a warning with false positives. |
Updated patch to address following issues:
include/clang/Sema/Sema.h | ||
---|---|---|
393–394 | This is not a reasonable way to track the relevant state here. The interesting cases are VarDecls and FieldDecls. For VarDecls, at the point where we see the delete, you can immediately check whether you have an new-expression as an initializer (and don't worry about the obscure cases where an initializer comes after the use). For FieldDecls, perform the check where you see the delete, and if you see either (a) no constructor with a matching new, or (b) only constructors with the wrong kind of new, then add your delete to a list to check at the end of the TU. That way, in almost all cases we can deal with the diagnostic immediately rather than at end of TU. (This avoids building up a big set of all delete expressions.) You'll also need to figure out how you're going to handle serialization/deserialization here, in the case where you hit one of these "not sure" cases in a PCH or similar. To that end, you should probably store a list of {SourceLocation, FieldDecl} pairs, or -- better -- a map from FieldDecl to the first source location where we saw a delete. (You'll need one extra bit to describe whether it was delete or delete[] I suppose...). |
include/clang/Sema/Sema.h | ||
---|---|---|
393–394 | I have implemented the first half of the patch, slightly different than your suggestion. We diagnose this immediately for VarDecl, as you said. For FieldDecls, delete expression is queued, if at least one constructor isn't defined (yet). Otherwise (if all constructor definitions are visible), warning is issued in ActOnCXXDelete right before function returns. Could you please explain the rationale for your point (b)? If we can see all constructors while analyzing a CXXDeleteExpr, we can reason about constructors and in-class initializers immediately. If no constructor initializes the member, we check in-class initializer (if exists). If all constructors initialize member with mismatching type of new, then we can diagnose them all immediately, instead of postponing it to the end of translation unit. I can think of only one case (aside from PCH case) that requires postponing this analysis to the end of translation unit; that is, when we cannot see at least one constructor's definition. I have a few questions regarding PCH/external source. I have no prior experience with ASTReader/Writer at all. Sorry, if my questions don't make any sense. Do you mean we should handle following case: // header; compiled as pch. #ifndef header struct X { int *a; ~X() { delete a; } // not sure X(); }; #endif // source file, includes above pch #include "header" X::X() : a(new int[1]) { } We encounter with a delete-expr in a PCH. We can't prove anything, because we can't see definition of X::X() yet. Even if there was an in-class initializer, that wouldn't help, exactly for the same reason. X::X() will be visible only at the end of translation unit. Therefore, we must write this information somewhere in PCH so that we can load it at the end of translation unit. I have followed UndefinedButUsed as a serialization/deserialization example. As far as I understood, I need to write a record, and read it back. I blindly followed your suggestion here :) This record contains, as you said, FieldDecl, SourceLocation and a bit indicating array form. I put them in an std::tuple<FieldDecl *, SourceLocation, bool>. Can that "bit" be represented as APInt(width=1)? I did, roughly: RecordData DeleteExprsToAnalyze; AddDeclRef(get<0>(tuple), DeleteExprsToAnalyze); AddSource(get<1>(tuple), DeleteExprsToAnalyze); AddAPInt(APInt(1, get<2>(tuple)), DeleteExprsToAnalyze); // ... if (!DeleteExprsToAnalyze.empty()) Stream.EmitRecord(DELETE_EXPRS_TO_ANALYZE, DeleteExprsToAnalyze); I am trying the PCH part now. I would like to learn the right way, as I might be doing something completely wrong. |
include/clang/Sema/Sema.h | ||
---|---|---|
393–394 |
The idea is: if you haven't seen all the constructors yet, but you have seen a constructor that used the right kind of new, then there's no need to queue the expression; the delete is OK.
This should probably be: if any constructor does not explicitly initialize the member, check the in-class initializer. (But it's probably redundant, since the constructor's initializer list will have been extended to contain a reference to the in-class initializer in this case.)
This is wasteful; DeleteExprsToAnalyze is a SmallVector of integers. Just do DeleteExprsToAnalyze.push_back(get<2>(tuple));. Other than that, this looks like the right approach. |
This is looking really good. Some fairly minor comments...
include/clang/Sema/Sema.h | ||
---|---|---|
8406–8408 | This should probably say "with the array form of 'delete'." Also, the function's return type is void; it won't return false. | |
lib/Sema/Sema.cpp | ||
1489–1492 ↗ | (On Diff #14188) | Can you avoid making a copy of the DenseMap here? |
lib/Sema/SemaExprCXX.cpp | ||
2254–2255 | \brief only goes on the first line. | |
2279 | Typo "eturn" | |
2356 | IgnoreParenImpCasts would seem better here; you don't find parenthesized new-expressions here. | |
2417–2419 | This seems like overspecification; just say that we require a field or a delete expression. | |
2475–2476 | Maybe pass Detector.NewExprs into DiagnoseMismatchedNewDelete and have it produce the notes as well as the error? That'd remove a bit of duplication between this function and the next one. | |
lib/Serialization/ASTWriter.cpp | ||
4347 ↗ | (On Diff #14188) | This writes out the map in a nondeterministic order. Maybe use an llvm::MapVector? |
include/clang/Sema/Sema.h | ||
---|---|---|
8406–8408 | Yes, the reason is.. it was the function that used to detect the mismatch. I should have written documentation for the latest revision. I have written them for myself, primarily, because I've got interrupted frequently while making this patch. | |
lib/Sema/SemaExprCXX.cpp | ||
2254–2255 | I vaguely recall that is how multi-line brief was supposed to be, but I can't seem to find that on doxygen documentation. | |
2459 | Do you have concerns about this function call? |
Addressed comments:
lib/Sema/SemaExprCXX.cpp | ||
---|---|---|
2293 | Remove this member and pass DE into analyzeDeleteExpr, which is the only place that uses it. | |
2357–2362 | Reverse the order of these checks to remove the redundancy: if (isa<ILE>(E) && 1 init) E = ILE->getInit(0)->IgnoreParenImpCasts() auto *NE = dyn_cast<CXXNewExpr>(E); | |
2379–2380 | Please give this a better name, saying what you're actually analyzing and what the return value might mean. Suggestion: switch from returning false if you find a matching *new-expression* to returning true and rename this to hasMatchingNewInCtor (and rename analyzeCtorInitializer to hasMatchingNewInCtorInit). | |
2397–2411 | The logic in here is half dealing with in-class initializers and half finalizing the result of your analysis for the field. It would be better to split this up so that this function doesn't do things that are unrelated to in-class initializers. | |
lib/Serialization/ASTReader.cpp | ||
3208 ↗ | (On Diff #14249) | Use const uint64_t Count = Record[I++]; here. |
lib/Serialization/ASTWriter.cpp | ||
4347 ↗ | (On Diff #14249) | APInt is overkill here. Just use DeleteExprsToAnalyze.push_back(DeleteExprsInfo.second.size()). |
Addressed:
include/clang/Sema/ExternalSemaSource.h | ||
---|---|---|
31 ↗ | (On Diff #16299) | Please order this before CXXConstructorDecl. |
include/clang/Sema/ExternalSemaSource.h | ||
---|---|---|
31 ↗ | (On Diff #16299) | Shouldn't CXXDeleteExpr come right after CXXConstructorDecl, and before CXXRecordDecl to preserve alphabetical ordering? |