[Lldb-commits] [lldb] [lldb] Store the return SBValueList in the CommandReturnObject (PR #127566)

Thu Feb 20 12:38:43 PST 2025

jimingham wrote:

> On Feb 20, 2025, at 1:52 AM, Pavel Labath ***@***.***> wrote:
> 
> 
> labath
>  left a comment 
> (llvm/llvm-project#127566)
> That sort of makes sense, but it doesn't sound like you're planning to do that soon, so I'm asking about what should we do until (and if) that happens. The reason I suggested stderr is because that made sense for the commands I'm thinking of. For example, the expression command can produce a result and some warnings and fixit-applied notes. I don't know if these currently go to stderr, but I think it would be okay if they did...
> 
> I don't think it's a good idea for commands to put any of their output anywhere but the command result. Anything that goes to stderr is lost, and particularly if is trying to run their own command loop, so that they are by hand printing the command result, there will be no way to associate that output with the command that produced it.
> 
> I see that there's been a misunderstanding. By "stderr" I meant the "stderr of the CommandReturnObject" (i.e., CommandReturnObject::m_err_stream, returned by SBCommandReturnObject::GetError), not the actual stderr of the debugger. So the convention would be that if CommandReturnObject contains a variable, then its output (GetOutput) would contain a rendering of rendering of that variable (and it can be ignored when doing your own printing), but the error output (GetError) would contain any additional data (like the fixits) that can/should be anyway.
> 
>  <https://github.com/llvm/llvm-project/pull/127566#issuecomment-2670992897> <https://github.com/notifications/unsubscribe-auth/ADUPVW2LCZ4HORI2JUMJNFD2QWQWLAVCNFSM6AAAAABXKWF5M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZQHE4TEOBZG4>
> 
> labath
>  left a comment 
> (llvm/llvm-project#127566)
>  <https://github.com/llvm/llvm-project/pull/127566#issuecomment-2670992897>
> That sort of makes sense, but it doesn't sound like you're planning to do that soon, so I'm asking about what should we do until (and if) that happens. The reason I suggested stderr is because that made sense for the commands I'm thinking of. For example, the expression command can produce a result and some warnings and fixit-applied notes. I don't know if these currently go to stderr, but I think it would be okay if they did...
> 
> I don't think it's a good idea for commands to put any of their output anywhere but the command result. Anything that goes to stderr is lost, and particularly if is trying to run their own command loop, so that they are by hand printing the command result, there will be no way to associate that output with the command that produced it.
> 
> I see that there's been a misunderstanding. By "stderr" I meant the "stderr of the CommandReturnObject" (i.e., CommandReturnObject::m_err_stream, returned by SBCommandReturnObject::GetError), not the actual stderr of the debugger. So the convention would be that if CommandReturnObject contains a variable, then its output (GetOutput) would contain a rendering of rendering of that variable (and it can be ignored when doing your own printing), but the error output (GetError) would contain any additional data (like the fixits) that can/should be anyway.
> 
> I'm sorry about the confusion. Does that sound better?
> 
That would work.
> We don't dump warnings on successful expression evaluation, though we do print fixits. I would suggest for the near term that if a command that produces a ValueObject returns successfully but with "notes" we should put those notes in the Error object - but still have the state be Success. That would make sense for the warnings and fixits since Adrian has been working on getting the error output into a structured form to better hold compiler warnings and errors.
> 
> I sort of like that, particularly as it lets you obtain the diagnostics in a structured form, but it also feels kinda wrong because -- if I'm not mistaken -- we are currently only setting the error field if we failed to retrieve the value. It sounds like it could confuse some code which is not expecting it.
> 
We always have a Status object in the ValueObject.  Up to now, if the Status object is in a Success state, then the contents of the Status object is undefined.  So I was suggesting giving a meaning to when the Status object is in a "Success" state for a ValueObject that is retrieved from a CommandReturnObject.  This shouldn't cause problems with other clients.  No one should be checking whether the current command returned an error by checking whether the status object has text in it or not, you should be calling Success() and seeing whether it is true or false.  After all, a failing command is not obligated to put error text in the Status.

OTOH, SBError (lldb_private::Status) doesn't have a "set text w/o setting the error state" method, so maybe it's better not to add that for just this purpose.  Something specific to the command interpreter makes more sense.  

In the long run, we need something more structured, because there's no requirement that you either "do all your warnings and then call ValueObjectPrinter" or vice versa.  So if we're handing the UI the pieces of the output with the intention that it reconstruct the console output but more interactively, we need to mark up the text like:

Then the UI could interleave these text fields with the interactive field that holds each local.

Jim
> I should also add that I don't really have a horse in this race. I have no plan on using this feature any time soon. I'm just thinking about the questions I would have if I was trying to implement it (either from the consumer or producer end). So it's possible this isn't something that's going to matter in practice...
> 
> —
> Reply to this email directly, view it on GitHub <https://github.com/llvm/llvm-project/pull/127566#issuecomment-2670992897>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2LCZ4HORI2JUMJNFD2QWQWLAVCNFSM6AAAAABXKWF5M6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZQHE4TEOBZG4>.
> You are receiving this because your review was requested.
> 

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