Add new Memory Event to refresh an address range

[yannickowow]

Hi,
I open this request to have the ability to throw an event if a range of address has been updated.
Here is my first idea:

• An event will inform a starting address that has been updated
• This event will contain the new data
• If this content is longer and filled by zeroes, an optional count can be used.

Thanks

interface MemoryEvent extends Event {
  event: 'memory';

  body: {
    /* Start address where memory has been updated */
    address: string;

    /**
     * The bytes read from memory, encoded using base64.
     */
    data: string;
    /* Number of bytes updated
     * If not specified, calculated using data length. 
     * If specified, fulfill with zeroes until count matches
     */
    count?: number;

  };
}

[connor4312]

The invalidated event is what we use in other areas for this, which will trigger the UI to refresh data as needed. Would that work for this use case?

I think we would also implicitly invalidate memory if the source of the memory being read (e.g. variables) changed

[yannickowow]

Hi,
It should work I guess. However, how to be sure that, using a Memory Viewer for example, we highlight the correct value in a given context ? Without infos about addresses, we only can do a diff between old and new memory content ?

EDIT: I was using GDB "Memory Changed" event as a reference:
=memory-changed,thread-group=id,addr=addr,len=len[,type="code"]

A memory changed for a given range should trigger a refresh. It should be perfect if we can Invalidate a given address and length

[weinand]

@yannickowow thanks for the proposal

Introducing a new event with specific properties for Memory ranges makes sense and corresponds to the other DAP events for Modules, Threads, LoadedSource, and Process.

I do not recommend to use the Invalidated event for this, because that event should be used to signal that some state in the debug adapter has changed (as opposed to state in the debuggee). An example for a debug adapter state change is a change of the format of variables.

@gregg-miskelly @yannickowow @connor4312
If there are no objections, I'll proceed updating the DAP schema.

[yannickowow]

Another possibility is to send an event with address and count (which will be required) but without data. And, after this event, a GUI need to send a readMemoryRequest with these attributes.
Since we tried to remove data from writeMemoryRequest do we need to remove data here as well for consistency ?

[gregg-miskelly]

I am assuming the primary scenario here something like: the client has a memory window, and the something decides make an operation that writes to memory (for example, the user modifies a local variable) and we want to let the memory window know that it should refresh the given address range if it was being shown. Is that correct?

Assuming so:

I think I would agree with @yannick-mamudo that we probably want to remove data since conceivably there could be a lot of these, and the client probably mostly doesn't care.

If a DA knows that some memory changed, but doesn't know what memory that was (ex: an assignment where the underlying debugger doesn't easily expose what addresses were modified), should we use an Invalidated event? Or should we allow address to be optional?

[puremourning]

I don’t think clients have any way to ‘interpret’ memoryReferences. They are opaque to clients so even if server returned an address and count, client would not have any way to map that to something it knows about.

Agree that Invalidated seems wrong for this, but in either case it requires the DA to remember (somehow?) that client has a memory window open for a given range. Unless I’m missing something, without this wouldn’t this event be really noisy… like every write to memory triggering an event?

if the intention is to only raise these events when the client cares, then there needs to be some synchronisation for client to request and then cancel these updates? Am I missing something obvious?

[yannickowow]

I don’t think clients have any way to ‘interpret’ memoryReferences. They are opaque to clients so even if server returned an address and count, client would not have any way to map that to something it knows about.

Yes you're right. But this event should return an address, and client interprets this as decimal or hexadecimal if it starts by 0x or not. Using memoryReferences on a request, it "resolves" to an address on response. (If I understand correctly)
See the ReadMemory description: https://microsoft.github.io/debug-adapter-protocol/specification#Requests_ReadMemory

Agree that Invalidated seems wrong for this, but in either case it requires the DA to remember (somehow?) that client has a memory window open for a given range. Unless I’m missing something, without this wouldn’t this event be really noisy… like every write to memory triggering an event?

If a client does not have a Memory Window opened, event will be ignored. Else, event should be raised by a Debug Adapter on memory update. This behaviour has nothing to do w/ client and should be handled with care from a DA.

