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

[apelete]

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 <[e-mail]>

[apelete]

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

[apelete]

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

Changes since last revision:

• fast forward rebase on git master branch.

[apelete]

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

[apelete]

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

Changes since last revision:

• split patch into AST changes unit to ease review process.

[dblaikie]

lib/AST/ASTDiagnostic.cpp

1686

Should this be a condition, or just an assertion?

lib/AST/ExprConstant.cpp

1992 ↗

(On Diff #56248)

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]

lib/AST/ASTDiagnostic.cpp

1686

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

[apelete]

lib/AST/ASTDiagnostic.cpp

1686

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 ↗

(On Diff #56248)

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:

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.
445. if (BufferSize + (End - Start) > BufferCapacity) {

...

458. } 459.
459. 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]

lib/AST/ASTDiagnostic.cpp

1686

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]

[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.

lib/AST/ASTDiagnostic.cpp

void PrintTemplateTemplate(TemplateDecl *FromTD, TemplateDecl *ToTD,
 
     std::string FromName = FromTD ? FromTD->getName() : "(no argument)";
     std::string ToName = ToTD ? ToTD->getName() : "(no argument)";
     if (FromTD && ToTD && FromName == ToName) {
       FromName = FromTD->getQualifiedNameAsString();
       ToName = ToTD->getQualifiedNameAsString();
     }
 
-    if (Same) {
+    if (Same && FromTD) {
       OS << "template " << FromTD->getNameAsString();
     } else if (!PrintTree) {
       OS << (FromDefault ? "(default) template " : "template ");
       Bold();
       OS << FromName;
       Unbold();
     } else {
       OS << (FromDefault ? "[(default) template " : "[template ");

lib/AST/NestedNameSpecifier.cpp

if (BufferSize + (End - Start) > BufferCapacity) {
       char *NewBuffer = static_cast<char *>(malloc(NewCapacity));
       if (BufferCapacity) {
         memcpy(NewBuffer, Buffer, BufferSize);
         free(Buffer);
       }
       Buffer = NewBuffer;
       BufferCapacity = NewCapacity;
     }
-    
+    assert(Buffer && "Buffer cannot be NULL");
+
     memcpy(Buffer + BufferSize, Start, End - Start);
     BufferSize += End-Start;
   }
   
   /// \brief Save a source location to the given buffer.
   void SaveSourceLocation(SourceLocation Loc, char *&Buffer,
                           unsigned &BufferSize, unsigned &BufferCapacity) {
     unsigned Raw = Loc.getRawEncoding();

lib/Driver/Tools.cpp

success =
         getAArch64MicroArchFeaturesFromMtune(D, A->getValue(), Args, Features);
   else if (success && (A = Args.getLastArg(options::OPT_mcpu_EQ)))
     success =
         getAArch64MicroArchFeaturesFromMcpu(D, A->getValue(), Args, Features);
   else if (Args.hasArg(options::OPT_arch))
     success = getAArch64MicroArchFeaturesFromMcpu(D, getAArch64TargetCPU(Args),
                                                   Args, Features);
 
-  if (!success)
+  if (!success && A)
     D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 
   if (Args.getLastArg(options::OPT_mgeneral_regs_only)) {
     Features.push_back("-fp-armv8");
     Features.push_back("-crypto");
     Features.push_back("-neon");
   }
 

lib/Sema/SemaInit.cpp

static bool hasCompatibleArrayTypes(ASTContext &Context, const ArrayType *Dest,
   // incomplete array type and the source is a constant array type.
   return Source->isConstantArrayType() && Dest->isIncompleteArrayType();
 }
 
 static bool tryObjCWritebackConversion(Sema &S,
                                        InitializationSequence &Sequence,
                                        const InitializedEntity &Entity,
                                        Expr *Initializer) {
+  assert(Initializer && "Initializer needs to be not NULL");
+
   bool ArrayDecay = false;
   QualType ArgType = Initializer->getType();
   QualType ArgPointee;
   if (const ArrayType *ArgArrayType = S.Context.getAsArrayType(ArgType)) {
     ArrayDecay = true;
     ArgPointee = ArgArrayType->getElementType();
     ArgType = S.Context.getPointerType(ArgPointee);
   }
if (ICS.Standard.First == ICK_Array_To_Pointer ||
       AddConversionSequenceStep(LvalueICS, ICS.Standard.getToType(0));
     }
     
     AddPassByIndirectCopyRestoreStep(DestType, ShouldCopy);
   } else if (ICS.isBad()) {
     DeclAccessPair dap;
     if (isLibstdcxxPointerReturnFalseHack(S, Entity, Initializer)) {
       AddZeroInitializationStep(Entity.getType());
-    } else if (Initializer->getType() == Context.OverloadTy &&
+    } else if (Initializer && Initializer->getType() == Context.OverloadTy &&
                !S.ResolveAddressOfOverloadedFunction(Initializer, DestType,
                                                      false, dap))
       SetFailed(InitializationSequence::FK_AddressOfOverloadFailed);
-    else if (Initializer->getType()->isFunctionType() &&
+    else if (Initializer && Initializer->getType()->isFunctionType() &&
              isExprAnUnaddressableFunction(S, Initializer))
       SetFailed(InitializationSequence::FK_AddressOfUnaddressableFunction);
     else
       SetFailed(InitializationSequence::FK_ConversionFailed);
   } else {
     AddConversionSequenceStep(ICS, DestType, TopLevelOfInitList);
 
     MaybeProduceObjCObject(S, *this, Entity);