[scan-build] fix logic error warnings emitted on llvm code base

[apelete]

Summary

This patch fixes logic error warnings of the type "called C++ object
pointer is null" that were emitted by Clang Static Analyzer on the
following files:

• lib/Transforms/Scalar/TailRecursionElimination.cpp,
• lib/Transforms/Scalar/StructurizeCFG.cpp,
• lib/Transforms/Scalar/ScalarReplAggregates.cpp,
• lib/Target/X86/AsmParser/X86AsmParser.cpp,
• lib/Target/Hexagon/RDFGraph.cpp,
• lib/Target/Hexagon/HexagonCFGOptimizer.cpp,
• lib/Target/ARM/Thumb2SizeReduction.cpp,
• lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp,
• lib/Target/ARM/ARMFrameLowering.cpp,
• lib/CodeGen/ScheduleDAGInstrs.cpp,
• lib/Bitcode/Writer/BitcodeWriter.cpp.

Signed-off-by: Apelete Seketeli <[e-mail]>

[kparzysz]

The Hexagon part looks good to me.

[MatzeB]

I looked at the ScheduleDAG and ARM changes here.

lib/CodeGen/ScheduleDAGInstrs.cpp

917–918

I think this is so obvious that we don't even need the assert. We will get a crash in MI->isDebugValue() anyway when MI is nullptr.

lib/Target/ARM/ARMFrameLowering.cpp

1710

ARMBaseRegisterInfo::requiresRegisterScavenging() always returns true. So this should never be nullptr, instead of adding the assert you can probably remove the nullptr-check in line 1610.

lib/Target/ARM/Thumb2SizeReduction.cpp

976–977

This looks like it needs to go inside the "if (MI->isInsideBundle())" case below. I must admit I also don't understand why the analyzer would warn here, there is nothing obvious that indicates that BundleMI may be a nullptr below...

[apelete]

[scan-build] fix logic error warnings emitted on llvm code base

Changes since last revision:

• lib/CodeGen/ScheduleDAGInstrs.cpp: remove assert and nullptr check on MachineInstr pointer to suppress analyzer warning: dereferencing MI pointer will induce a crash if it is null anyway,

• lib/Target/ARM/ARMFrameLowering.cpp: remove assert and nullptr check on RegScavenger pointer since ARMBaseRegisterInfo::requiresRegisterScavenging() always returns true,

• lib/Target/ARM/Thumb2SizeReduction.cpp: move assert closer to corresponding code block.

lib/CodeGen/ScheduleDAGInstrs.cpp

917–918

In that case, not checking MI as I did below should be enough to suppress the analyzer warning.
Would that be a good solution (since MI->isDebugValue() will induce a crash if MI is nullptr anyway) ?

lib/Target/ARM/Thumb2SizeReduction.cpp

976–977

Will move the assert inside the "if (MI->isInsideBundle())" case.

It seems the analyzer is warning because of:

942. MachineInstr *BundleMI = nullptr;

969. if (MI->isBundle()) {
970. BundleMI = MI;
971. continue;
972. }

If the condition is false, then BundleMI is nullptr at this point.

[apelete]

[scan-build] fix logic error warnings emitted on llvm 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]

This patch was split into D19083, D19410, D19966, D19968, D19973 and D19975 to ease review process.

lib/Bitcode/Writer/BitcodeWriter.cpp

if (IsModuleLevel && hasVSTOffsetPlaceholder()) {
     assert((VSTOffset & 31) == 0 && "VST block not 32-bit aligned");
     Stream.BackpatchWord(VSTOffsetPlaceholder, VSTOffset / 32);
   }
 
   Stream.EnterSubblock(bitc::VALUE_SYMTAB_BLOCK_ID, 4);
 
   // For the module-level VST, add abbrev Ids for the VST_CODE_FNENTRY
   // records, which are not used in the per-function VSTs.
-  unsigned FnEntry8BitAbbrev;
-  unsigned FnEntry7BitAbbrev;
-  unsigned FnEntry6BitAbbrev;
+  unsigned FnEntry8BitAbbrev = VST_ENTRY_8_ABBREV;
+  unsigned FnEntry7BitAbbrev = VST_ENTRY_7_ABBREV;
+  unsigned FnEntry6BitAbbrev = VST_ENTRY_6_ABBREV;
   if (IsModuleLevel && hasVSTOffsetPlaceholder()) {
     // 8-bit fixed-width VST_CODE_FNENTRY function strings.
     BitCodeAbbrev *Abbv = new BitCodeAbbrev();
     Abbv->Add(BitCodeAbbrevOp(bitc::VST_CODE_FNENTRY));
     Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8)); // value id
     Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8)); // funcoffset
     Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
     Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 8));

lib/CodeGen/ScheduleDAGInstrs.cpp

