-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[NFC]Add assert to avoid possibly deref nullptr #65564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@XinWang10 This patch doesn't fix the issue that you discovered. Most vendors ship LLVM with assertions disable for performance reasons. Can you fix this properly by either making the code that depends on MMI conditional or by making it impossible to have a null MMI member in AsmPrinter? |
+1 to what Adrian said. This commit doesn't improve anything: it just changes the type of crash from a segmentation fault to an abort, if you have asserts enabled. If the internal-consistency guarantee is that |
As a general rule of thumb, |
@JDevlieghere @adrian-prantl I know your principle is right. The reason I changed in this way is that this code has existed for several years, we haven't found any crash here so far, so I think add an assert would be a light weight fix to it. Using if-condition must be correct, but considering I have no evidence MMI would be nullptr from author's intention and if-condition could take more resource, I made this decision. I think we could let it here and if it do have risk it's not too late to change it back. |
First off, I appreciate you trying to fix this issue.
As Adrian and I pointed out earlier, the current patch doesn't fix anything. I would go even further and say it makes things worse, because the assert states that
You pointed out that it can be null if
Clearly, it's possible for a pass to not exist. If someone were to build LLVM without
Yes, we should use an if-condition instead. |
This reverts commit 99fb65f.
(doesn't seem to be debug info related) +1 to @aprantl and @JDevlieghere - if the null deref was reachable, then this needs proper error handling, the assert is insufficient/not the right tool. (& this needs a test case, if it is reachable) |
…llvm#66187) This reverts commit 99fb65f because it won't benefit.
assert is more appropriate here and fixes `runtime error: execution reached an unreachable program point` in a -DLLVM_USE_SANITIZER=Undefined build (-fno-sanitize-recover=all causes llc to exit instead of crash (report_fatal_error)) when testing MachineVerifier/test_g_assert_[sz]ext.mir.
These 2 functions could be called by AsmPrinter::doInitialization in AsmPrinter.cpp. doInitialization init MMI in the beginning
MMI = MMIWP ? &MMIWP->getMMI() : nullptr;
, MMI has the possibility to be nullptr, which could make the later deref crash. I think in most time MMI could not be nullptr, but from the view of function implementation, it could be, so I'd like to add assert to it, if this could be a problem, then we could avoid crash.