[Lldb-commits] [lldb] [lldb] Add support for updating string during debug process (PR #67782)

[via lldb-commits]

jimingham wrote:


> On Oct 5, 2023, at 1:05 AM, Pavel Kosov ***@***.***> wrote:
> 
> 
> BTW, I have no problem with the general direction of this change. It makes a lot more sense to ask a synthetic child provider to change a value - since it does represent the value of the ValueObject - rather than the summary which is just some free-form text. And being able to change characters in a string seems a reasonable thing to do, so switching the std::string comprehension from a Summary provider to a Synthetic Child Provider is the way to do that. So that part if fine.
> 
> But std::strings abound in code, and so if we're going to make this change I don't think we can make that printing less space efficient, which this change seems to have done. We should figure out why that's the case and fix for this to be a really good change.
> 
> So, is it Ok to use current SetValueFromCString api from ValueObject to ask synthetic provider to update the underlying string?
> 
I'm going to use SCP for "synthetic child provider" as I was getting tired of typing it out...  And I think you know most of what I'm saying here, but I wanted to make sure we are talking about it the same way...

Since "SetValueFromCString" is an API on ValueObject, we are free to give that whatever meaning makes sense.

However, I think it's confusing here to start with the model that ValueObjects with SCPs have "underlying strings".  The vast majority of data types represented by synthetic child providers have no single underlying string, they are mapping aggregates to aggregates or reinterpreting various regions (contiguous or not) of memory as though it were an aggregate of some sort.  So there will for the most part be no "underlying string".

More generally, we don't really do "SetValueFromCString" for aggregate types, only for scalar values.  So I don't see how SetValueFromCString is a useful general API on the aggregate type that actually holds the SCP.  You really have to specify the particular child you want to change.

What does makes pretty clear sense is that if SetValueFromCString is called on one of the synthetic children produced by the ValueObject holding the SCP, the child ValueObject should ask the parent's SCP: "can you change my value to the incoming string".  The SCP API would have to grow another interface "SetValueOfChildFromCString" to support this.  The SCP has to know how to do that in a way that round-trips (i.e. if I make a second ValueObject from the same variable, it will also show the changed value).  If it can do that, then it will make the change.  Note, it may very well have to change others of the synthetic child values to make that happen, depending on how the child is generated from the underlying data being represented.  It's also not guaranteed to be 1-1, since there might be many ways to change the underlying data that produce the requested change on the synthetic child.  We should probably decide whether "I can change to represent this, but not uniquely" should be an error or not.

> You mentioned previously that we may add SetSummaryFromCString api - in fact currently this is what I am doing - changing the whole string through its summary (please check attached gif for example).
> 
I think this is also a useful API though it is a bit confusingly named.  Sadly I can't think of a better one that isn't overly verbose.  You aren't really setting the summary - that's not a terribly useful operation if the underlying value doesn't produce that new summary when asked for it again.  It's really "ask the summary provider if it knows how to change the underlying data such that it will produce this new summary string".  But anyway, we can make this clear in the API docs.

By the way, as a side note, watching the part of your example where you change the raw string guts, it looks like we don't update summaries of a ValueObject if one of its children gets changed.  Be good to file bug on that one so we don't forget.

> But the problem with the new API is the same as for the changing characters through SBValue::GetData - IDE doesn't support it
> 
In the case of std::strings, which seems to be your primary motivator here, I think considering the string value to be the summary makes the most sense.  There really isn't a scalar type of "a buffer of memory starting at 0x123 and ending sometime later (maybe signaled by there being a `\0` somewhere)."  So representing std::string with a value providing SCP is not at all natural.  Also in this case, it's useful to be able to see its raw contents easily in the UI as you were doing in your example, and if you swapped the std::string ValueObject for some kind of a "C-String" ValueObject, it would be awkward to get back to the raw fields.

The UI knows that it is inserting a string it got from GetSummary in that slot in the UI, so it can easily know to call SetSummaryFromCString when that slot is edited.  I am not in favor of using a strained model for as ubiquitous a data type as std::string in lldb just because we don't want to have to teach the UI how to handle it properly.

Jim
> —
> Reply to this email directly, view it on GitHub <https://github.com/llvm/llvm-project/pull/67782#issuecomment-1748331579>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7EA2Y2567SD5JCJMTX5ZS6PAVCNFSM6AAAAAA5MH6OUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBYGMZTCNJXHE>.
> You are receiving this because you commented.
> 



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