That's why I suppose that we need to remove data from my previous feature request, because if window is closed, this event can be quite heavy. In the second hand, you might not call for a readMemoryRequest.

[puremourning]

a client does not have a Memory Window opened, event will be ignored
should be handled with care from a DA
In the second hand, you might not call for a readMemoryRequest.

I still think there's a pathological case here. if the server is to produce an event on any change to any memory that ever had a readMemeoryRequest made for it, then on a long debug session, can't that combine to a lot of redundant chatter.

Example:

• start a debug session
• open memory window and readMemory (range A - B)
• close "memory window" in client
• open memory window and readMemory (range C - D)
• close "memory window" in client
• ... etc.
• open memory window and readMemory (range Y - Z)
• close "memory window" in client
• At this point, is the debug adapter sending updates for all writes to memory in range A-Z, even though the client does not have any memory window open?

I agree that removing data reduces the chatter somewhat, but it's still potentially significant work for both client and adapter.

Using memoryReferences on a request, it "resolves" to an address on response

I didn't realise that was the intention of the address result in ReadMemoryResponse, but I guess it's workable.

[yannickowow]

a client does not have a Memory Window opened, event will be ignored
should be handled with care from a DA
In the second hand, you might not call for a readMemoryRequest.

I still think there's a pathological case here. if the server is to produce an event on any change to any memory that ever had a readMemeoryRequest made for it, then on a long debug session, can't that combine to a lot of redundant chatter.

I might explained it incorrectly or you missed the point.

A Debug Adapter will not be aware of any Memory Window opened nor previous requests done.
You will send a Memory Event if you've done a Set Variable / Set Expression event in a given address, or because any load has modified memory ranges/content for example. If your Memory Window is opened, you'll refresh previous content, else, you will ignore this event.

These sort of events are already supported by GDB, using "memory-changed" MI Event. Your current adapter will only need to translate between GDB and DAP response.

[puremourning]

I’m still utterly confused. What I would suggest is that if/when this is included in the specification that a clear set of use cases and expected client and server behaviours are included. I don’t think that just referencing some gdb behaviour is sufficient for that. Maybe a message flow diagram etc.

Certainly I would only implement this in my client if the there are clear bounds on the frequency of this event and that servers are well directed in how to implement it.

Happy to discuss further ofc.

[connor4312]

Another possibility is to send an event with address and count (which will be required) but without data. And, after this event, a GUI need to send a readMemoryRequest with these attributes.

I think this would solve your issue, @puremourning. That removes the notion of "windows" and provides a lightweight way to indicate when the underlying memory changes, which could span across opened "windows". The lifecycle of that memory would be tied to the Variable it's associated with, and notifications would be sent using the same behavior as "invalidated".

Memory references can also be returned from evaluation and disassembly--I'm not sure whether a DA would ever want to indicate mutation on those.

[weinand]

I think we've achieved agreement about the event's properties: the data of the original proposal is gone.
In addition I've tried to come up with an event description that captures the discussion from above.

/**
  * This event indicates that some memory has been updated.
  * Clients typically react to the event by issuing a `readMemory` request if they show the memory range
  * in the UI, otherwise they ignore the event.
  * Debug adapters can use this event to indicate that the contents of a memory range has changed
  * due to some other DAP request like `setVariable` or `setExpression`.
  * Debug adapters are not expected to use this event to indicate each and every memory change of a
  * running program, because that information is typically not available from debuggers and it would flood
  * clients with too many events.
  */
interface MemoryEvent extends Event {
  event: 'memory';

  body: {
    /* Start address where memory has been updated.
     * Treated as a hex value if prefixed with '0x', or as a decimal value otherwise. 
     **/
    address: string;

    /* Number of bytes updated.
     */
    count: number;
  };
}

Opinions?

[yannickowow]

Sounds great to me.
However, what should be the behaviour of this event if count is undefined ? Should we interpreit it as count = 1 ?
Since we removed data from the event, count argument should be mandatory instead ?

[weinand]

@annickowow oops, copy-paste error... thanks for spotting this. I've fixed this in my comment above.

[puremourning]

Looking better, thanks!

I wonder if we can slightly modify the wording to be clear about how the client uses this event to issue a 'readMemory' request.

