Update VSCode dependencies and implement write memory request

[colin-grant-work]

The memory write request is now an official part of the DAP, and it's fairly straightforward to implement in GDB, and it would be nice to have to enable the memory write feature here.

The DAP suggests that if you don't intend to support partial writes, you should check whether the whole block is writeable before attempting to write. Grateful for any pointers on how that might be done.

Signed-off-by: Colin Grant colin.grant@ericsson.com

[eclipse-cdt-bot]

Can one of the admins verify this patch?

[jonahgraham]

Grateful for any pointers on how that might be done.

@colin-grant-work - I missed this question when it was originally asked. If you have memory maps properly populated you can check them before doing the write.

However the use of allowPartial without a corresponding supports flag is a bit annoying. Rather than spending too much time trying to implement this, I would check that the front ends of interest (Theia?) uses allowPartial all the time and then file an improvement on DAP to make allowPartial have to be true unless supportNonPartial writes is true.

Perhaps @paul-marechal has other ideas.

[colin-grant-work]

@jonahgraham, thanks for your input.

I would check that the front ends of interest (Theia?) uses allowPartial all the time and then file an improvement on DAP to make allowPartial have to be true unless supportNonPartial writes is true.

At the moment, the client (or at least the one I'm interested in) is making requests for single bytes at a time, so allowPartial wouldn't really make sense, and it may not be necessary to support it at the moment. @federicobozzini, since you're working on the same client, you may have an opinion about whether it's desirable to implement the machinery for more sophisticated writeMemory requests at the moment. What are your thoughts?

[paul-marechal]

rerun tests

[paul-marechal]

@jonahgraham

However the use of allowPartial without a corresponding supports flag is a bit annoying.

Agreed. But my issue is that I really don't know GDB enough to properly have an opinion... From some GDB/MI doc -data-write-memory-bytes doesn't even have a return value, so I'd be curious to know what GDB does if memory if partially not writable?

Rather than spending too much time trying to implement this, I would check that the front ends of interest (Theia?) uses allowPartial all the time and then file an improvement on DAP to make allowPartial have to be true unless supportNonPartial writes is true.

Like Colin I think we can get away with not supporting the allowPartial flag at the moment...

Regarding suggesting changes to the DAP, the question is what's actually an issue right now? I don't understand why partialWrite should be true by default? Is there anything in the current DAP specification with regards to writeMemory that cannot be done by GDB? I am genuinely asking as I really lack experience with GDB and inspecting memory...

What could be temporarily done is to figure out which regime GDB operates in when using -data-write-memory-bytes (does it partially writes or not?) and then require the allowPartial flag to be set accordingly by clients making requests, and finally just throw an error if the client calls writeMemory in an unsupported regime?

[paul-marechal]

But it is also legitimate to postpone this feature's implementation until there is a public client that uses the API... Maybe by then we'll have more concrete use-cases of how the client wants to write to memory, cross check with what can be done through GDB and finally potentially propose improvements to the DAP?

[paul-marechal]

@jonahgraham also it seems like the build/test bot won't listen to me... Do I need to write something else to trigger it?

rerun tests

[jonahgraham]

@jonahgraham also it seems like the build/test bot won't listen to me... Do I need to write something else to trigger it?

Did you change user names recently? I added paul-marechal to the admin list https://ci.eclipse.org/cdt/view/npm%20builds/job/cdt-gdb-adapter-verify/jobConfigHistory/showDiffFiles?timestamp1=2020-12-02_22-32-40&timestamp2=2021-09-09_17-41-00

[jonahgraham]

PS - I will answer other stuff later.

[colin-grant-work]

But it is also legitimate to postpone this feature's implementation until there is a public client that uses the API...

Bit of chicken and egg. Somebody's got to do it before someone can decide to use it. And then maybe if they think it should be done better, they'll contribute an improvement :-).

[jonahgraham]

@jonahgraham

However the use of allowPartial without a corresponding supports flag is a bit annoying.

Agreed. But my issue is that I really don't know GDB enough to properly have an opinion... From some GDB/MI doc -data-write-memory-bytes doesn't even have a return value

GDB will return an error if it fails to write (see General Design for some wooly words on return codes).

, so I'd be curious to know what GDB does if memory if partially not writable?

A simple write command that contains memory that is not writeable will write up until the error IIRC. Of course something in the stack needs to know it isn't writeable. This can be done with memory regions* or in the remote protocol's M command

* (useful commands in there to test behaviour) - the memory map can be filled from the target memory map format

GDB will not return how many of the bytes succeeded (enhancement request to GDB?). So, for example if you write a 32-bit word that was only partially writeable, GDB will return an error, but some of the bytes may have been written.

Rather than spending too much time trying to implement this, I would check that the front ends of interest (Theia?) uses allowPartial all the time and then file an improvement on DAP to make allowPartial have to be true unless supportNonPartial writes is true.

Like Colin I think we can get away with not supporting the allowPartial flag at the moment...

On further consideration I agree that allowPartial can be left for later and fail the whole WriteMemory request if it fails during the write. That will leave the target in an inconsistent state so the front end should re-read memory to ensure it is up to date. Note the GDB manual says something similar:

There’s no guarantee that whenever an MI command reports an error, GDB or the target are in any specific state, and especially, the state is not reverted to the state before the MI command was processed. Therefore, whenever an MI command results in an error, we recommend that the frontend refreshes all the information shown in the user interface.

Regarding suggesting changes to the DAP, the question is what's actually an issue right now? I don't understand why partialWrite should be true by default? Is there anything in the current DAP specification with regards to writeMemory that cannot be done by GDB? I am genuinely asking as I really lack experience with GDB and inspecting memory...

