add commands to change gdb session radices

[omarArm]

This PR essentially solves the issue #203
Added two commands to change gdb session radix from decimal to hex and vice versa. The commands are available in the command palette under the names:

1. CDT: Set Output Radix to Hex
2. CDT: Set Output Radix to Decimal

[cwalther]

Sounds useful, thanks! What does this affect exactly? I noticed it seems to affect integer-typed variables in the Variables view, anything else? (And I was a bit surprised to see that the view appeared to update automatically, without sending an invalidated event.)

Initial thoughts:

• Should there be a when condition on having an active debug session of the right type?
• Should this be added to the context menu of the Variables view?
• Probably doesn’t matter, but using -gdb-set (the MI command) would look more fitting to me than set (the console command).
• Would the “CDT” prefix better be specified as "category": "CDT" instead of in the command name? Also, there is no precedent for “CDT” as far as I can see – maybe “CDT-GDB” or something like that would be more recognizable to users not familiar with CDT?
• I will try if I can also easily integrate this in our extension where I currently use my own debugger contribution using a subclass of GDBTargetDebugSession with type: 'inos'.

[omarArm]

@cwalther glad you found it useful! Here is a list of answers to all of the points. The output radix is a global setting. Basically it affects all variables/expressions everywhere. That includes the variables window, the watch window, the debug console, and hovering over expressions in text files.

• The whole extension only gets activated on debug anyway. Check here
• I thought at first to add this to the context menu of different views. Then I had the idea that this is a global setting in GDB, so I should make it a global setting in the extension as well.
• Using the mi command would make more sense to me if the command was being sent by a specific MSDAP command. However, since I am just using an evaluate request, I went with the normal GDB command way
• That is a good point, I will use the category property for the command instead of adding it manually. I will use CDT-GDB prefix instead, the CDT was rather a placeholder.
• The command is available for target debugging and host debugging. I think it should easily work.

Final Note: I am planning to add some granularity in a future PR, in which users can choose which expression/variable would they like to be viewed in a hex formatting. I just figured to add a global setting for now, since that is the "easy" part of it

[cwalther]

Now that I try it properly, I find that it does not work. Executing the command fails with an error notification “Failed to set output radix to hex: CodeExpectedError: Evaluation of expression without frameId is not supported.” Do you maybe have local changes to cdt-gdb-adapter that I don’t have? In fact, the line frameId: undefined, // Currently required by CDT GDB Adapter seems useless to me, because the undefined value doesn’t make it through JSON serialization.

The whole extension only gets activated on debug anyway.

But that applies to any debugger, not just the one from this extension where we know that the adapter can deal with this evaluate request. Also, the extension may be activated for other reasons, and it stays activated even when there is no cdt-gdb debug session anymore.

[cwalther]

