sending memory reference as part of evaluate request response

[omarArm]

Should implement #539

Changes:

1. Whenever a response for the evaluateRequest is finally getting prepared, the adapter would send an MI command retrieving the address of the expression (if available)
2. If address is available, use it as a memory reference.

The perks of implementing such a feature is that vscode will then set the canReadMemory context. As mentioned in the original issue linked above

[cwalther]

Looks like a useful addition! I haven’t tried it, but a few comments from reading the code.

[omarArm]

@cwalther I squashed a few commits and CI is now working, could you please do a final review and merge if it's acceptable to you?

[cwalther]

This point from one of my first comments is still open:

Also, the DAP specification says

For pointer type eval results, this is generally a reference to the memory address contained in the pointer.

so in that case we shouldn’t apply the & operator but use the value directly. Can we somehow determine whether the expression is pointer-typed?

I leave it to you (or @jreineckearm if he is around) whether to fix this here or keep it as a known deficiency to be fixed later. If “later” is not in the near future, an issue should be opened for it. Let me know what you prefer (or if you disagree entirely).

Apart from that, the code looks good to me, and I have now tried it and it appears to work as intended. 👍

The commits are still a bit of a mess: e.g. the last one includes a change in GDBDebugSessionBase.ts that has nothing to do with what the commit message says.

There are two changes here that I would prefer to keep in separate commits because they are unrelated to the topic of the PR:

• the handling of supportsMemoryEvent (part of 26f523d) (that is the solution to Handle memory notifications #533 (review))
• the fixing of the flaky test in a4fccab.

The rest of the changes I would prefer to see squashed into one commit, because some of the later commits undo or amend changes done in earlier ones, and we don’t need to have this back-and-forth recorded. Let me know if you disagree.

Could you bring things into such a shape of three commits using an interactive rebase (ask if you need help)?

[omarArm]

Oh, sorry I must have missed it. I am not sure what would be the best way to implement it yet. Therefore, I would rather have it in a separate PR. That being said, I created an issue for it. IMO, we should create issues for any future changes even if they are planned to be done in the near future.

I cleaned up the commits as requested as well

[cwalther]

Thanks for opening the issue!

The commits are not quite cleaned up yet, I’m afraid. Commits 2 and 3 still contain changes that belong in commit 1, e.g. the catch (err) → catch part. (Documentation of how to split commits)

[cwalther]

It still doesn’t seem quite right (removals in 424db4e?) and has somehow become four commits now, but I don’t have time for a closer look right now, will continue tomorrow.

[omarArm]

@cwalther thanks for the feedback, could you please check now?

[cwalther]

Sorry, still not good. See inline comments on commit “checking if client supports memory events”.

Here is what I would do to clean it up:

Name a reference to the previous state for more convenient access

git branch addMemoryReferenceToEvaluateRequest-backup

Split the commit that contains too many changes into a temporary one with the extraneous changes and the real one with the intended changes (following instructions)

git rebase -i origin/main
# change second commit from "pick" to "e"
git reset HEAD^
git add src/integration-tests/evaluate.spec.ts
git add -p src/integration-tests/debugClient.ts
# e
# remove the supportsMemoryEvent line but keep the supportsMemoryReferences one
git commit -m "temp"
git add -u
git commit -C addMemoryReferenceToEvaluateRequest^
git rebase --continue

Squash the temporary commit into its parent

git rebase -i origin/main
# change temp "temp" commit from "pick" to "f"

Check whether the result is as expected

git diff addMemoryReferenceToEvaluateRequest-backup addMemoryReferenceToEvaluateRequest
# diff should be empty
git range-diff addMemoryReferenceToEvaluateRequest-backup...addMemoryReferenceToEvaluateRequest
# should show desired modifications to the first two commits, no change to commit messages
git branch -D addMemoryReferenceToEvaluateRequest-backup

Use git status and git diff --staged anywhere in between to see what you are doing.

Does that help?

[omarArm]

@cwalther please have a look now. This was actually a really fun git exercise for me. Thank you for nudging me towards it

[cwalther]

Thank you, now it’s looking good!

Glad the nudging helped. I can’t spend that effort every time, but if it helps people get more proficient and need less nudging in the future then it’s well spent.

[jreineckearm]

Thanks for the review, @cwalther ! And apologies for chiming in only now. Was on holiday again, but should be back now for a bit.

I leave it to you (or @jreineckearm if he is around) whether to fix this here or keep it as a known deficiency to be fixed later. If “later” is not in the near future, an issue should be opened for it. Let me know what you prefer (or if you disagree entirely).

@omarArm , let's have a look at what can be done. I believe there was some heuristics to determine a pointer in other parts of the code. But I also remember that those parts needed review for corner cases.

I see you have already raised #547 . Thanks!

[jreineckearm]

Nit: probably one of the two variables would have been enough (undefined for memoryReferenceResult meaning no memory reference). Maybe next time we touch this area.