When I said that I forgot that the error message from GDB is lacking enough information to know how much of the write succeeded. i.e. the write with GDB will be partial, but not enough information is available to return how much of the partial result was written.

What could be temporarily done is to figure out which regime GDB operates in when using -data-write-memory-bytes (does it partially writes or not?) and then require the allowPartial flag to be set accordingly by clients making requests, and finally just throw an error if the client calls writeMemory in an unsupported regime?

GDB does not match either regime. As I mentioned above, the writes are all partial, but without the required return info.

[jonahgraham]

GDB does not match either regime. As I mentioned above, the writes are all partial, but without the required return info.

Of course, using memory maps or splitting up writes, can make it match either regime. But lets avoid splitting up writes if possible because we probably should trust either the front end or the memory map to control how wide a word can be written at any point.

From earlier comment:

[...] is making requests for single bytes at a time [...]

This works for memory on devices that allow byte writing, but for writes in some architectures and for things like memory mapped registers, aligned word writes are important.

[colin-grant-work]

[...] is making requests for single bytes at a time [...]

This works for memory on devices that allow byte writing, but for writes in some architectures and for things like memory mapped registers, aligned word writes are important.

I should clarify that 'single bytes at a time' refers here to 'frontend bytes' - the memory inspector allows the user to configure the word size they're interested in and uses that as the unit for sending write memory requests. It's a bit of a hack, since the 'correct' behavior obviously depends on the architecture and isn't something that can be configured at runtime, but it seemed like the most flexible way to handle it for now, and adopters of the memory inspector code can fix the word size or switch to an architecture-based configuration, if they know what they're targeting.

On further consideration I agree that allowPartial can be left for later and fail the whole WriteMemory request if it fails during the write. That will leave the target in an inconsistent state so the front end should re-read memory to ensure it is up to date. Note the GDB manual says something similar:

I previously had the DA send a stop event when it was done writing memory. I've removed that, but it sounds like it might be better to bring it back, since the stop event is a fairly universal way of telling the frontend to update itself? Alternatively, we could just hope that frontends are smart enough to send the read memory request and then update themselves regardless of the response they get back?

[federicobozzini]

@colin-grant-work At the moment we are ignoring the allowPartial option for the writeMemory operation. If more bytes are targeted at the same time and there is a failure we just "rollback" the changes and consider the operation to have failed.

[jonahgraham]

@eclipse-cdt-bot run tests

[jonahgraham]

I think we have come to the conclusion that the patch set as submitted is a good solution. The request will return error or success regardless of partial and I think that is ok enough.

I appreciate the base64 tests, but any change to have a test that actually writes memory?

[colin-grant-work]

@jonahgraham, I'm happy to write a test to actually see if it works, of course. I haven't looked in detail at the actual functional tests, but I imagine that the readMemory tests can serve as a reasonable guide for how to proceed?

[jonahgraham]

@jonahgraham, I'm happy to write a test to actually see if it works, of course. I haven't looked in detail at the actual functional tests, but I imagine that the readMemory tests can serve as a reasonable guide for how to proceed?

Yes. Don't hesitate to ask followup questions.

To test that writes fail, using https://sourceware.org/gdb/current/onlinedocs/gdb/Memory-Region-Attributes.html is probably an easy way to make GDB return an error.

If you have problems setting up your local system to run tests, you can run them on docker. There is some docs here on that https://github.com/eclipse-cdt/cdt-gdb-adapter/blob/master/src/integration-tests/README.md#running-the-tests-using-docker

[colin-grant-work]

@jonahgraham, thanks for the pointer to memory regions as a way to generate a failure. Is there a convenient way to run GDB-console commands from inside tests, something like mem 10 20 ro to make a particular region read-only temporarily? I don't see a corresponding MI command, so it doesn't seem it can be done over the MI channel.

[jonahgraham]

@colin-grant-work initCommands (part of LaunchRequestArguments) can be used to send arbitrary commands to GDB at the beginning of the session. It may be too early for your use case. When doing "target" launches (instead of native) there are also connectCommands and preRunCommands (part of TargetLaunchRequestArguments)

All commands can be sent over MI (the reverse is not true). So in MI mode you can just do mem 10 20 ro. However it is generally better form to wrap them in -interpreter-exec like this -interpreter-exec console "mem 10 20 ro" (see docs)

As for running such arbitrary commands in tests - that is a useful thing to add, I would add a new custom command to allow arbitrary commands to be sent. I would scope it in a test like name to discourage it being part of the API or anything. e.g. like this, but perhaps a better name:

    /**
     * Handle requests not defined in the debug adapter protocol.
     */
    protected customRequest(command: string, response: DebugProtocol.Response, args: any): void {
        if (command === 'cdt-gdb-adapter/test-arbitrary-command') {
            this.testArbitraryCommandRequest(response as ArbitraryCommandResponse, args);
        } else if (command === 'cdt-gdb-adapter/Memory') {
             this.memoryRequest(response as MemoryResponse, args);
        } else {
            return super.customRequest(command, response, args);
        }
    }

[colin-grant-work]

@jonahgraham, thanks! Plenty to chew on there - very helpful.

[colin-grant-work]

@jonahgraham, I think it's ready to be looked at again. I've written a couple of tests. One should simply succeed; the other sets memory to read-only before trying to write, and it expects failure. Let me know if you'd like me to test other scenarios (partial read-only, e.g.).

[jonahgraham]

@eclipse-cdt-bot ok to test

[jonahgraham]

@eclipse-cdt-bot run tests

[jonahgraham]

New tests are running and passing https://ci.eclipse.org/cdt/job/cdt-gdb-adapter-verify/139/testReport/.._dist_integration-tests_/Memory%20Test%20Suite/