void ScheduleDAGInstrs::buildSchedGraph(AliasAnalysis *AA,
   // ExitSU.
   addSchedBarrierDeps();
 
   // Walk the list of instructions, from bottom moving up.
   MachineInstr *DbgMI = nullptr;
   for (MachineBasicBlock::iterator MII = RegionEnd, MIE = RegionBegin;
        MII != MIE; --MII) {
     MachineInstr *MI = std::prev(MII);
-    if (MI && DbgMI) {
+
+    if (DbgMI) {
       DbgValues.push_back(std::make_pair(DbgMI, MI));
       DbgMI = nullptr;
     }
 
     if (MI->isDebugValue()) {
       DbgMI = MI;
       continue;
     }

lib/Target/ARM/ARMFrameLowering.cpp

void ARMFrameLowering::determineCalleeSaves(MachineFunction &MF,
   // our instructions are positive-offset only, so conservatively consider
   // that case to want a spill slot (or register) as well. Similarly, if
   // the function adjusts the stack pointer during execution and the
   // adjustments aren't already part of our stack size estimate, our offset
   // calculations may be off, so be conservative.
   // FIXME: We could add logic to be more precise about negative offsets
   //        and which instructions will need a scratch register for them. Is it
   //        worth the effort and added fragility?
-  bool BigStack = (RS && (MFI->estimateStackSize(MF) +
-                              ((hasFP(MF) && AFI->hasStackFrame()) ? 4 : 0) >=
-                          estimateRSStackSizeLimit(MF, this))) ||
+  bool BigStack = ((MFI->estimateStackSize(MF) +
                   MFI->hasVarSizedObjects() ||
                   (MFI->adjustsStack() && !canSimplifyCallFramePseudos(MF));
 
   bool ExtraCSSpill = false;
   if (BigStack || !CanEliminateFrame || RegInfo->cannotEliminateFrame(MF)) {
     AFI->setHasStackFrame(true);
 
     // If LR is not spilled, but at least one of R4, R5, R6, and R7 is spilled.
if (BigStack && !ExtraCSSpill) {
       if (Extras.size() && NumExtras == 0) {
         for (unsigned i = 0, e = Extras.size(); i != e; ++i) {
           SavedRegs.set(Extras[i]);
         }
       } else if (!AFI->isThumb1OnlyFunction()) {
         // note: Thumb1 functions spill to R12, not the stack.  Reserve a slot
         // closest to SP or frame pointer.
         const TargetRegisterClass *RC = &ARM::GPRRegClass;
         RS->addScavengingFrameIndex(MFI->CreateStackObject(RC->getSize(),
                                                            RC->getAlignment(),
                                                            false));
       }
     }
   }
 
   if (ForceLRSpill) {
     SavedRegs.set(ARM::LR);

lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp

void ARMMachObjectWriter::recordRelocation(MachObjectWriter *Writer,
   unsigned Type = 0;
   const MCSymbol *RelSymbol = nullptr;
 
   if (Target.isAbsolute()) { // constant
     // FIXME!
     report_fatal_error("FIXME: relocations to absolute targets "
                        "not yet implemented");
   } else {
+    assert(A && "symbol is needed to resolve constant variables");
     // Resolve constant variables.
     if (A->isVariable()) {
       int64_t Res;
       if (A->getVariableValue()->evaluateAsAbsolute(
               Res, Layout, Writer->getSectionAddressMap())) {
         FixedValue = Res;
         return;
       }

lib/Target/ARM/Thumb2SizeReduction.cpp

for (; MII != E; MII = NextMII) {
     MachineInstr *MI = &*MII;
     if (MI->isBundle()) {
       BundleMI = MI;
       continue;
     }
     if (MI->isDebugValue())
       continue;
 
     LiveCPSR = UpdateCPSRUse(*MI, LiveCPSR);
 
     // Does NextMII belong to the same bundle as MI?
     bool NextInSameBundle = NextMII != E && NextMII->isBundledWithPred();
 
     if (ReduceMI(MBB, MI, LiveCPSR, IsSelfLoop)) {
       Modified = true;
       MachineBasicBlock::instr_iterator I = std::prev(NextMII);
       MI = &*I;
       // Removing and reinserting the first instruction in a bundle will break
       // up the bundle. Fix the bundling if it was broken.
       if (NextInSameBundle && !NextMII->isBundledWithPred())
         NextMII->bundleWithPred();
     }
 
     if (!NextInSameBundle && MI->isInsideBundle()) {
+      assert(BundleMI && "MachineInstr is null, abort reducing width of instructions");
       // FIXME: Since post-ra scheduler operates on bundles, the CPSR kill
       // marker is only on the BUNDLE instruction. Process the BUNDLE
       // instruction as we finish with the bundled instruction to work around
       // the inconsistency.
       if (BundleMI->killsRegister(ARM::CPSR))
         LiveCPSR = false;
       MachineOperand *MO = BundleMI->findRegisterDefOperand(ARM::CPSR);
       if (MO && !MO->isDead())

lib/Target/Hexagon/HexagonCFGOptimizer.cpp

if (MII != MBB->end()) {
           continue;
         }
 
         if ((NumSuccs == 2) && LayoutSucc && (LayoutSucc->pred_size() == 1)) {
 
           // Ensure that BB2 has one instruction -- an unconditional jump.
           if ((LayoutSucc->size() == 1) &&
               IsUnconditionalJump(LayoutSucc->front().getOpcode())) {
+            assert(JumpAroundTarget && "jump target is needed to process second basic block");
             MachineBasicBlock* UncondTarget =
               LayoutSucc->front().getOperand(0).getMBB();
             // Check if the layout successor of BB2 is BB3.
             bool case1 = LayoutSucc->isLayoutSuccessor(JumpAroundTarget);
             bool case2 = JumpAroundTarget->isSuccessor(UncondTarget) &&
               JumpAroundTarget->size() >= 1 &&
               IsUnconditionalJump(JumpAroundTarget->back().getOpcode()) &&
               JumpAroundTarget->pred_size() == 1 &&

lib/Target/Hexagon/RDFGraph.cpp

 }
 
 // Create data-flow links for all instructions in the block node BA. This
 // will include updating any phi nodes in BA.
 void DataFlowGraph::linkBlockRefs(DefStackMap &DefM, NodeAddr<BlockNode*> BA) {
   // Push block delimiters.
   markBlock(BA.Id, DefM);
 
+  assert(BA.Addr && "block node address is needed to create a data-flow link");
   // For each non-phi instruction in the block, link all the defs and uses
   // to their reaching defs. For any member of the block (including phis),
   // push the defs on the corresponding stacks.
   for (NodeAddr<InstrNode*> IA : BA.Addr->members(*this)) {
     // Ignore phi nodes here. They will be linked part by part from the
     // predecessors.
     if (IA.Addr->getKind() == NodeAttrs::Stmt)
       linkStmtRefs(DefM, IA);

lib/Target/X86/AsmParser/X86AsmParser.cpp

bool X86AsmParser::parseDirectiveEven(SMLoc L) {
   if (getLexer().isNot(AsmToken::EndOfStatement)) {
     TokError("unexpected token in directive");
     return false;  
   }
   if (!Section) {
     getStreamer().InitSections(false);
     Section = getStreamer().getCurrentSection().first;
   }
+  assert(Section && "must have section to emit alignment");
   if (Section->UseCodeAlign())
     getStreamer().EmitCodeAlignment(2, 0);
   else
     getStreamer().EmitValueToAlignment(2, 0, 1, 0);
   return false;
 }
 /// ParseDirectiveWord
 ///  ::= .word [ expression (, expression)* ]

lib/Transforms/Scalar/ScalarReplAggregates.cpp

for (Value::use_iterator UI = I->use_begin(), E = I->use_end(); UI!=E;) {
     }
   }
 }
 
 /// RewriteBitCast - Update a bitcast reference to the alloca being replaced
 /// and recursively continue updating all of its uses.
 void SROA::RewriteBitCast(BitCastInst *BC, AllocaInst *AI, uint64_t Offset,
                           SmallVectorImpl<AllocaInst *> &NewElts) {
+  assert((BC && AI) && "BitCast and Alloca cannot be null" );
   RewriteForScalarRepl(BC, AI, Offset, NewElts);
   if (BC->getOperand(0) != AI)
     return;
 
   // The bitcast references the original alloca.  Replace its uses with
   // references to the alloca containing offset zero (which is normally at
   // index zero, but might not be in cases involving structs with elements
   // of size zero).

lib/Transforms/Scalar/StructurizeCFG.cpp

BasicBlock *Flow = BasicBlock::Create(Context, FlowBlockName,
                                         Func, Insert);
   DT->addNewBlock(Flow, Dominator);
   ParentRegion->getRegionInfo()->setRegionFor(Flow, ParentRegion);
   return Flow;
 }
 
 /// \brief Create a new or reuse the previous node as flow node
 BasicBlock *StructurizeCFG::needPrefix(bool NeedEmpty) {
+  assert(PrevNode && "cannot reuse null PrevNode as flow node");
+
   BasicBlock *Entry = PrevNode->getEntry();
 
   if (!PrevNode->isSubRegion()) {
     killTerminator(Entry);
     if (!NeedEmpty || Entry->getFirstInsertionPt() == Entry->end())
       return Entry;
 
   }

lib/Transforms/Scalar/TailRecursionElimination.cpp

while (true) {
     --BBI;
   }
 
   // If this call is marked as a tail call, and if there are dynamic allocas in
   // the function, we cannot perform this optimization.
   if (CI->isTailCall() && CannotTailCallElimCallsMarkedTail)
     return nullptr;
 
+  assert(F && "BasicBlock parent did not have an enclosing method");
   // As a special case, detect code like this:
   //   double fabs(double f) { return __builtin_fabs(f); } // a 'fabs' call
   // and disable this xform in this case, because the code generator will
   // lower the call to fabs into inline code.
   if (BB == &F->getEntryBlock() &&
       FirstNonDbg(BB->front().getIterator()) == CI &&
       FirstNonDbg(std::next(BB->begin())) == TI && CI->getCalledFunction() &&
       !TTI->isLoweredToCall(CI->getCalledFunction())) {