[PATCH] D19084: [scan-build] fix warnings emitted on Clang AST code base

Apelete Seketeli via cfe-commits cfe-commits at lists.llvm.org
Thu May 12 10:07:06 PDT 2016


apelete added inline comments.

================
Comment at: lib/AST/ASTDiagnostic.cpp:1686
@@ -1685,3 +1685,3 @@
 
-    if (Same) {
+    if (Same && FromTD) {
       OS << "template " << FromTD->getNameAsString();
----------------
rtrieu wrote:
> dblaikie wrote:
> > Should this be a condition, or just an assertion?
> This should be an assertion.  Same == true implies FromTD and ToTD are not null.
What do you mean by "This should be an assertion" ?
There's already an assertion on FromTD and ToTD values at the beginning of PrintTemplateTemplate() so duplicating it here didn't make sense to me: should I replace the if() with an assertion anyway ?

================
Comment at: lib/AST/ExprConstant.cpp:1992
@@ -1991,2 +1991,3 @@
                                         int64_t Adjustment) {
+  assert(E && "expression to be evaluated must be not NULL");
   CharUnits SizeOfPointee;
----------------
dblaikie wrote:
> Does the static analyzer assume any pointer parameter may be null? (I didn't think it did that - I thought it only assumed it could be null if there was a null test somewhere in the function?) That seems a bit too pessimistic in most codebases, especially LLVM - we pass a lot of non-null pointers around.
Not all pointer parameters trigger a warning during static analysis of Clang codebase, but for the ones that do the reason isn't always clear to me (I'm probably missing some background on static analysis here).

Let me know if you think the warning here should be ignored.

================
Comment at: lib/AST/NestedNameSpecifier.cpp:460
@@ +459,3 @@
+
+    assert(Buffer && "Buffer cannot be NULL");
+
----------------
dblaikie wrote:
> Again, is this assuming that the Buffer parameter may be null? That seems unlikely to be useful in the LLVM codebase where we have lots of guaranteed non-null pointers floating around (I'm surprised this wouldn't cause tons of analyzer warnings, so presumably it's something more subtle?)
Code path proposed by the analyzer is the following:

484. NestedNameSpecifierLocBuilder::
485. NestedNameSpecifierLocBuilder(const NestedNameSpecifierLocBuilder &Other) 
486.   : Representation(Other.Representation), Buffer(nullptr),
487.     BufferSize(0), BufferCapacity(0)
488. {
...
500.   Append(Other.Buffer, Other.Buffer + Other.BufferSize, Buffer, BufferSize,
501.                 BufferCapacity);
502. }
The constructor is called on line 484 and it initalizes Buffer to nullptr, BufferSIze to 0 and BufferCapacity to 0 as shown above.
Then, after evaluating all branches in the constructor body to false, it calls Append() with the parameters previously initialized:

441.   void Append(char *Start, char *End, char *&Buffer, unsigned &BufferSize,
442                         unsigned &BufferCapacity) {
443.     if (Start == End)
444.       return;
445.
446.     if (BufferSize + (End - Start) > BufferCapacity) {
...
458.     }
459.
460.     memcpy(Buffer + BufferSize, Start, End - Start);

At this point, after evaluating the branches at lines 443 and 446 to false, the analyzer concludes that 'Buffer + BufferSize' is a "Null pointer passed as an argument to a 'nonnull' parameter", hence the assert I proposed just before this line.

It turns out that this suggested codepath is indeed unlikely because either:
- Buffer parameter is null and so is BufferCapacity when Append() is called, in which case branch at line 446 is unlikely to evaluate to false, or
- Buffer parameter is initialized to a non-null value in the constructor at line 494 and is passed as a non-null parameter when Append() is called.

I chose however to trust the tool and prevent unforseen conditions to lead to that path by asserting Buffer value.

Let me know if you think the warning here should be ignored.


http://reviews.llvm.org/D19084





More information about the cfe-commits mailing list