search

SNSystems / dexter

DExTer - Debug Experience Tester
MIT License
33 stars 6 forks source link

Remove watch commands, allow arbitrary expressions in Program State #37

Closed SLTozer closed 5 years ago

SLTozer commented 5 years ago

Does not include the actual removal of DexWatch (although it no longer does anything) or of DexWatch from tests, as these changes are trivial but amount to a very large diff which is unnecessary for a practical review of the change.

OCHyams commented 5 years ago

Changes look good.

How does this change interact with annotate-expected-values? annotate-expected-values is used to generate DexExpectWatchValues and DexExpectStepKind commands based on evaluating DexWatch commands at -O0.

We might want to keep DexWatch commands for this purpose?

SLTozer commented 5 years ago

Changes look good.

How does this change interact with annotate-expected-values? annotate-expected-values is used to generate DexExpectWatchValues and DexExpectStepKind commands based on evaluating DexWatch commands at -O0.

We might want to keep DexWatch commands for this purpose?

Good catch; I think annotate-expected-values will need to be changed to add line numbers to the DexExpectWatchValues command, as otherwise the value for the watched expression will be checked at every step of the program, including those where it is out of scope or uninitialized. I think the simple solution is to have it use the earliest line on which an expression is DexWatched as the from_line, and the last as the to_line. Thoughts?

OCHyams commented 5 years ago

I think the simple solution is to have it use the earliest line on which an expression is DexWatched as the from_line, and the last as the to_line. Thoughts?

This, or maybe we should change DexWatch so that it doesn't have to be on each source line. I think it makes sense that DexWatch also takes the from_line, to_line, and on_line args.

I think that would make DexWatch easier to use?

If we did go down this path I reckon it might have to come as a separate patch. What do you think about this?

SLTozer commented 5 years ago

I think there's maybe a little bit too much speculation about what would be a good use for DexWatch, without much empirical evidence either way. I'd say it should be a separate patch, but that work should itself be contingent on a demonstrated need for it (especially considering that we're aiming to put this upstream soon).

OCHyams commented 5 years ago

Seems fair. In which case I think your original suggestion would be fine.

I think the simple solution is to have it use the earliest line on which an expression is DexWatched as the from_line, and the last as the to_line.

@TomWeaver18, @jmorse, thoughts?

TomWeaver18 commented 5 years ago

apart from small nit, LGTM

jmorse commented 5 years ago

LGTM with some minor quibbles asked inline. This is going to result in all expressions used being evaluated in each stack frame, yes? (I think we've been over the impact of that so it should be fine).

SLTozer commented 5 years ago

LGTM with some minor quibbles asked inline. This is going to result in all expressions used being evaluated in each stack frame, yes? (I think we've been over the impact of that so it should be fine).

Yes, that's the method we've gone with. Also, are you referring to quibbles by previous reviewers, or quibbles of your own making? (Asking because I can't see any inline comments added from you)

OCHyams commented 5 years ago

Please can you confirm that the info about DexExpectProgramState is correct in Commands.md in PR #39?