This is an archive of the discontinued LLVM Phabricator instance.

Differential D19410

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

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

Details

Reviewers
rengolin
grosbach
compnerd
MatzeB
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 added reviewers: rengolin, • tstellarAMD, pcc, atrick.
apelete updated this object.
apelete added a subscriber: llvm-commits.
Herald added subscribers: mzolotukhin, arsenm, aemerson. · View Herald TranscriptApr 22 2016, 7:45 AM
• tstellarAMD added inline comments.Apr 22 2016, 7:54 AM
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
• tstellarAMD added inline comments.Apr 25 2016, 7:07 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.
apelete added a comment.May 1 2016, 10:06 AM

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

• tstellarAMD edited edge metadata.May 2 2016, 7:16 AM

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

apelete added a comment.May 2 2016, 9:29 AM
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 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 a subscriber: weimingz.May 5 2016, 4:48 AM
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() || ... || ...

apelete mentioned this in D19498: [scan-build] fix logic error warnings emitted on llvm code base.May 5 2016, 4:07 PM
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)

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

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.
Herald added a subscriber: rengolin. · View Herald TranscriptMay 9 2016, 3:19 AM
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
apelete added a comment.May 11 2016, 1:10 AM
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.

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.

Revision Contents

PathSize
lib/
Target/
ARM/
AsmParser/
4 lines
MCTargetDesc/
1 line
1 line

Diff 56543

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 9,955 Lines • ▼ Show 20 Lines
/// ::= .align /// ::= .align
bool ARMAsmParser::parseDirectiveAlign(SMLoc L) { bool ARMAsmParser::parseDirectiveAlign(SMLoc L) {
// NOTE: if this is not the end of the statement, fall back to the target // 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. // agnostic handling for this directive which will correctly handle this.
if (getLexer().isNot(AsmToken::EndOfStatement)) if (getLexer().isNot(AsmToken::EndOfStatement))
return true; return true;
// '.align' is target specifically handled to mean 2**2 byte alignment. // '.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");
rengolinUnsubmitted
Not Done
ReplyInline Actions

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

rengolin: We don't want asserts in assembly parsers, people can write pretty much anything. Make this an…
compnerdUnsubmitted
Not Done
ReplyInline Actions

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

compnerd: Yes, adding an error here would be reasonable, however, if we do that, please add a test case…
rengolinUnsubmitted
Not Done
ReplyInline Actions

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

rengolin: If not having a section here is an assembler error, not an assembly error, than asserting is…
rengolinUnsubmitted
Not Done
ReplyInline Actions

I agree, this assert is correct.

rengolin: I agree, this assert is correct.
if (Section->UseCodeAlign())
getStreamer().EmitCodeAlignment(4, 0); getStreamer().EmitCodeAlignment(4, 0);
else else
getStreamer().EmitValueToAlignment(4, 0, 1, 0); getStreamer().EmitValueToAlignment(4, 0, 1, 0);
return false; return false;
} }
/// parseDirectiveThumbSet /// parseDirectiveThumbSet
▲ Show 20 Lines • Show All 160 Lines • Show Last 20 Lines

lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp

Show First 20 Lines • Show All 400 Lines • ▼ Show 20 Linesvoid ARMMachObjectWriter::recordRelocation(MachObjectWriter *Writer,
unsigned Type = 0; unsigned Type = 0;
const MCSymbol *RelSymbol = nullptr; const MCSymbol *RelSymbol = nullptr;
if (Target.isAbsolute()) { // constant if (Target.isAbsolute()) { // constant
// FIXME! // FIXME!
report_fatal_error("FIXME: relocations to absolute targets " report_fatal_error("FIXME: relocations to absolute targets "
"not yet implemented"); "not yet implemented");
} else { } else {
assert(A && "symbol is needed to resolve constant variables");
rengolinUnsubmitted
Not Done
ReplyInline Actions

probably better if you do:

if (A && A-getVariable()) {
rengolin: probably better if you do: if (A && A-getVariable()) {
// Resolve constant variables. // Resolve constant variables.
if (A->isVariable()) { if (A->isVariable()) {
int64_t Res; int64_t Res;
if (A->getVariableValue()->evaluateAsAbsolute( if (A->getVariableValue()->evaluateAsAbsolute(
Res, Layout, Writer->getSectionAddressMap())) { Res, Layout, Writer->getSectionAddressMap())) {
FixedValue = Res; FixedValue = Res;
return; return;
} }
▲ Show 20 Lines • Show All 70 Lines • Show Last 20 Lines

lib/Target/ARM/Thumb2SizeReduction.cpp

Show First 20 Lines • Show All 983 Lines • ▼ Show 20 Lines if (ReduceMI(MBB, MI, LiveCPSR, IsSelfLoop)) {
MI = &*I; MI = &*I;
// Removing and reinserting the first instruction in a bundle will break // Removing and reinserting the first instruction in a bundle will break
// up the bundle. Fix the bundling if it was broken. // up the bundle. Fix the bundling if it was broken.
if (NextInSameBundle && !NextMII->isBundledWithPred()) if (NextInSameBundle && !NextMII->isBundledWithPred())
NextMII->bundleWithPred(); NextMII->bundleWithPred();
} }
if (!NextInSameBundle && MI->isInsideBundle()) { if (!NextInSameBundle && MI->isInsideBundle()) {
assert(BundleMI && "MachineInstr is null, abort reducing width of instructions");
rengolinUnsubmitted
Not Done
ReplyInline Actions

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

rengolin: this assert is in the wrong place. It should be just after BundleMI is created.
rengolinUnsubmitted
Not Done
ReplyInline Actions

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()) {
rengolin: turns out I was wrong. This cannot be an assert, as it doesn't have to break when BuidleMI is…
// FIXME: Since post-ra scheduler operates on bundles, the CPSR kill // FIXME: Since post-ra scheduler operates on bundles, the CPSR kill
// marker is only on the BUNDLE instruction. Process the BUNDLE // marker is only on the BUNDLE instruction. Process the BUNDLE
// instruction as we finish with the bundled instruction to work around // instruction as we finish with the bundled instruction to work around
// the inconsistency. // the inconsistency.
if (BundleMI->killsRegister(ARM::CPSR)) if (BundleMI->killsRegister(ARM::CPSR))
LiveCPSR = false; LiveCPSR = false;
MachineOperand *MO = BundleMI->findRegisterDefOperand(ARM::CPSR); MachineOperand *MO = BundleMI->findRegisterDefOperand(ARM::CPSR);
if (MO && !MO->isDead()) if (MO && !MO->isDead())
▲ Show 20 Lines • Show All 60 Lines • Show Last 20 Lines