-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[llvm-c] Add missing nullptr check in LLVMGetFirstDbgRecord #151101
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
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: Maxime Arthaud (arthaud) ChangesI'm using the LLVM C bindings through the llvm-sys rust crate, and notice that LLVMGetFirstDbgRecord and LLVMGetLastDbgRecord are segfault-ing when called on instructions without debug markers. I found out it's missing a null pointer check. This PR fixes the issue. Full diff: https://github.com/llvm/llvm-project/pull/151101.diff 1 Files Affected:
diff --git a/llvm/lib/IR/Core.cpp b/llvm/lib/IR/Core.cpp
index f7ef4aa473ef5..56190545eb024 100644
--- a/llvm/lib/IR/Core.cpp
+++ b/llvm/lib/IR/Core.cpp
@@ -2984,6 +2984,8 @@ LLVMValueRef LLVMIsATerminatorInst(LLVMValueRef Inst) {
LLVMDbgRecordRef LLVMGetFirstDbgRecord(LLVMValueRef Inst) {
Instruction *Instr = unwrap<Instruction>(Inst);
+ if (!Instr->DebugMarker)
+ return nullptr;
auto I = Instr->DebugMarker->StoredDbgRecords.begin();
if (I == Instr->DebugMarker->StoredDbgRecords.end())
return nullptr;
@@ -2992,6 +2994,8 @@ LLVMDbgRecordRef LLVMGetFirstDbgRecord(LLVMValueRef Inst) {
LLVMDbgRecordRef LLVMGetLastDbgRecord(LLVMValueRef Inst) {
Instruction *Instr = unwrap<Instruction>(Inst);
+ if (!Instr->DebugMarker)
+ return nullptr;
auto I = Instr->DebugMarker->StoredDbgRecords.rbegin();
if (I == Instr->DebugMarker->StoredDbgRecords.rend())
return nullptr;
|
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.
Looks reasonable to me. Can this be tested in llvm/tools/llvm-c-test/debuginfo.c?
7031036
to
a2a9b1f
Compare
It looks like llvm/tools/llvm-c-test/debuginfo.c is testing functions that create LLVM IR, while my PR is fixing functions that read LLVM IR. We don't have C functions to create debug markers, from what I understand, so I'm not sure how to test this properly without having to add new C binding functions. Do we have tests somewhere for reading a .bc or .ll file with the C bindings? |
790d7d8
to
338ef0b
Compare
I realized that I can probably add a *.ll test in llvm/test/Bindings/llvm-c, but there is actually no way to do anything with a LLVMDbgRecordRef. We should probably add bindings to get information from a LLVMDbgRecordRef, then add tests for it. I will look into it. |
338ef0b
to
ccdd7ff
Compare
For a test, it should be sufficient to just check that the result is null (if there are no debug records). I think all you need to do is create an instruction without debuginfo and then test that calling LLVMGetFirstDbgRecord on it does not crash. |
ccdd7ff
to
c30a117
Compare
Fix a crash when calling LLVMGetFirstDbgRecord or LLVMGetLastDbgRecord on instructions without debug markers.
c30a117
to
e12de59
Compare
Done. Ran the test with |
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.
It looks like you address the outstanding comments, LGTM
Clearing through a long backlog of notifications... sorry this PR has been left. Do you have commit access or do you need this merging for you @arthaud ?
I need this merged for me, thank you! |
@arthaud Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
I'm using the LLVM C bindings through the llvm-sys rust crate, and notice that LLVMGetFirstDbgRecord and LLVMGetLastDbgRecord are segfault-ing when called on instructions without debug markers. I found out it's missing a null pointer check. This PR fixes the issue.
If possible, I would love if this was included in a potential 20.1.9 release.