This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by apelete on Apr 22 2016, 7:45 AM.

Details

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 <apelete@seketeli.net>

Diff Detail

Event Timeline

apelete updated this revision to Diff 54648.Apr 22 2016, 7:45 AM
apelete retitled this revision from to [scan-build] fix logic error warnings emitted on llvm code base.
apelete updated this object.
apelete added a subscriber: llvm-commits.
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 added inline comments.Apr 22 2016, 8:21 AM
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 updated this revision to Diff 54662.Apr 22 2016, 8:50 AM

[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.
apelete marked 2 inline comments as done.Apr 22 2016, 8:52 AM
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 updated this revision to Diff 54867.Apr 25 2016, 9:24 AM

[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 marked an inline comment as done.Apr 25 2016, 9:29 AM
apelete added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
1839 ↗(On Diff #54867)

Thanks for looking at it again.

apelete updated this revision to Diff 55750.May 1 2016, 9:34 AM
apelete marked an inline comment as done.

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

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

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 updated this revision to Diff 55834.May 2 2016, 9:35 AM
apelete edited edge metadata.

[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 updated this revision to Diff 56264.May 5 2016, 4:36 AM

[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.
apelete retitled this revision from [scan-build] fix logic error warnings emitted on llvm code base to [scan-build] fix warnings emitted on LLVM ARM code base.May 5 2016, 4:38 AM
apelete updated this object.
apelete edited reviewers, added: compnerd, grosbach, MatzeB; removed: tstellarAMD, atrick, pcc.
rengolin added inline comments.
lib/Target/ARM/ARMFrameLowering.cpp
1610 ↗(On Diff #56264)

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 added inline comments.May 5 2016, 9:02 AM
lib/Target/ARM/ARMFrameLowering.cpp
1610 ↗(On Diff #56264)

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 added inline comments.May 5 2016, 9:11 AM
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 added inline comments.May 5 2016, 10:52 AM
lib/Target/ARM/ARMFrameLowering.cpp
1610 ↗(On Diff #56264)

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 added inline comments.May 6 2016, 7:20 AM
lib/Target/ARM/ARMFrameLowering.cpp
1610 ↗(On Diff #56264)

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

weimingz added inline comments.May 6 2016, 9:52 AM
lib/Target/ARM/ARMFrameLowering.cpp
1610 ↗(On Diff #56264)

Sure. I will do.

weimingz added inline comments.May 6 2016, 2:04 PM
lib/Target/ARM/ARMFrameLowering.cpp
1610 ↗(On Diff #56264)
apelete updated this revision to Diff 56543.May 9 2016, 3:19 AM

[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 edited edge metadata.May 9 2016, 3:32 AM

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 updated this revision to Diff 56549.May 9 2016, 4:12 AM
apelete edited edge metadata.

[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.
apelete updated this object.May 9 2016, 4:14 AM
rengolin accepted this revision.May 10 2016, 10:30 AM
rengolin edited edge metadata.

Thanks Apelete,

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

cheers,
--renato

This revision is now accepted and ready to land.May 10 2016, 10:30 AM
apelete updated this revision to Diff 56857.EditedMay 11 2016, 1:05 AM
apelete edited edge metadata.

[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

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.

apelete updated this object.May 11 2016, 1:11 AM
rengolin closed this revision.May 12 2016, 5:40 AM

Current changes in as r269285. Thanks!

Please open a new revision for any future changes.