Support value formatting by omarArm · Pull Request #506 · eclipse-cdt-cloud/cdt-gdb-adapter

omarArm · 2026-03-09T10:54:57Z

Should solve eclipse-cdt-cloud/cdt-gdb-vscode#210

New implementation:

Updated the function sendVarSetFormatToHex to make it more generic to different value formats. Also changed the return type from void to string, to retrieve the actual value.
In the evaluate request, added support for setting value format for expressions
Added a new property displayFormat to VarObjType

Now users can provide format specifiers to their expressions. It can be

,x  -> hex
,o  -> octal
,b -> binary
,d -> decimal
,z -> zero-hexadecimal

Old implementation:

Supporting value formatting in evaluate requests.

Updated function sendVarSetFormatToHex to return string that includes updated value in hex instead of void
Checking on format property of evaluate request arguments before sending result

Notes

the feature only works with primitive data types so far

jonahgraham

In addition, please write a test or two to show requests with the format set.

omarArm · 2026-03-12T14:11:18Z

After further discussions in this PR, also in eclipse-cdt-cloud/cdt-gdb-vscode#208 and privately with @jreineckearm

I drastically changed the implementation in this PR to accommodate multiple value formats and also make it easier for other extensions to make use of the adapter.

jreineckearm

Thanks for the updates, see some comments.

asimgunes

Minor comments about code quality.

jreineckearm · 2026-03-19T12:59:35Z

Thanks for the feedback, @asimgunes . Comments make sense. Be aware that @omarArm is away until beginning of April. Hence will only pick the feedback up afterwards.

jreineckearm · 2026-03-26T05:28:40Z

Converted to draft until Omar is back to sort out review comments.

cwalther

I am not familiar enough with these code areas for a thorough architectural review, but in the details it looks sensible now.

In some anecdotal testing on an Indel target, it appears to work.

I notice that when I make a watch expression with a local variable and a format specifier, that also affects the display of that variable in the Variables view. Is that intentional? Also, when I have two watch expressions with the same expression, but different format specifiers, they affect each other – that is probably not intentional. I think being able to see the same value in two different formats would sometimes be useful.

jreineckearm

Given feedback makes a lot of sense, thanks to everyone helping here.
Some of the feedback should probably go into a new PR to keep the changes better reviewable. Omar is working on some final changes for this one here.

I am away until Thursday next week. Can please one of the other committers help with merging after Omar did these changes and while I am away? Thanks

jonahgraham · 2026-04-01T14:13:08Z

watch expression with a local variable and a format specifier, that also affects the display of that variable in the Variables view.

What happens is that you also can't look at the same value in different formats at all, e.g:

when I step it is rather random what format gets chosen:

Between two of those the value of k doesn't change, but because the format changes, VSCode is highlighting it like a changed value

I think this is related to the earlier discussion about keeping track of what user requested "this time" vs what GDB has been told var format is. I can look more closely later, but there certainly seems to be a specific use case here that is problematic.

Interestingly CDT doesn't support setting individual expressions to a specific format, instead the setting applies to all the displayed variables. But what CDT did is used the "details" pane to display all the formats:

omarArm · 2026-04-02T08:45:12Z

@cwalther @jonahgraham I did some preliminary investigation on why that might be happening. My guess so far is that caching the value format might not be a good idea after all, since GDB does that for us anyway.

I suggest we merge the current PR first and then, for better review granularity, I will start right away with another PR to improve the behaviour.

cwalther

I am fine with merging this as-is (squashed and rebased) and addressing the crosstalk separately, but I’ll defer to @jonahgraham.

jonahgraham · 2026-04-09T13:31:59Z

I see you have continued to work on this - I just realized I never commented in response to #506 (comment) - but since the current implementation can show incorrect data I don't think we should merge until resolved. I see @omarArm has added new commits since with title "eliminating interference between same expressions with different valu…" so it sounds like that is the way things are progressing anyway.

omarArm · 2026-04-09T13:58:06Z

@jonahgraham yes, we came to the same conclusion that we should not merge without fixing the interference issue. After some investigation, I opted for the current implementation.

jonahgraham

This LGTM - I tried it out and had a look. I know you are still polishing and resolving test failures, but besides that we should be good to go.

cwalther · 2026-04-09T14:57:06Z

Would using -var-evaluate-expression -f <format> instead of toggling the format achieve the same thing, but save one MI call?

omarArm · 2026-04-10T09:04:26Z

@cwalther nice catch. I must have missed this command while reading the documentation. I tried it and it worked

cwalther · 2026-04-10T14:14:28Z

    expectRejection,
    fillDefaults,
-    gdbAsync,
+    //gdbAsync,


One more leftover comment.

Also, I think the commits that touch this should be squashed separately from the rest, because that is not really related to this PR, but rather an amendment of #523. (Making a separate PR for it is probably overkill, but if you want I can merge it.)

Done. I will squash before merging then

cwalther

Code looks good to me now, thanks for the patience! (You could have left the async, if you prefer it that way, my comments are not meant as orders.) I would like to give it another test, but that will have to wait until Monday. If you would prefer to have it merged before the weekend, I am fine with @jreineckearm or anyone else merging.

jreineckearm · 2026-04-10T15:01:14Z

Thanks for the review, @cwalther .

We had discussed offline why the async isn't needed anymore and that it worst case can add a tiny overhead at runtime (certainly not critical here). I am OK to wait until Monday, I just would like us to get done with it. Still have a full backlog on our main CMSIS Debugger project.

…e formats