OK, found the problem: it is that GDBDebugSessionBase.evaluateRequestcalls doEvaluateRequest with alwaysAllowCliCommand = false, in contrast to GDBTargetDebugSession. I wonder what the reason for that was. @thorstendb-ARM (cdt-gdb-adapter#414)?

[omarArm]

@cwalther I tested that using a couple of boards that I have. I don't have a test setup for host GDB testing unfortunately . That is why I didn't catch the error. Maybe that could be avoided using the when clause? I will look for a solution and get back to you

[cwalther]

Also, now that I try it properly in a gdbtarget configuration, I find that it in fact does not automatically update the Variables view (it did when I entered the command in the debug console). So it seems like you do need to send an invalidated event after all. Or did that work for you?

Edit: Wait, we are in VS Code here, not in the adapter. So maybe the refresh can be done directly, without having the adapter send an event. Haven’t checked the API docs for that.

[omarArm]

The update happens after I do a single step. In other words, yes, we have to send another request afterwards to update all windows. The problem is that there is no "dummy" request in my mind that we can use for that. The invalidated event is an event sent from the adapter to the client, so I am not sure how can we use that.

Update: after doing some research, I think the only way we could do it is basically add a feature in the adapter in which it sends an invalidated event whenever a set command is provided via an evaluate request. It will be a special case just like what we have for the breakpoint unsupported commands. I can do that in a separate PR of course after this one gets accepted. What do you @cwalther @jreineckearm @jonahgraham think?

[jreineckearm]

See some minor comments inline.

@omarArm , given this is one of multiple planned changes, we probably should open an issue here that outlines the changes on the adapter and the vscode extension. To give reviewers insights in what's to come.

Regarding open questions of the conversation above:

I thought at first to add this to the context menu of different views.

Agree with the context menu for easy access (and as discussed internally). The global switch then would be context-insensitive. Couple of views where it would make sense: Variables, Watch, Source Editor. Happy to further discuss. As Omar writes, more PRs to come.

Should there be a when condition on having an active debug session of the right type?

Agreed, let's test more how this behaves in reality. I believe the activation events are "first time activation" after loading an extension. So, connect/disconnect may leave the commands available after all connections have closed.

OK, found the problem: it is that GDBDebugSessionBase.evaluateRequestcalls doEvaluateRequest with alwaysAllowCliCommand = false, in contrast to GDBTargetDebugSession. I wonder what the reason for that was. @thorstendb-ARM (eclipse-cdt-cloud/cdt-gdb-adapter#414)?

If I remember correctly, we (@thorstendb-ARM ) made these changes at a time where we started familiarizing with the code base. And we didn't want to make massively disruptive changes. Now that we understand things a little more, I am tempted to always allow undefined as value and use the global scope as specified in DAP. That should simplify the code and hopefully make some error messages go away. Could be in a different PR though as it impacts more than just the new feature here.

Invalidate event

Need to better understand this area and what triggers events. In general, we should limit heuristics based on detected commands.
I think the provided context can make a difference? For example like in when commands are echoed to the debug console. Let's capture this in the overall feature issue to create, @omarArm , and address separately.

[cwalther]

OK, I’ll get out of the way for a bit while you have the internal discussions and until I get a better impression of the further planned changes (I shouldn’t spend all day on this anyway).

add a feature in the adapter in which it sends an invalidated event whenever a set command is provided via an evaluate request.

Yes, something like that was what I had in mind as a worst-case solution, or maybe a custom command sent to the adapter that only causes it to send an event back. But maybe there is a simpler way of triggering the effect of an invalidate event locally, without involving the adapter, I haven’t checked.

I also noticed GDB notify async: cmd-param-changed,param="output-radix",value="10" in the verbose output – maybe the adapter could turn that into an invalidate event. That could cover more parameters.

[jreineckearm]

Thanks, @cwalther , your feedback is much appreciated as always. Will certainly ask for more in a bit.

I also noticed GDB notify async: cmd-param-changed,param="output-radix",value="10" in the verbose output – maybe the adapter could turn that into an invalidate event. That could cover more parameters.

I like that idea. It would also cover other operations that change the radix in the GDB client.

[jonahgraham]

I think the only way we could do it is basically add a feature in the adapter in which it sends an invalidated event whenever a set command is provided via an evaluate request.

Sounds like a reasonable conclusion. I am fine for that to be in another PR.

[jreineckearm]

See one review comment. Also, should we call the commands Set Global Output Radix... (note that additional Global). As a preparation for more fine granular radix settings?

Functionality works with known limitations for now. So, I'd be good getting this in as an increment.

But needs follow-up work to make easier to use:

• cdt-gdb-vscode
  • Add context menu entries in debug views that are impacted by this, if possible: Variables, Watch, Source Editor. Toggle would be preferred. To see how this agrees with additional more fine granular radix settings, for example per watch expression. New child ticket to Add a command that allow users to change GDB session radix #201
• cdt-gdb-adapter
  • Send an invalidate all event if seeing the notification that @cwalther pointed out.
  • Allowing undefined frames for evaluate requests and variables request cdt-gdb-adapter#503 to properly support undefined frame IDs as global scope as specified in MSFT DAP.
• Provide "refresh all" type operation cdt-gdb-adapter#430 is not a must but sorting that will help us going forward with programmatic view refresh requests.

I hope I have all involved issues now somehow linked to #201

[cwalther]

As long as the commands are only in the command palette, I am OK with not making them conditional on the debugger type – when a user chooses a palette command explicitly labeled with “CDT-GDB” while using a debugger that is not CDT-GDB, it’s their own fault if they get unpredictable results.

(And that also sidesteps the question of how to best make things conditional on any debugger based on cdt-gdb-adapter, not just those with type: gdb and type: gdbtarget, such as mine.)

[jreineckearm]

LGTM