Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to choose which editor to use for editing variable binary data #155597

Open
thegecko opened this issue Jul 19, 2022 · 20 comments
Open
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Milestone

Comments

@thegecko
Copy link
Contributor

There has been a new feature introduced to view and edit binary data of variables:

https://code.visualstudio.com/updates/v1_64#_viewing-and-editing-binary-data

This is a powerful feature and has the potential to expose fine-grained workflows such as disassembly views.
However, it enforces the user installs and uses the Hex Editor ms-vscode.hexeditor extension:

const HEX_EDITOR_EXTENSION_ID = 'ms-vscode.hexeditor';

Can a setting be exposed to allow the user to override this behaviour and choose a different extension for inspecting variables?

@weinand weinand assigned connor4312 and unassigned weinand Jul 19, 2022
@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Jul 19, 2022
@connor4312
Copy link
Member

This is not something I am keen to do, as the interface that VS Code uses to expose the memory that is open in the hex editor is not a public contract. It's subject to change, and change is likely to happen if/when we support non-contiguous memory.

@connor4312 connor4312 added the feature-request Request for new features or functionality label Jul 19, 2022
@vscodenpa vscodenpa added this to the Backlog Candidates milestone Jul 19, 2022
@vscodenpa
Copy link

This feature request is now a candidate for our backlog. The community has 60 days to upvote the issue. If it receives 20 upvotes we will move it to our backlog. If not, we will close it. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

@thegecko
Copy link
Contributor Author

This is not something I am keen to do, as the interface that VS Code uses to expose the memory that is open in the hex editor is not a public contract. It's subject to change, and change is likely to happen if/when we support non-contiguous memory.

Can this contract be made public at that point and allow other extensions to be used? And even marked as experimental in the meantime?

VS Code as a composable environment should ideally offer the flexibility for users to choose the extensions they need.
If there isn't a choice, perhaps the hex editor functionality should become part of the VS Code core codebase?

@connor4312
Copy link
Member

perhaps the hex editor functionality should become part of the VS Code core codebase?

That's something we discussed, and decided to take a similar approach to the one we use flame chart where we install it on the fly. For a feature that a very small percentage of VS Code users are likely to see, we didn't want to increase our overall footprint for it.

Can this contract be made public at that point and allow other extensions to be used? And even marked as experimental in the meantime?