It says:

• Clients typically react to the event by issuing a readMemory request if they show the memory range

A readMemory request contains a memoryReference: string; not an address. From above, I think the expectation is that clients will:

• issue a readMemory request with a memoryReference supplied from the variables request (or similar)
• Note the returned address, and use the original count and unreadableBytes to determine the range of memory that's been displayed.
• Listen to any MemoryEvent and and re-issue any readMemory request with the original memoryReference whose displayed data overlap the updated range.

Therefore, perhaps this wording (or similar), changes in bold:

• Clients typically react to the event by re-issuing a readMemory request if the updated memory range overlaps the displayed range ...

[connor4312]

Something interesting about the previous Read/WriteMemory requests is that they don't imply that all memory is in single contiguous range, and therefore don't require that addresses be unique. Since they're always give using an adapter-controlled MemoryReference, my plan for js-debug was to use that as effectively (or literally) the variable reference, and then return all memory as starting at address 0 -- since V8 does not tell us physically where binary types are stored, and for all I know might move them around arbitrarily.

However, since refresh memory only has an address, now this implies or requires that addresses for read memory are unique. For JavaScript (and I'm guessing several other interpreted languages) this means that I effectively have to write an "allocator" to slot binary data I read from the runtime into arbitrary places in $\mathbb{Z}^+$.

I think using the memoryReference + offset like we do ReadMemory and WriteMemory makes sense from a consistency standpoint if nothing else, and we should clarify whether clients are expected to treat individual memoryReferences as being part of a single continuous range.

[weinand]

@connor4312 good point about the "parameter consistency".
Based on your feedback I've replaced the address by a memoryReference/offset pair and I tried to be more precise about how individual memoryReferences relate to each other.

@puremourning I've tried to fold your proposed wording into the comment.

/**
  * This event indicates that some memory range has been updated.
  * Clients typically react to the event by re-issuing a readMemory request if they show the memory
  * identified by the memoryReference and if the updated memory range overlaps the displayed range.
  * Clients should not make assumptions how individual memoryReferences relate to each other,
  * so they should not assume that they are part of a single continuous address range and might overlap.
  * Debug adapters can use this event to indicate that the contents of a memory range has changed
  * due to some other DAP request like `setVariable` or `setExpression`.
  * Debug adapters are not expected to use this event to indicate each and every memory change of a
  * running program, because that information is typically not available from debuggers and it would flood
  * clients with too many events.
  */
interface MemoryEvent extends Event {
  event: 'memory';

  body: {
    /**
     * Memory reference of a memory range that has been updated.
     */
    memoryReference: string;

    /**
     * Starting offset in bytes where memory has been updated. Can be negative.
     */
    offset: number;

    /**
     * Number of bytes updated.
     */
    count: number;
  };
}

[connor4312]

Looks great!

[weinand]

Here is the final generated spec in TypeScript:

/** Arguments for 'initialize' request. */
export interface InitializeRequestArguments {

  // ...

  /** Client supports the memory event. */
  supportsMemoryEvent?: boolean;
}


/** Event message for 'memory' event type.
  This event indicates that some memory range has been updated. It should only be sent if the debug adapter
  has received a value true for the `supportsMemoryEvent` capability of the `initialize` request.
  Clients typically react to the event by re-issuing a `readMemory` request if they show the memory
  identified by the `memoryReference` and if the updated memory range overlaps the displayed range.
  Clients should not make assumptions how individual memory references relate to each other, so they
  should not assume that they are part of a single continuous address range and might overlap.
  Debug adapters can use this event to indicate that the contents of a memory range has changed due
  to some other DAP request like `setVariable` or `setExpression`. Debug adapters are not expected
  to emit this event for each and every memory change of a running program, because that information
  is typically not available from debuggers and it would flood clients with too many events.
*/
export interface MemoryEvent extends Event {
  // event: 'memory';
  body: {
    /** Memory reference of a memory range that has been updated. */
    memoryReference: string;
    /** Starting offset in bytes where memory has been updated. Can be negative. */
    offset: number;
    /** Number of bytes updated. */
    count: number;
  };
}