[scan-build] fix warnings emitted on LLVM ARM code base

[apelete]

Summary

Fix "Logic error" warnings of the type "Called C++ object pointer is
null" reported by Clang Static Analyzer on the following files:

• lib/Target/ARM/AsmParser/ARMAsmParser.cpp,
• lib/Target/ARM/Thumb2SizeReduction.cpp.

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

[• tstellarAMD]

lib/Target/AMDGPU/SIInstrInfo.cpp

1867–1870 ↗

(On Diff #54648)

A better fix would be move this if block to the top of the function before the first use of DefinedRC.

[apelete]

lib/Target/AMDGPU/SIInstrInfo.cpp

1867–1870 ↗

(On Diff #54648)

This is what I wanted to do in the first place, but I wondered if doing the check and returning early would still give a correct result; I wasn't sure about the impact it would have had on the logic of the function :-).

Will fix this, thanks for your review.

[apelete]

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

Following changes were done since last revision:

• lib/Target/AMDGPU/SIInstrInfo.cpp: check DefineRC early and return if operand expects an immediate value.

[• tstellarAMD]

lib/Target/AMDGPU/SIInstrInfo.cpp

1839 ↗

(On Diff #54662)

Looking at this again. I think this should return !MO.isReg() and it should be moved below

if (!MO)

MO = &MI->getOperand(OpIdx);

Sorry I missed this the first time.

[apelete]

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

Changes since last revision:

• check TargetRegisterClass variable value after MachineOperand initialization and return based on the kind of MachineOperand.

[apelete]

lib/Target/AMDGPU/SIInstrInfo.cpp

1839 ↗

(On Diff #54867)

Thanks for looking at it again.

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

[• tstellarAMD]

The AMDGPU changes look OK to me. If you want to commit those in a separate patch, that's fine with me.

[apelete]

In D19410#418545, @tstellarAMD wrote:

The AMDGPU changes look OK to me. If you want to commit those in a separate patch, that's fine with me.

Ok, I'm going to split AMDGPU changes into a separate patch, hoping it will help speeding up the landing in trunk.
I'll need your help to check it in for me though :-).

Thanks.

[apelete]

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

Changes since last revision:

• split changes in lib/Target/AMDGPU/SIInstrInfo.cpp into a separate patch.

[apelete]

[scan-build] fix warnings emitted on LLVM ARM code base

Changes since last revision:

• split patch into ARM changes unit to ease review process,
• cherry-pick ARM changes from D19498.

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

1610

Better ask @weimingz about this one.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

9965

We don't want asserts in assembly parsers, people can write pretty much anything. Make this an error.

lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp

409

probably better if you do:

if (A && A-getVariable()) {

lib/Target/ARM/Thumb2SizeReduction.cpp

992

this assert is in the wrong place. It should be just after BundleMI is created.

[compnerd]

lib/Target/ARM/ARMFrameLowering.cpp

1610

Ugh, this is *subtle*. If we are going to simplify this, please add a couple of asserts.

assert(requiresRegisterScavenging() && "ARM requires register scavenging");
assert(RS && "Target requiring register scavenging not provided register scavenger");

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

9965

Yes, adding an error here would be reasonable, however, if we do that, please add a test case that demonstrates the invalid input. I believe that the assertion here is valid as we will always have a section (even if the user inserts it in the wrong location, theres the implicit initial text section to which we will refer).

[rengolin]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

9965

If not having a section here is an assembler error, not an assembly error, than asserting is fine. Users should never see this.

[weimingz]

lib/Target/ARM/ARMFrameLowering.cpp

1610

Agree. We need to simplify the BigStack test. It's hard to read.

It would be better if we refactor it to:

unsigned EstimatedStackSize = MFI->estimateStackSize(MF) + ...

bool BgsTack = EstimatedStackSize >= estimatedRSStackSizeLImit() || ... || ...

[rengolin]

lib/Target/ARM/ARMFrameLowering.cpp

1610

Weiming, can you do this one once you re-commit your patch?

[weimingz]

lib/Target/ARM/ARMFrameLowering.cpp

1610

Sure. I will do.

[weimingz]

lib/Target/ARM/ARMFrameLowering.cpp

1610

Please review http://reviews.llvm.org/D19896

[apelete]

[scan-build] fix warnings emitted on LLVM ARM code base

Changes since last revision:

• lib/Target/ARM/ARMFrameLowering.cpp: removed changes, fix is done in D19896.

[rengolin]

Please, change the two conditions and remove the two asserts I commented on.

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

9965

I agree, this assert is correct.

lib/Target/ARM/Thumb2SizeReduction.cpp

992

turns out I was wrong. This cannot be an assert, as it doesn't have to break when BuidleMI is not defined.

Please change the condition to:

if (BundleMI && !NextInSameBundle && MI->isInsideBundle()) {

[apelete]

[scan-build] fix warnings emitted on LLVM ARM code base

Changes since last revision:

• lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp: remove asert and check against value MCSymbol pointer and existence of variable value within MCSymbol,

• lib/Target/ARM/Thumb2SizeReduction.cpp: remove assert since BundleMI processing should continue if BundleMI is nullptr.

[rengolin]

Thanks Apelete,

Assuming this change passes "make check-all", I'm happy with them.

cheers,
--renato

[apelete]

[scan-build] fix warnings emitted on LLVM ARM code base

Changes since last revision:

• lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp: remove the changes in this file since they break the following tests:
  
  Failing Tests (25):
  • LLVM :: CodeGen/ARM/2011-01-19-MergedGlobalDbg.ll
  • LLVM :: CodeGen/ARM/2011-08-02-MergedGlobalDbg.ll
  • LLVM :: CodeGen/ARM/debug-info-blocks.ll
  • LLVM :: CodeGen/ARM/debug-info-sreg2.ll
  • LLVM :: CodeGen/ARM/local-call.ll
  • LLVM :: CodeGen/ARM/none-macho.ll
  • LLVM :: CodeGen/ARM/thumb1-varalloc.ll
  • LLVM :: CodeGen/Thumb2/constant-islands.ll
  • LLVM :: DebugInfo/ARM/bitfield.ll
  • LLVM :: DebugInfo/ARM/s-super-register.ll
  • LLVM :: DebugInfo/ARM/split-complex.ll
  • LLVM :: LTO/ARM/runtime-library-subtarget.ll
  • LLVM :: MC/ARM/aligned-blx.s
  • LLVM :: MC/ARM/macho-movwt.s
  • LLVM :: MC/ARM/misaligned-blx.s
  • LLVM :: MC/ARM/thumb1-relax-br.s
  • LLVM :: MC/ARM/tls-directives.s
  • LLVM :: MC/MachO/ARM/aliased-symbols.s
  • LLVM :: MC/MachO/ARM/compact-unwind-armv7k.s
  • LLVM :: MC/MachO/ARM/darwin-ARM-reloc.s
  • LLVM :: MC/MachO/ARM/darwin-Thumb-reloc.s
  • LLVM :: MC/MachO/ARM/long-call-branch-island-relocation.s
  • LLVM :: MC/MachO/ARM/static-movt-relocs.s
  • LLVM :: MC/MachO/ARM/thumb-bl-jbits.s
  • LLVM :: MC/MachO/ARM/thumb2-movw-fixup.s

[apelete]

In D19410#425989, @rengolin wrote:

Thanks Apelete,

Assuming this change passes "make check-all", I'm happy with them.

I removed some of the changes that seem to break the unit tests and I'll look into why when I get some time.
In the meantime, if the remaining changes look OK to you please go on and commit for me as I don't have commit access.

[rengolin]

Current changes in as r269285. Thanks!

Please open a new revision for any future changes.

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.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

 ///   ::= .align
 bool ARMAsmParser::parseDirectiveAlign(SMLoc L) {
   // NOTE: if this is not the end of the statement, fall back to the target
   // agnostic handling for this directive which will correctly handle this.
   if (getLexer().isNot(AsmToken::EndOfStatement))
     return true;
 
   // '.align' is target specifically handled to mean 2**2 byte alignment.
-  if (getStreamer().getCurrentSection().first->UseCodeAlign())
+  const MCSection *Section = getStreamer().getCurrentSection().first;
+  assert(Section && "must have section to emit alignment");
+  if (Section->UseCodeAlign())
     getStreamer().EmitCodeAlignment(4, 0);
   else
     getStreamer().EmitValueToAlignment(4, 0, 1, 0);
 
   return false;
 }
 
 /// parseDirectiveThumbSet

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

if (ReduceMI(MBB, MI, LiveCPSR, IsSelfLoop)) {
       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())