Send invalidate event on radix change (eclipse-cdt-cloud#523) Sends an invalidate variables event to client when seeing GDB async notification that `output-radix` changes. removing residue changes originally introduced to var manager

trying out new eval expression command code cleanup restoring function prototype to old version prettier

removing a single leftover comment

cwalther

Tested, looks good.

I assume @jreineckearm will merge once you are done tidying up commits; if not, ping me if you want me to do it.

jreineckearm

Thanks, LGTM! Gave it a try, and it took me a while to realize I shouldn't add a space between , and x in ,x to make it work. ;-)
Thanks, @cwalther , for your support on this one!
Merging

cwalther · 2026-04-13T10:00:52Z

But please get the commits into a clean shape before merging. (As mentioned I would prefer two commits.)

cwalther · 2026-04-13T10:05:25Z

You squashed it into one. Oh well, not a big deal.

jreineckearm · 2026-04-13T10:07:11Z

But please get the commits into a clean shape before merging. (As mentioned I would prefer two commits.)

Sorry, forgot about it.... let me know if you'd like me to revert and split up.

cwalther · 2026-04-13T10:11:00Z

Do you have push access to the main branch? Last time I tried, I didn’t, the only write access I had was using the GitHub web UI to merge PRs.

But anyway, don’t bother if you have more important things to do. The piece that doesn’t belong here is small and not important.

jreineckearm · 2026-04-13T10:19:54Z

I am afraid not, you should be able to see branch protection rules here: https://otterdog.eclipse.org/projects/ecd.cdt-cloud/repos/cdt-gdb-adapter#branch-protection-rules
I believe the important bit is requires_pull_request.

The piece that doesn’t belong here is small and not important.

OK, sorry again. Had completely forgotten about it over the weekend and only saw your comment the very second that I had pushed squash and merge after cleaning up the commit message.....

cwalther · 2026-04-13T10:24:49Z

No worries.

omarArm requested review from cwalther, jonahgraham and jreineckearm March 9, 2026 10:56

jreineckearm reviewed Mar 9, 2026

View reviewed changes

Comment thread src/mi/var.ts Outdated

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

Comment thread src/mi/var.ts Outdated

jonahgraham reviewed Mar 10, 2026

View reviewed changes

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

omarArm requested a review from asimgunes March 11, 2026 10:08

omarArm mentioned this pull request Mar 12, 2026

Set Value Format Property to Hex eclipse-cdt-cloud/cdt-gdb-vscode#208

Closed

jreineckearm reviewed Mar 12, 2026

View reviewed changes

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

Comment thread src/mi/var.ts

Comment thread src/mi/var.ts

asimgunes reviewed Mar 19, 2026

View reviewed changes

Comment thread src/varManager.ts Outdated

Comment thread src/varManager.ts Outdated

jreineckearm marked this pull request as draft March 26, 2026 05:28

omarArm marked this pull request as ready for review March 31, 2026 14:12

cwalther approved these changes Mar 31, 2026

View reviewed changes

Comment thread src/mi/var.ts Outdated

jonahgraham reviewed Mar 31, 2026

View reviewed changes

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

jreineckearm reviewed Apr 1, 2026

View reviewed changes

cwalther approved these changes Apr 2, 2026

View reviewed changes

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

jonahgraham reviewed Apr 9, 2026

View reviewed changes

cwalther reviewed Apr 10, 2026

View reviewed changes

Comment thread src/mi/var.ts Outdated

cwalther requested changes Apr 10, 2026

View reviewed changes

cwalther approved these changes Apr 10, 2026

View reviewed changes

Supporting value formatting

48e566b

omarArm added 7 commits April 10, 2026 17:05

updating tests

1e9d46f

eliminating interference between same expressions with different valu…

cdb4c6d

…e formats

adding a test for view interference case

411c591

Send invalidate event on radix change (eclipse-cdt-cloud#523) Sends an invalidate variables event to client when seeing GDB async notification that `output-radix` changes. removing residue changes originally introduced to var manager

trying out removing the guard for CI

5d3163c

add format option to evaluate expression command

fab0df9

trying out new eval expression command code cleanup restoring function prototype to old version prettier

removing commented out code

263e9a6

removing a single leftover comment

removing unneeded async

93d43d3

omarArm force-pushed the supportValueFormatting branch from 0d69d2b to 93d43d3 Compare April 10, 2026 15:07

omarArm and others added 3 commits April 13, 2026 10:31

Merge branch 'main' into supportValueFormatting

95efc9d

removing unneeded import

4184ee6

prettier

067cbfc

cwalther reviewed Apr 13, 2026

View reviewed changes

Comment thread src/gdb/GDBDebugSessionBase.ts Outdated

updating error message to include all format specifiers

dc9c1cf

jreineckearm approved these changes Apr 13, 2026

View reviewed changes

jreineckearm merged commit 039367f into eclipse-cdt-cloud:main Apr 13, 2026
4 checks passed

Conversation

omarArm commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New implementation:

Old implementation:

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonahgraham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

omarArm commented Mar 12, 2026

Uh oh!

jreineckearm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asimgunes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jreineckearm commented Mar 19, 2026

Uh oh!

jreineckearm commented Mar 26, 2026

Uh oh!

cwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jreineckearm left a comment

Choose a reason for hiding this comment

Uh oh!

jonahgraham commented Apr 1, 2026

Uh oh!

omarArm commented Apr 2, 2026

Uh oh!

cwalther left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahgraham commented Apr 9, 2026

Uh oh!

omarArm commented Apr 9, 2026

Uh oh!

jonahgraham left a comment

Choose a reason for hiding this comment

Uh oh!

cwalther commented Apr 9, 2026

Uh oh!

omarArm commented Apr 10, 2026

Uh oh!

Uh oh!

cwalther Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

omarArm Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cwalther left a comment

Choose a reason for hiding this comment

Uh oh!

jreineckearm commented Apr 10, 2026

Uh oh!

cwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omarArm commented Mar 9, 2026 •

edited

Loading

cwalther left a comment •

edited

Loading