I think it could be marked experimental after non-continuous memory is available (and when a few questions are sorted microsoft/debug-adapter-protocol#239 microsoft/debug-adapter-protocol#238).

@ronakay22

This comment was marked as off-topic.

@benmcmorran
Copy link
Member

I'm on a team building VS Code extensions for embedded development. Directly viewing memory is an important part of the debugging process for these bare-metal applications to diagnose issues like stack overflows and data corruption. We currently integrate with VS Code's built-in debug memory view but given its lineage as a hex file editor it has a number of limitations that make it less suitable for this application. For example, the base address is wrong (it always appears as zero), the values do not change when memory changes in the debug session, and the 128KB reads are far too large for many embedded devices which often have limited memory and sections of memory that cannot be read from (e.g. write-only address that map to the physical state of a pin).

@haneefdm recently reached out to our team after identifying these same limitations and creating his own extension. What is the current thinking around memory views in VS Code and potential extensibility points? I see at least two options, but I'm sure there are others:

  • Focus entirely on the existing vscode-hexeditor extension and improve its compatibility with embedded scenarios. This will likely also require changes to VS Code core to make memory reads smaller, correctly set up base addresses, and support a gesture to open an arbitrary address, for example.
  • Provide a documented extensibility point for custom binary data editors and encourage extension developers to interface with that extensibility point.

If improvements to vscode-hexeditor aren't currently on the backlog, in the short term we plan to point our users at the new memview extension.

@connor4312
Copy link
Member

connor4312 commented Aug 31, 2022

Thanks for the comment! I think few of your concerns are actually a result of VS Code using the hex editor...

We currently integrate with VS Code's built-in debug memory view but given its lineage as a hex file editor it has a number of limitations that make it less suitable for this application. For example, the base address is wrong (it always appears as zero), the values do not change when memory changes in the debug session

  • "the base address is wrong" -- this is a bug we should fix
  • "values do not change when memory changes in the debug session" - This should work, and I just verified that it looks like it still is using our test debug adapter. To confirm, are you sending valid DAP memory events?

the 128KB reads are far too large for many embedded devices which often have limited memory and sections of memory that cannot be read from

It is expected that the returned value from readMemory can be less than 128KB, and in fact DAP has unreadableBytes for that purpose, though this is currently treated as an EOF.

I think all of the limitations in the blog post are in fact ambiguities in DAP that we have not yet clarified--or places thaht it has not expanded to cover--and that we tried not to assume in VS Code. I have a couple issues you may be interested in weighing in on.

Because these are DAP problems, I'm not surprised @haneefdm saw similar issues in VS, since they also use DAP (though they often extend it for their 1st party integrations and may have worked around some limitations independently)

You are of course free to integrate with 3rd party extensions rather than using standard memory support, however we would appreciate participation in DAP--from you as well as @haneefdm if they have time!--to make VS Code and all other DAP-consuming editors and IDEs better 🙂 It also might be the case that you want to use a 3rd party editor initially and then switch back to the built-in support once DAP and VS Code meet your requirements.

@haneefdm
Copy link
Contributor

haneefdm commented Aug 31, 2022

While my extension is not yet mature, it has a lot of features (some still in my sandbox) already. I want to provide some links for your perusal.

In Preview right now: https://github.com/mcu-debug/memview
In link above, I propose a URI based API to start a memory view. Everything else (including the lifecycle, and persistence, is handled by the URI handler). Feel free to make alternate proposals. I wanted it to be compatible with the way you already launch the Hex Editor (user-specified scheme and authority)

https://github.com/Marus/cortex-debug/wiki/Memory-Viewer
Needs to make this up to date with regard to decisions already made.

Editing discussion: mcu-debug/memview#4

@haneefdm
Copy link
Contributor

Actually, I did not see any DAP problems. Except for one described below.

I think asking for any more memory than that fits in a couple of screen fulls is wasteful. It interferes with single stepping and can be very slow for embedded. This is the way the disassembler works which is super nice.

As for page size. This could be nice if the Debug Adapter knows about page sizes suitable for general purposes. In our case, we don't know what that would be because there are so many gdb-servers and thousands of devices -- and we are agnostic to devices/gdb-servers. In Cortex-Debug frontend (we need to move this to the backend), we never issue a request of more than 4K but fulfill all large requests as if it is one request. The 4K comes from experience dealing with 7 different gdb-servers. Even in memview I keep memory requests small (0.5KB) -- it does not affect performance IMO, scrolling is still smooth, etc.

The one area of concern in the DAP is errors/failures. There are two broad kinds

  • The debugger is busy (perhaps the user did a continue, perhaps they are furiously single stepping), or the DA/debugger paused briefly to help set a breakpoint
  • We have a genuine read/write error because no one knew that this area of the memory is not accessible. Many times gdb does not know. The gdb-server knows but sometimes it doesn't either. Happens at the edge of memories as well. Can cause page faults even in embedded.

Sometimes one word with a span of 128 words is not writable (or readable) but that is another matter and is an edge case.

My issue is that when an error occurs, we don't know if we should retry or it will never work. This is also the same problem with evaluating expressions. Different DAs deal with errors differently and report them differently. Where the DAP can help is having a way to indicate in the error response what kind of an error it is. Especially if a request cannot be satisfied because the debugger (gdb) won't allow it at this time. Other debuggers may not have an issue with satisfying requests while the program is running. I bet IAR debugger will have no issues accessing memory while the program is running. They do that all the time.

Btw, we expect memory references base/start address to be an expression. Not always a constant which a memoryReference is. This was a big request for us and now, people expect this.

Nothing to do with DAP but, one area VSCode could consider is providing better APIs for debugger frontends that are detached from the DA

  • Ability to query the list of debug sessions
  • Query the status of each (started, running, stopped, etc.)
  • Events we could subscribe to is also good

Does not obviate the need for trackers though. But saves everyone a lot of trouble and prone to errors.

I have seen many issues submitted on this but users were always redirected to using the Debug Tracker. So, I didn't ask again. There are now 3 Debug Trackers that I know of running in one session. One is in Cortex-Debug, there is one in Memview, and one in MS Embedded Tools.

I have seen many other trackers in various other extensions as well.

@haneefdm
Copy link
Contributor

"values do not change when memory changes in the debug session" - This should work, and I just verified that it looks like it still is using our test debug adapter. To confirm, are you sending valid DAP memory events?

@connor4312 All I did was to use existing DAs (Cortex-Debug and maybe cppdbg as well) and created a memory view from the Variables Panel and watched the traffic between VSCode and DAs every time the program pauses. I only saw an initial read. Never again.

We don't support MemoryEvents. Like the protocol says, it is supposed to be used in conjunction with setVariable/setExpression that too if the client is interested.

If we are expecting only memory events to refresh memories, then no. But you are not expecting such events when the program pauses after running for a bit, do you?

I still see MemoryEvent as bunch of work and is not that simple. We don't have memory references for variables so we have to get them. With an expression, we have no idea what that expression did. The expression could even include function calls. We have to pretty much assume all information is stale if

  • a request for set variable/expression was made
  • a request for write memory was made
  • a command was issued from the Debug Console REPL

A cursory look at MIEngin, cpptools, and cspy shows that there is no support for MemoryEvents. Could you refer me to the DA that generates these events?

@connor4312
Copy link
Member

connor4312 commented Aug 31, 2022

The 4K comes from experience dealing with 7 different gdb-servers. Even in memview I keep memory requests small (0.5KB) -- it does not affect performance IMO, scrolling is still smooth, etc.

Thanks for the insight on page size. Maybe using a smaller default is sufficient; I'll experiment with that as well.

The one area of concern in the DAP is errors/failures. There are two broad kinds...Sometimes one word with a span of 128 words is not writable (or readable) but that is another matter and is an edge case.

Please open DAP issues for these!

Btw, we expect memory references base/start address to be an expression. Not always a constant which a memoryReference is.

Can you explain a little more? If I understand correctly, canonical way this would be done in DAP today would be to evaluate and use the memoryReference in the result of that to show the viewer. Did you try that approach and did it not work for you?

If we are expecting only memory events to refresh memories, then no. But you are not expecting such events when the program pauses after running for a bit, do you?...

I think you are making an assumption about the way memoryReferences work which is not in the specification. This is what I am asking about in microsoft/debug-adapter-protocol#238. Today, the variableReferences that get returned from evaluation and variables from a stackframe only while the DA is stopped. Thus, we don't "refresh" the memory the next time the program pauses, because the memoryReference (under current rules) is no longer guaranteed to be valid.

I definitely do want to do in VS Code what you do in your extension where the memory view can be kept open and updated over time, but the reason we don't is that it isn't ensured that it's safe to do so, yet 🙂

Could you refer me to the DA that generates these events?

https://github.dev/microsoft/vscode-mock-debug does this when changing a variable.

@haneefdm
Copy link
Contributor

haneefdm commented Sep 1, 2022

Can you explain a little more? If I understand correctly, canonical way this would be done in DAP today would be to evaluate and use the memoryReference in the result of that to show the viewer. Did you try that approach and did it not work for you?

Yes, it is a two-step approach. And that is what we do in the two different memory viewers we have (Cortex-Debug and memview and did them both) Cortex-Debug had this for about 3 years now but it is very simple in terms of presentation and features, which is why I started memview so it can be used by any debugger.

@vscodenpa
Copy link

🙂 This feature request received a sufficient number of community upvotes and we moved it to our backlog. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

@vscodenpa vscodenpa modified the milestones: Backlog Candidates, Backlog Sep 3, 2022
@haneefdm
Copy link
Contributor

Just FYI: A couple of weeks ago, our repo (and extension) has moved into a new organization.

https://github.com/mcu-debug/memview

I am not sure what the next steps are here. It is marked as Backlog but is there anything I can do to help?

@haneefdm
Copy link
Contributor

Just pinging again. @connor4312 do you know what I can do to help? I know the HexEditor made some progress but it still falls short in terms of life-cycle management.

Also, with my extension, we don't need VSCode proper to do any memory reads itself or worry about refreshes and life-cycle management. No need for a virtual file system either. This may need a second option on who manages the data model (actual reads/writes, binding to debug sessions, etc.) We can do a much better job and is not a one size fits all mechanism, because we are tailored for debugging. Not file editing.

@thegecko
Copy link
Contributor Author

Any update on allowing the user to specify their own memory view extension here?

@thegecko
Copy link
Contributor Author

Another ping for an update on this. If an approach can be agreed, I'm happy to open a PR to implement.

@jnz86
Copy link

jnz86 commented Aug 10, 2023

Another vote here.

When working in embedded, the Microsoft hex editor is less than ideal. I should be able to select a different default editor for use when clicking on the View Binary Data button from the watch window.

@thegecko
Copy link
Contributor Author

thegecko commented Nov 4, 2023

FYI, I have opened #197458 to potentially resolve this issue through exposing additional debug settings.

It's not pretty and may want to be a different approach, but it does the job and I'm keen to keep the ball rolling on this issue.

@thegecko
Copy link
Contributor Author

We now have an initial release of our advanced Memory Inspector:

https://marketplace.visualstudio.com/items?itemName=eclipse-cdt.memory-inspector

It could be a good test case for replacing the basic HexEdit view using #197287

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

9 participants