[AsmWritter] Fixed "null check after dereferencing" warning

[xbolva00]

Summary

The 'BB->getParent()' pointer was utilized before it was verified against nullptr. Check lines: 3567, 3581.

[xbolva00]

@RKSimon , since we have quite a lot "null check after dereferencing" warnings in LLVM, is this "pattern" ok? I mean, new assert before dereferencing the pointer..

[RKSimon]

In D69751#1731246, @xbolva00 wrote:

@RKSimon , since we have quite a lot "null check after dereferencing" warnings in LLVM, is this "pattern" ok? I mean, new assert before dereferencing the pointer..

I'm not sure what patterns you have in mind? In this case it looks like we just need to cleanup the logic (and assert that we don't have orphan BBs), other cases its more about ensuring common code isn't prematurely pulled out (which you might be referring to with IsEntryBlock?).

The patch makes sense to me but @jyknight should confirm since he touched this code most recently.

llvm/lib/IR/AsmWriter.cpp

3567

Maybe BB && BB->getParent() ?

3583

http://lab.llvm.org:8080/coverage/coverage-reports/llvm/coverage indicates this never gets called so the assert looks alright to me.

[xbolva00]

llvm/lib/IR/AsmWriter.cpp

3583

Link does not work for me.

You don't have permission to access /coverage/coverage-reports/llvm/coverage/ on this server.

[xbolva00]

Addressed review comment.

[xbolva00]

llvm/lib/IR/AsmWriter.cpp

3583

http://lab.llvm.org:8080/coverage/coverage-reports/llvm/

this works

[xbolva00]

Ping, this looks kinda easy to review...

[jyknight]

Oops, it was my fault that I put an access before the check.

I think it'd be better to keep the ability to print it if for some reason there's no parent. It should never be required in normal circumstances, but this function can also be used while debugging, where that would be convenient.

[xbolva00]

I am not sure...

3572 - You will dereference BB->getParent() here anyway.

[xbolva00]

It should never be required in normal circumstances, but this function can also be used while debugging, where that would be convenient.

Not sure how it can be used currently, maybe with a big luck, since there is null pointer dereferencing.

I am gonna commit this.

[jyknight]

In D69751#1737360, @xbolva00 wrote:

It should never be required in normal circumstances, but this function can also be used while debugging, where that would be convenient.

Not sure how it can be used currently, maybe with a big luck, since there is null pointer dereferencing.

I am gonna commit this.

It surely cannot currently. That's why I was saying it _would be nice_ for it to be fixed so that if you have a block which is unattached, you can actually print it in a debugging situation, rather than crashing. That was the original intent, and it's certainly not critical, but it still makes sense.

[xbolva00]

Ok. If you want this debugging feature, feel free to modify code as you wish :) I was just worried that I may break it even more, somehow.

[tydeu]

@jyknight @xbolva00

Couldn't printing orphaned basic blocks be supported by simply having IsEntryBlock be true if the block is orphaned? That is, by simply changing the top two lines of the function to:

assert(BB && "block is null!");
bool IsEntryBlock = !BB->getParent() || BB->isEntryBlock();

For some background: I encountered an unexpected crash while trying to dump and orphan basic block for debugging purposes. When I investigated it, I discovered this old thread about the issue. Since printing orphaned basic blocks is still broken in 2021, I thought I would resurrect this topic (as being able to print them would be useful to me).

llvm/lib/IR/AsmWriter.cpp

if (Arg->hasName()) {
     int Slot = Machine.getLocalSlot(Arg);
     assert(Slot != -1 && "expect argument in function here");
     Out << " %" << Slot;
   }
 }
 
 /// printBasicBlock - This member is called for each basic block in a method.
 void AssemblyWriter::printBasicBlock(const BasicBlock *BB) {
+  assert(BB && BB->getParent() && "block without parent!");
   bool IsEntryBlock = BB == &BB->getParent()->getEntryBlock();
   if (BB->hasName()) {              // Print out the label if it exists...
     Out << "\n";
     PrintLLVMName(Out, BB->getName(), LabelPrefix);
     Out << ':';
   } else if (!IsEntryBlock) {
     Out << "\n";
     int Slot = Machine.getLocalSlot(BB);
     if (Slot != -1)
       Out << Slot << ":";
     else
       Out << "<badref>:";
   }
 
-  if (!BB->getParent()) {
-    Out.PadToColumn(50);
-    Out << "; Error: Block without parent!";
-  } else if (!IsEntryBlock) {
+  if (!IsEntryBlock) {
     // Output predecessors for the block.
     Out.PadToColumn(50);
     Out << ";";
     const_pred_iterator PI = pred_begin(BB), PE = pred_end(BB);
 
     if (PI == PE) {
       Out << " No predecessors!";
     } else {