[NFC] Fix possibly deref nullptr

[XinWang10]

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.

[skan]

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

[XinWang10]

In D157948#4587289, @skan wrote:

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]

In D157948#4587327, @XinWang10 wrote:

In D157948#4587289, @skan wrote:

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?

[XinWang10]

In D157948#4587733, @skan wrote:

In D157948#4587327, @XinWang10 wrote:

In D157948#4587289, @skan wrote:

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]

In D157948#4588131, @XinWang10 wrote:

In D157948#4587733, @skan wrote:

In D157948#4587327, @XinWang10 wrote:

In D157948#4587289, @skan wrote:

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?

[XinWang10]

In D157948#4588502, @skan wrote:

In D157948#4588131, @XinWang10 wrote:

In D157948#4587733, @skan wrote:

In D157948#4587327, @XinWang10 wrote:

In D157948#4587289, @skan wrote:

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]

• use assert instead

[skan]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

578

Move this to AsmPrinter::getDataLayout()

[XinWang10]

• mv to datalayout

[skan]

LGTM

[MaskRay]

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]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

386

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

[MaskRay]

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

unsigned AsmPrinter::getFunctionNumber() const {
   return MF->getFunctionNumber();
 }
 
 const TargetLoweringObjectFile &AsmPrinter::getObjFileLowering() const {
   return *TM.getObjFileLowering();
 }
 
 const DataLayout &AsmPrinter::getDataLayout() const {
+  assert(MMI && "MMI could not be nullptr!");
   return MMI->getModule()->getDataLayout();
 }
 
 // Do not use the cached DataLayout because some client use it without a Module
 // (dsymutil, llvm-dwarfdump).
 unsigned AsmPrinter::getPointerSize() const {
   return TM.getPointerSize(0); // FIXME: Default address space
 }
if (MAI->doesSupportDebugInformation()) {
     bool EmitCodeView = M.getCodeViewFlag();
     if (EmitCodeView && TM.getTargetTriple().isOSWindows()) {
       Handlers.emplace_back(std::make_unique<CodeViewDebug>(this),
                             DbgTimerName, DbgTimerDescription,
                             CodeViewLineTablesGroupName,
                             CodeViewLineTablesGroupDescription);
     }
     if (!EmitCodeView || M.getDwarfVersion()) {
+      assert(MMI && "MMI could not be nullptr here!");
       if (MMI->hasDebugInfo()) {
         DD = new DwarfDebug(this);
         Handlers.emplace_back(std::unique_ptr<DwarfDebug>(DD), DbgTimerName,
                               DbgTimerDescription, DWARFGroupName,
                               DWARFGroupDescription);
       }
     }
   }
case ExceptionHandling::ARM:
     break;
   case ExceptionHandling::WinEH:
     switch (MAI->getWinEHEncodingType()) {
     default: llvm_unreachable("unsupported unwinding information encoding");
     case WinEH::EncodingType::Invalid:
       break;
     case WinEH::EncodingType::X86:
     case WinEH::EncodingType::Itanium:
       ES = new WinException(this);
       break;
     }
     break;
   case ExceptionHandling::Wasm:
     ES = new WasmException(this);
     break;
   case ExceptionHandling::AIX:
     ES = new AIXException(this);

llvm/lib/Target/X86/X86LowerAMXType.cpp

if (IsPHI) {
     Value *PhiOp = cast<PHINode>(V)->getIncomingValue(0);
     II = cast<IntrinsicInst>(PhiOp);
   } else {
     II = cast<IntrinsicInst>(V);
   }
   Value *Row = II->getOperand(0);
   Value *Col = II->getOperand(1);
 
-  Instruction *UserI = dyn_cast<Instruction>(U.getUser());
+  Instruction *UserI = cast<Instruction>(U.getUser());
   IRBuilder<> Builder(UserI);
   Value *Stride = Builder.getInt64(64);
   std::array<Value *, 4> Args = {Row, Col, Ptr, Stride};
 
   Value *TileLoad = Builder.CreateIntrinsic(Intrinsic::x86_tileloadd64_internal,
                                             std::nullopt, Args);
   UserI->replaceUsesOfWith(V, TileLoad);
 }