Set Value Format Property to Hex

[omarArm]

Almost implements #206

After investigations, it turns out that most widely used C/C++ extensions just append expressions in the watch window with a ,x to view values in hex. This has been implemented here as well, instead of a context menu entry.

[omarArm]

Sorry, I had to convert to draft because of debug session breaking for an unknown reason

Edit: ready for review

[cwalther]

From a quick glance, it looks like this would interfere with any debugger, not just ones based on cdt-gdb-adapter. Do we want that? There may be programming languages in which a ,x suffix is an important part of an expression that should not be stripped off.

One would expect that if DAP defines a format.hex property, there would also be a built-in UI for setting it in VS Code, rather than every debugger having to hack it in like this…

[omarArm]

@cwalther unfortunately, there aren't any UI in vscode that does that. At least that is what is stated by vscode maintainers on multiple issues, an example could be found here. It has been reiterated that it should be the responsibility of the extension

[cwalther]

I think the GdbDebugTracker class could be made easier to use for other extensions (such as mine) to register for their own debug types if the registering was not done directly in the constructor. It could also be added to the official API of the extension (return value from the activate function). (I am not 100% confident yet that introducing my own debug type was the right way to go, but it seemed the most straightforward. How do other cdt-gdb-adapter extenders do that?)

Discarding the Disposable returned from registerDebugAdapterTrackerFactory means that the factory will not be unregistered when the extension is deactivated.

[omarArm]

@cwalther I forgot about the disposable, I added it now.

I refactored the code and made it easier to add new debug types. However, I am not sure I entirely get what you mean with adding it to the official API of the extension? do you mean adding some sort of command so users can provide a string to do so? or is there something that I am missing? if so please point me to a documentation that shows what I am missing

[cwalther]

Regarding extension API: Does this help?

Extension writers can provide APIs to other extensions by returning their API public surface from the activate-call.

Regarding the code: I’m not sure how registering for both GDB_DEBUG_TYPES and newDebugType in the same method would help me. When my extension calls that with its own debug type, we would end up being registered twice for gdb and gdbtarget, once from cdt-gdb-vscode and once from my extension (which I assume will result in two trackers being instantiated).

I would like to be able to do something like this in my extension’s activate function (where "inos" is my debug type)

const GdbDebugTracker = vscode.extensions.getExtension('eclipse-cdt.cdt-gdb-vscode').exports.GdbDebugTracker;
context.subscriptions.push(vscode.debug.registerDebugAdapterTrackerFactory("inos", new
GdbDebugTracker()));

or

const GdbDebugTracker = vscode.extensions.getExtension('eclipse-cdt.cdt-gdb-vscode').exports.GdbDebugTracker;
context.subscriptions.push(new GdbDebugTracker().register("inos"));

or

context.subscriptions.push(vscode.extensions.getExtension('eclipse-cdt.cdt-gdb-vscode').exports.registerDebugTracker("inos"));

or maybe we can come up with a better API.

[omarArm]

done. However, I don't have any way to test this, so please let me know how it goes on your setup

[cwalther]

Sure, I will give it a try once it stabilizes.

We are getting closer, but I don’t think this will work as intended yet because it adds the Disposable to the subscriptions of the cdt-gdb-vscode extension and not of the calling extension.

[omarArm]

Done, sorry for taking so long. It is my first exposure to the idea of providing extension APIs

[cwalther]

No worries, we are all learning together! (It generally seems like cdt-gdb-vscode/cdt-gdb-adapter were not specifically designed with extenders like what I am currently trying in mind, but we can pave the way as we go.)

You are still discarding the disposable, just one step further. Returning it from the GdbDebugTracker.registerDebugTracker method is not sufficient, because I cannot call that from my extension. It also needs to be returned from the registerDebugTracker API function that calls it.

[omarArm]

Done

[jreineckearm]

TBF: I am not overly happy with passing the entire tracker object through the extension API and giving direct access to disposables which should be owned by the CDT GDB Adapter. Feels a bit unsafe.
I'd rather prefer a function which registers a type name without returning internal objects, or just returning a boolean indicating success.

An alternative could be to expose the supported types as a VS Code extension setting. But that is probably not as convenient for reusing the extension.

@cwalther , may I ask how you reuse this for your own type? Do you use this extension off-the-shelf and only have a customized version of the adapter?

[cwalther]

@cwalther , may I ask how you reuse this for your own type? Do you use this extension off-the-shelf and only have a customized version of the adapter?

That is a good question, and is in fact something I have meant to discuss for a while. This detail discussion here is probably not the right place for that, so I have opened an new issue for it: #209.

Maybe that already addresses your concerns, otherwise I am happy to go into more detail on why I think what you propose wouldn’t work.

[jreineckearm]

Thanks for sharing more details, @cwalther . This is indeed a discussion we should have rather sooner than later to avoid designing ourselves into a corner. Note that we at Arm use a managed launch.json/tasks.json approach in CMSIS Solution/CMSIS Debugger and expect only expert users to look at the JSON files themselves (we had a similar but not the same approach like you in the beginning where we registered a * debug provider and looked after server values to identify our setups). Others derive their own types on source level AFAIK (best for them to answer though).
Let me digest this a little and answer in the other issue. It's probably also one for the next monthly sync or a dedicated call with interested parties.

For this PR, I would suggest to keep the extension API out for the time being and limit to gdb and gdbtarget. The type registering or whichever solution we come up with deserves a separate work package. And @omarArm will be unavailable for a couple of weeks soon. Hence, I'd love to get some of the PRs in flight over the line before he's gone, even if we have to cut down on enhancements.

[cwalther]

For this PR, I would suggest to keep the extension API out for the time being and limit to gdb and gdbtarget. The type registering or whichever solution we come up with deserves a separate work package.

That is fine with me.

[omarArm]

@cwalther @jreineckearm if so, I will revert the changes for adding to the extension API

[jreineckearm]

@cwalther @jreineckearm if so, I will revert the changes for adding to the extension API

Yes, please. Let's finish this increment here and take a bit of time to think the entire adoption story through, first.

[jonahgraham]

I don't understand why the logic should be in the extension and not in the adapter. i.e. in evaluateRequest in the adapter parse out the ,x in a similar way that > is handled. In otherwords, is there some strong reason we should implement this in the extenion?

[omarArm]

@jonahgraham So I have two reasons for doing it through the extension and not the adapter.

1. The idea that I had in mind was that if someone would use the adapter standalone for a different client, then they shouldn't care about any hacks in the adapter. On the otherhand vscode maintainers already announced multiple times that extension developers should hack their way through supporting value formatting.
2. If another extension is using cdt-gdb-vscode extension, then they might have their own custom views with custom evaluate requests. They shouldn't care about hacks when they are prepping the arguments for their custom evaluate requests and they should just use official MSDAP properties

[cwalther]

I am not sure the logic to set hex: false when there is no ,x suffix makes sense. To me, absence of a suffix means “I don’t care whether I get hex, choose what’s appropriate” or “use the global setting“, and not “I don’t want hex”.

Also, what about messages that already contain a message.arguments.format, is it appropriate to just replace that? E.g. because another DebugAdapterTracker has already done a similar thing, or because, as you mention in 2. above, the request comes from a custom view that expresses its format preferences the DAP way?

[omarArm]

After offline discussions with @jreineckearm, we decided to close this PR and switch the implementation to the adapter's side. Please refer to eclipse-cdt-cloud/cdt-gdb-adapter#506