This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Fix possibly deref nullptr
ClosedPublic

Authored by XinWang10 on Aug 14 2023, 8:37 PM.

Details

Summary
  1. In X86LowerAMXType.cpp dyn_cast could lead to UserI be nullptr which coud be dref in IRBuilder constructor.
  2. In AsmPrinter.cpp, doInitialization could make MMI be nullptr if MMIWP->getMMI() is false, then the deref after could be unexpected.

Diff Detail

Event Timeline

XinWang10 created this revision.Aug 14 2023, 8:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 8:37 PM
XinWang10 requested review of this revision.Aug 14 2023, 8:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 8:37 PM
skan added a comment.Aug 14 2023, 9:31 PM

Is this a NFC change or a bug fix? If NFC, shouldn't we use assert?

Is this a NFC change or a bug fix? If NFC, shouldn't we use assert?

cast could use assert internally, I think it's same to explicit assert.

skan added a comment.Aug 15 2023, 3:08 AM

Is this a NFC change or a bug fix? If NFC, shouldn't we use assert?

cast could use assert internally, I think it's same to explicit assert.

Then how about others?

Is this a NFC change or a bug fix? If NFC, shouldn't we use assert?

cast could use assert internally, I think it's same to explicit assert.

Then how about others?

OK, the other two places are in same function, line 432 shows

MMI = MMIWP ? &MMIWP->getMMI() : nullptr;

So MMI could be nullptr, I just add add condition for those could deref this MMI, no func change.

skan added a comment.Aug 15 2023, 7:40 AM

Is this a NFC change or a bug fix? If NFC, shouldn't we use assert?

cast could use assert internally, I think it's same to explicit assert.

Then how about others?

OK, the other two places are in same function, line 432 shows

MMI = MMIWP ? &MMIWP->getMMI() : nullptr;

So MMI could be nullptr, I just add add condition for those could deref this MMI, no func change.

No, if MMI could be nullptr, then it's not a NFC change. If we never crash at this point, assert should be used instead.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
578

Why?

Is this a NFC change or a bug fix? If NFC, shouldn't we use assert?

cast could use assert internally, I think it's same to explicit assert.

Then how about others?

OK, the other two places are in same function, line 432 shows

MMI = MMIWP ? &MMIWP->getMMI() : nullptr;

So MMI could be nullptr, I just add add condition for those could deref this MMI, no func change.

No, if MMI could be nullptr, then it's not a NFC change. If we never crash at this point, assert should be used instead.

Then I could remove NFC label.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
578

Constructor would deref MMI

WinException::WinException(AsmPrinter *A) : EHStreamer(A) {
  // MSVC's EH tables are always composed of 32-bit words.  All known 64-bit
  // platforms use an imagerel32 relocation to refer to symbols.
  useImageRel32 = (A->getDataLayout().getPointerSizeInBits() == 64);
...
const DataLayout &AsmPrinter::getDataLayout() const {
  return MMI->getModule()->getDataLayout();
}
XinWang10 retitled this revision from [NFC]Fix possibly deref nullptr to Fix possibly deref nullptr.Aug 15 2023, 6:40 PM
XinWang10 updated this revision to Diff 550582.Aug 15 2023, 8:20 PM
  • use assert instead
skan added inline comments.Aug 15 2023, 8:39 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
578

Move this to AsmPrinter::getDataLayout()

XinWang10 updated this revision to Diff 550586.Aug 15 2023, 8:42 PM
  • mv to datalayout
skan accepted this revision.Aug 15 2023, 9:25 PM

LGTM

This revision is now accepted and ready to land.Aug 15 2023, 9:25 PM
skan retitled this revision from Fix possibly deref nullptr to [NFC] Fix possibly deref nullptr.Aug 15 2023, 9:26 PM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
386

MMI-> expresses the intention that MMI cannot be nullptr. This assert has very little value. I think we generally don't add it.

XinWang10 marked an inline comment as done.Aug 16 2023, 11:58 PM
XinWang10 added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
386

OK, I may mov this to higher level to avoid static analyzer report in another patch.

MaskRay added inline comments.Aug 17 2023, 1:08 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
386

Please just don't send patches just to appease static analyzer. Their reports must be analyzed. Low value reports can be turned down/ignored.