This is an archive of the discontinued LLVM Phabricator instance.

[AsmWritter] Fixed "null check after dereferencing" warning
ClosedPublic

Authored by xbolva00 on Nov 2 2019, 10:05 AM.

Details

Summary

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

Event Timeline

xbolva00 created this revision.Nov 2 2019, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2019, 10:05 AM

@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 , 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 marked an inline comment as done.Nov 2 2019, 12:25 PM
xbolva00 added inline comments.
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 updated this revision to Diff 227585.Nov 2 2019, 12:28 PM

Addressed review comment.

xbolva00 marked an inline comment as done.Nov 2 2019, 12:30 PM
xbolva00 added inline comments.

Ping, this looks kinda easy to review...

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.

I am not sure...

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

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.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 7 2019, 10:38 AM
This revision was automatically updated to reflect the committed changes.

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.

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 added a subscriber: tydeu.Aug 12 2021, 6:06 AM

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