This is an archive of the discontinued LLVM Phabricator instance.

[scan-build] fix warnings emitted on Clang AST code base
Needs ReviewPublic

Authored by apelete on Apr 13 2016, 4:52 PM.

Details

Summary

This path fixes the following warnings that were reported while
running Clang Static Analyzer on Clang code base:

API: argument with 'nonnull' attribute passed null, on file:

  • lib/AST/NestedNameSpecifier.cpp.

Logic error: called C++ object pointer is null, on files:

  • lib/AST/ASTDiagnostic.cpp,
  • lib/AST/ExprConstant.cpp,
  • lib/AST/DeclObjC.cpp.

Signed-off-by: Apelete Seketeli <apelete@seketeli.net>

Diff Detail

Event Timeline

apelete updated this revision to Diff 53642.Apr 13 2016, 4:52 PM
apelete retitled this revision from to [clang-analyzer] fix warnings emitted on clang code base.
apelete updated this object.
apelete added a subscriber: cfe-commits.

Ping :).
Can someone help this one find its way into he main repo ?

apelete updated this revision to Diff 55746.May 1 2016, 9:08 AM

[clang-analyzer] fix warnings emitted on clang code base

Changes since last revision:

  • fast forward rebase on git master branch.

Waiting for review, could someone please have a look at this one ?

apelete updated this revision to Diff 56248.May 5 2016, 2:21 AM

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

Changes since last revision:

  • split patch into AST changes unit to ease review process.
apelete retitled this revision from [clang-analyzer] fix warnings emitted on clang code base to [scan-build] fix warnings emitted on Clang AST code base .May 5 2016, 2:25 AM
apelete updated this object.
apelete edited reviewers, added: rtrieu, doug.gregor; removed: rjmccall.
dblaikie added inline comments.
lib/AST/ASTDiagnostic.cpp
1686–1688

Should this be a condition, or just an assertion?

lib/AST/ExprConstant.cpp
1992

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.

lib/AST/NestedNameSpecifier.cpp
460

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?)

rtrieu added inline comments.May 5 2016, 2:22 PM
lib/AST/ASTDiagnostic.cpp
1686–1688

This should be an assertion. Same == true implies FromTD and ToTD are not null.

apelete added inline comments.May 12 2016, 10:07 AM
lib/AST/ASTDiagnostic.cpp
1686–1688

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 ?

lib/AST/ExprConstant.cpp
1992

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.

lib/AST/NestedNameSpecifier.cpp
460

Code path proposed by the analyzer is the following:

  1. NestedNameSpecifierLocBuilder::
  2. NestedNameSpecifierLocBuilder(const NestedNameSpecifierLocBuilder &Other)
  3. : Representation(Other.Representation), Buffer(nullptr),
  4. BufferSize(0), BufferCapacity(0)
  5. {

...

  1. Append(Other.Buffer, Other.Buffer + Other.BufferSize, Buffer, BufferSize,
  2. BufferCapacity);
  3. }

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:

  1. void Append(char *Start, char *End, char *&Buffer, unsigned &BufferSize,

442 unsigned &BufferCapacity) {

  1. if (Start == End)
  2. return; 445.
  3. if (BufferSize + (End - Start) > BufferCapacity) {

...

  1. } 459.
  2. 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.

rtrieu added inline comments.May 12 2016, 3:58 PM
lib/AST/ASTDiagnostic.cpp
1686–1688

Given the choice between changing the condition and adding an assertion, the assertion is the correct choice. If Same is true, the first branch must be taken or the wrong message will be printed.

The assertion in this function is (FromTD || ToTD) which allows for FromTD to be null. That means only the case of FromTD is null and TD is null is excluded. This allows the case for for FromTD to be null and ToTD to be non-null, which is case the static analysis is warning about. However, template diffing only sets Same to true when both FromTD and ToTD are non-null, but this isn't checked or enforced anywhere.

To be explicit, this is the kind of assertion and the location it should be present:

if (Same) {
  assert(FromTD && ToTD &&
         "Same implies both template arguments are non-null.");
apelete updated this revision to Diff 57571.May 18 2016, 1:13 AM
apelete retitled this revision from [scan-build] fix warnings emitted on Clang AST code base to [scan-build] fix warnings emitted on Clang AST code base.

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

Changes since last revision:

  • lib/AST/ASTDiagnostic.cpp: replace if() condition by assert to ensure that template diffing print message is done only when both 'FromTD' and 'ToTD' are non-null.