[Lldb-commits] [lldb] [llvm] [mlir] [DebugInfo] Add symbolic branches to DIExpression (PR #210850)

[Eric Christopher via lldb-commits]

echristo wrote:

> I tend to agree with @felipepiovezan -- it's not that there's anything wrong with this code, instead it's not clear how we should consider it's composition with other features because nothing generates symbolic-branches yet, and we can't consider the use-cases. This manifests as uncertainty and hedging at later dates.
> 

I did think that this was a fairly straightforward implementation of a dwarf2 feature that doesn't see any use yet. I'd have liked to avoid an RFC, as I really did think this part was fairly straightforward and similar to how a number of things have been landed, but I did a little reading between the lines on some of the code and think I see how and why the concerns in this thread are manifesting.

Specifically making sure that @felipepiovezan and @Snowy1803 also see the comments here as well and @adrian-prantl as I think the 4 of you are the folks that have looked at this particular area the most.

I'm still taking a look at DIArgList handling and how we're going to integrate that into symbolic debugging. It mostly looks straightforward, but I've already found a couple of inconsistencies or edge cases in the code that might be good to handle while someone is in there :) Largely I'm trying to stay with the documented behavior and not necessarily what's in the verifier, but also what's in the passes. While I've been investigating I think I've found a few latent bugs so I'll be sending those out as separate patches. You're right that this should probably have some discussion between the group of us and if we do want DIArgList at least theoretically handled before we add any branch support then we should do it up front. FWIW I'm currently playing around with how I think it maybe should go, but it's going to take me a few days - couple of high priority work items taking my attention. If y'all have any particular thoughts I would love to hear them.

I did try to give a little forward direction and motivation in comments here that the next few steps are dwarf3 procedures which are the next feature I've got a patch ready for - those require some changes in Metadata (patch that I'll put up shortly) to add support for calls at which point we have expressions that have control flow and a little bit of consolidation for when expressions get large :)

We'll also be implementing dwarf6 extensions as those get approved through committee - the gpu workgroup has been busy the last few months/years :)

> A prime example is implicit pointers: you consider them in this patch and add carve-outs, however to the best of my knowledge nothing produces implicit-pointers today, and won't until #187642 lands (which may or may not have stalled). But we can at least know that because there's an overall plan/direction.
> 

This is a good one to talk about actually. It kinda sums up some of the simplification decisions I tried to make so that we could make the harder ones with some code in front of us :)

> I think @Snowy1803 has hit the nail on the head with composition with DIArgList: could this lead to two incompatible flavours of DIExpressions? Conceivably this is a legitimate direction because different applications (GPUs etc) have different characteristics, but it's worth considering from the top level down rather than implementation-upwards. Ensuring that "dead" or "invalid" #dbg_values are still retained to kill invalidated variable assignments is a consideration too.

These are all good questions and no, I don't think we could have two incompatible flavors of DIExpressions. That would be honestly unfortunate and we should try to avoid that - I don't really see it happening with how things happen in the stack machine, but if someone is concerned about something in particular I'd like to know. As far as the two flavors of DIExpressions, however, my next larger change does involve that a little bit and we should probably chat about it. I am going to write an RFC for it, but the summary is that metadata operands for DIExpressions makes calls to procedures work straightforwardly with LTO, etc. Currently if you don't use metadata for your expression you stay on version 3, but if you do then you get bumped to version 4, but only of the DIExpression. Anyhow, that's something for another RFC, but to give you a heads up.

And yes, poisoned expressions are on the list :)

Thanks everyone for the comments and concerns and give me a few days (and feel free to hit me up on discord if you want a real time conversation) and I'll have something in front of y'all. Opinions are very welcome, this is a fussy area for sure. If you do decide you're ok with the incremental patch for this and we can work on DIArgList together then I'm obviously ok with that too :)

Thanks!





https://github.com/llvm/llvm-project/pull/210850