[Lldb-commits] [lldb] [llvm] [NFC][DebugInfo] Make some block-start-position methods return iterators (PR #124287)
hvdijk wrote: I'm taking a look, but I'm having some doubts about the change itself, `DIBuilder::insertDbgValueIntrinsic`'s `BasicBlock *BB = InsertBefore->getParent();` (where `InsertBefore` is an iterator) doesn't seem safe, it seems like it should be valid to pass an at-the-end iterator there that cannot be dereferenced. I'll take a more in-depth look at the change and see whether I'm able to add any testing. https://github.com/llvm/llvm-project/pull/124287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [NFC][DebugInfo] Make some block-start-position methods return iterators (PR #124287)
hvdijk wrote: Thanks again, it ends up looking much better (IMO) with `InsertPosition`, and it manages to avoid the change on the users' side that I was hoping to avoid. https://github.com/hvdijk/llvm-project/commit/dibuilder-insertposition Since this allows us to have a single method rather than separate overloads (since `InsertPostion` handles both `Instruction *` arguments and `BasicBlock *` arguments via conversion) it also avoids the need for duplication in the unit testing. TODO (feedback welcome): Figure out what to do with the C API. In my commit, it compiles with a deprecation warning, but it's providing an interface for doing the exact thing that is meant to be deprecated, and the interface is inconsistent with how instructions are inserted through the C API, so my thinking is that this API should probably be replaced with one that uses `IRBuilder`? https://github.com/llvm/llvm-project/pull/124287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [NFC][DebugInfo] Make some block-start-position methods return iterators (PR #124287)
hvdijk wrote: Thanks, I knew it was possible (it had to be: it would be possible to create a dummy instruction, insert it at the specified point, and inspect its parent) but had not yet found the best way of doing it, I was trying to get it working with `IP.getNodePtr()->getParent()`. I'll update it to use that instead before I post anything for review. https://github.com/llvm/llvm-project/pull/124287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [NFC][DebugInfo] Make some block-start-position methods return iterators (PR #124287)
hvdijk wrote: Given that https://llvm.org/docs/RemoveDIsDebugInfo.html specifically documents these functions as "no plans to deprecate", even though I personally feel like they are a bad idea, I just updated them so they compile with no deprecation warnings. Does this look like a good approach? Should I create a PR, or would you prefer a different approach and/or would you like to make some changes to it? https://github.com/llvm/llvm-project/pull/124287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [NFC][DebugInfo] Make some block-start-position methods return iterators (PR #124287)
hvdijk wrote: Hi, in principle this change makes sense but it causes a bit of an issue with [`DIBuilder`](https://llvm.org/doxygen/classllvm_1_1DIBuilder.html) not yet having been updated to take iterators. The workaround for that at the moment is trivial (change e.g. `DIB.insertDeclare(..., BB.getFirstNonPHIOrDbg())` to `DIB.insertDeclare(..., &*BB.getFirstNonPHIOrDbg())` as you do in other places in this PR), but that workaround would break when the update does land to have `insertDeclare` etc. take iterators. Could I ask what the best way forward here is for downstream users that need to remain compatible with multiple LLVM versions? Is it better to make this change in downstream projects, or would we be able to update `DIBuilder` quickly enough that we can avoid that? https://github.com/llvm/llvm-project/pull/124287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [NFC][DebugInfo] Make some block-start-position methods return iterators (PR #124287)
hvdijk wrote: Yes, thank you, something like that looks great! I should be able to do some testing with that tomorrow to check to make sure I am not missing anything. https://github.com/llvm/llvm-project/pull/124287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
