[PATCH] D18073: Add memory allocating functions

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks for iterating on the patch! Some comments in-line.



================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:569
+  // allocating functions initialized to nullptr, which will never equal a
+  //non-null IdentifierInfo*, and never trigger on a non-Windows platform.
+  if (Ctx.getTargetInfo().getTriple().isWindowsMSVCEnvironment()) {
----------------
Please add a space here to align with the rest of the comment.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:622
+  if (FD->getKind() == Decl::Function) {
+    if(isStandardNewDelete(FD, C))
+      return false;
----------------
The identifier info is null for other operators as well (e.g., +). 
If you want to bail early when the identifier for a function is null, you should do so directly instead of trying to enumerate all cases.

For example:
```
if (!FunI)
  return false;
```
This will be future-proof in case additional additional kinds of function declarations are added without identifiers.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:648
+	  }
+    }
+
----------------
The indentation seems off here.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:664
+          && FunI == II_win_alloca)) {
+        assert(!isStandardNewDelete(FD, C) && "We should not reach this point"
+                                              "with a C++ operator.");
----------------
You should remove this assert.

Since you have guaranteed that you are targeting windows before checking and you initially II_win_alloca to a non-null value when targeting windows it can never be the case that II_win_alloca will be null here.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:814
+    initIdentifierInfo(C.getASTContext());
+    IdentifierInfo *FunI = FD->getIdentifier();
+
----------------
Do you want to do the same early bailout for an operator here also?


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:850
+               && FunI == II_realloc_dbg) &&
+               !isStandardNewDelete(FD, C.getASTContext())) {
+      State = ReallocMem(C, CE, false, State);
----------------
I don't think the `!isStandardNewDelete()` is needed here.

Also, this is triggering -Wlogical-op-parentheses for me when building with clang.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:859
+               FunI == II_calloc_dbg)) &&
+               !isStandardNewDelete(FD, C.getASTContext())) {
+      State = CallocMem(C, CE, State);
----------------
Same here.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:866
+               FunI == II_free_dbg)) &&
+               !isStandardNewDelete(FD, C.getASTContext())) {
+      State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
----------------
Same here.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:874
+               FunI == II_win_tempnam_dbg || FunI == II_wtempnam_dbg)) &&
+               !isStandardNewDelete(FD, C.getASTContext())) {
+      State = MallocUpdateRefState(C, CE, State);
----------------
Same here.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:881
+               FunI == II_win_alloca) &&
+               !isStandardNewDelete(FD, C.getASTContext())) {
+      if (CE->getNumArgs() < 1)
----------------
Same here.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1183
+
+  //const FunctionDecl *FD = C.getCalleeDecl(CE);
+  //const IdentifierInfo *FI = FD->getIdentifier();
----------------
You should remove the commented-out code.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1185
+  //const IdentifierInfo *FI = FD->getIdentifier();
+  assert(Att->getModule() != nullptr && "Only C++ operators should have a null"
+                                        "IdentifierInfo. We should not reach "
----------------
The explanation in the assert is not correct. This invariant holds because Sema guarantees that the module is not null when checking the attribute. See `handleOwnershipAttr()` for details.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1191
+      (Att->getModule() != II_malloc_dbg) &&
+	  !isStandardNewDelete(C.getCalleeDecl(CE), C.getASTContext()))
+    return nullptr;
----------------
The '!isStandardNewDelete` check doesn't make any sense here. The code is dealing with the identifier in specified attribute and not the name of the called function.


================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1285
+
+  assert(Att->getModule() != nullptr && "Only C++ operators should have a null"
+                                        "IdentifierInfo. We should not reach "
----------------
Same here.


================
Comment at: llvm/tools/clang/test/Analysis/alternative-malloc-api.c:88
+
+// What does "ContentIsDefined" refer to?
+void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
----------------
I don't know the answer to this. Perhaps @zaks.anna recalls -- it looks like she wrote the test that this was copied from. But I don't think this comment adds much, do you?


================
Comment at: llvm/tools/clang/test/Analysis/alternative-malloc-api.c:95
+
+// What does "ContentIsDefined" refer to?
+void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) {
----------------
Same here.


https://reviews.llvm.org/D18073