AGS 4: Watch script variables at runtime

[ivan-mogilko]

This implements an mechanism for retrieving contents of a script memory via a debugger communication interface. Adds the "Watch variables" panel in the Editor, which lets user type in variable names, and receive current values.

The list of variables currently is worked with using a context menu that has Add/Remove/Clear commands. You may also edit variable's name by single clicking on a existing item in the list.

A tall screenshot under the spoiler: **CLICK HERE**


What works:

• Global variables, including ones imported from other scripts, plugins or the engine itself;
• Local variables, including function parameters. Their lifescope should be detected correctly.
• Resolves any chain of struct members and pointers access.
• Accessing array elements using [n] notation.
• Managed pointers are displayed as handle values; this is not entirely useful, but lets to see if they are null, and compare among themselves.
• Special hardcoded treatment for managed String types which resolves them to the internal char buffer.

What does not work:

1. Attributes (aka properties). Attributes in AGS are secretly pairs of get/set functions. Reading an attribute's value means calling a getter function. There are two major issues with that currently:
   • First, such call may have side effects, including but not limited to creation of a managed object or returning a managed object with incrementing its ref count;
   • Second, if an attribute is user-defined, then this would require to run a script. But engine currently does not support running more scripts while in a "break" state. So, this is something left for the future consideration.
2. Displaying full contents of a struct's or managed struct's object (or array, fwiw) after typing its name. I suppose that is theoretically possible, but will not do this at the first stage. For now you'll have to type each struct's member separately.

How this is achieved

1. While compiling the script, compiler (optionally) builds a "table of contents" of all script's variables, global and local, and own functions (aka ScriptTOC). This table of contents is saved as a (optional) compiled script's extension, similarly to the previously added RTTI table. For the moment I enabled this for Debug build mode only.
2. Implemented a new debugger command: "GETVAR", which requests value of a script variable from the running engine. command has an argument containing variable's name, or chain of access of any complexity, e.g. mystruct.member_field.internal_field etc. In order to process this command and correctly resolve all memory addresses engine requires "table of contents" from the current script and RTTI. If these are not available, then it will fail. If they are present then engine builds a memory access instruction, and then processes this instruction as a separate operation. This is when it gets to the final memory address, reads a value of certain size, and then ~formats it into a string, depending on its type.~ encodes value's bytes into a base64 string, except when value is already a string, and sends back to debugger.
3. Implemented a new debugger command "RECVVAR" which is sent by the engine back to debugger, with the previous query result. "GETVAR" and "RECVVAR" have a arbitrary generated "request id" field, which lets to connect them together (in case of multiple or parallel commands).
4. On the Editor's side, added a "Watch variables" panel, which is basically a list with 3 columns: variable name, type and value. User adds entries (I added a simple context menu for now) and inputs variable names. Editor sends a "GETVAR" command, receives "RECVVAR" command, and displays a value in the corresponding item. Editor sends these commands in following cases:
   • When a breakpoint is hit - all entries are updated in a batch;
   • Per step during step-by-step execution
   • When a new item is added, and run script is currently in break.

Other notes...

6. Global variables need only their address in script's global memory, name and type.
7. Imported global variables are marked as such at TOC generation, and their actual address is resolved on the fly when retrieving variable value.
8. Local variables are the trickiest. For them the TOC records their lifetime scope, using bytecode positions: pos at which the variable is allocated, and pos at which it's removed from the stack. When the engine is resolving these it searches for the vars that are allocated prior to the current execution pos, and not yet removed. Additionally, this search is restricted by the current function's scope (also recorded in bytecode positions).

Design issues

1. Script TOC does not have to be saved as a compiled script extension. Unlike RTTI, it's not used for any script operations, only for debugging. Therefore, as an alternative, it may be saved in a separate file.
2. I consider the way TOC lists imports a temporary solution. The reason why I need to add them at all is because we need to know import's type in order to resolve it properly. Existing list of imports in script data does not have anything but names, and there's no existing reliable way to define their types at runtime at linking stage (exports may come from plugins too, for example). So, for a moment the easiest way to record their types is at compilation stage, using their declarations. But the problem is that they make a lot of duplicate entries in script TOCs. And also ScriptTOC is intended to serve an index of things found inside script, so presence of imported declarations there looks like a logical flaw. I suppose that their type info could be added to existing list of imports in the script data instead.
3. [DONE] Value formatting is currently done on the engine's side, but it may be better to leave this for debugger (editor). In such case engine should only return memory contents packed in certain format (base64).

Backporting to AGS 3

If there's a wish to backport this watch feature to ags3, the RTTI generation must also be backported. Any RTTI-based features (in scripting) may be omitted, but RTTI table itself has to be present to be able to access nested fields and recognize types.

Known problems and TODOs

1. FIXED ~Parsing array access is done inconveniently in "query variable" code, and should be rewritten.~
2. PARTIALLY HANDLED No buffer size check or array bounds check is currently done (although it's been planned), so it's possible to read past the valid data using a high enough array index.
3. FIXED ~Plain fields of builtin structs may be read incorrectly, their access has to be rewritten.~
4. FIXED ~TOCs compiled by an old compiler report regular array of managed pointers as a "managed pointer" itself, which results in unexpected values when typing only such array's name (without element index).~
5. FIXED ~With TOCs compiled by a new compiler there's a small mistake in local variable scope, which results in wrong local variable results at the first line inside a nested scope and the first line right after a nested scope.~
6. Multidimensional arrays may have issues (need testing); also I have suspicion that their types may not be correctly tagged in RTTI (must investigate). Also - mixed arrays, like static array of dynamic arrays which is recently supported by the new compiler.
7. Support "nested expression" (variable references) as an array index in []; this means that multiple variables have to be resolved in a sequence (or recursively).
8. FIXED ~Null pointers return no value instead of "0".~
9. DONE ~String values should perhaps be reported in double quotes, in order to distinguish them from any special results, like "0" for a null string, or some service messages ("invalid variable" etc).~
10. DONE ~Char variables should perhaps have both numeric value and char value shown in a result string.~
11. DONE ~Return errors from the engine, instead of empty value string on failure.~
12. FIXED (presumably) ~There's a potential opportunity of missing an engine's reply in the editor; that's because listening is currently coded a bit incorrectly.~
13. DONE ~In the Editor, "Output" panel is always sent to front whenever a compilation is started. Possibly it should bring "Watch variables" in front as soon as a test run is launched.~
14. It would be convenient to save last watched variables in a workspace settings (see also #2384).
15. PARTIAL ~Minor GUI improvements may be wanted, but these may addressed separately afterwards.~

[ivan-mogilko]

I believe that this feature now works for the most part. What remains are looking for nuances and edge cases, and usability improvements.

The biggest next change which I have in todo here is to move return value formatting to the Editor side, and make engine return plain array of bytes, accompanied by a type hint. Possibly encoded in base64?

EDIT: Another thing that could be supported here is having "nested expression" (variable reference) as an array index in [], which means multiple variables have to be resolved in a sequence (or recursively etc). I believe that this is already doable with the existing code.

[ericoporto]

I can't figure it out how to test this, do I have to do something to make the watch variable panel not be empty? Edit: uhm... I tried to manually add a variable in my project but got and error...

An exception 0xC0000005 occurred in ACWIN.EXE at EIP = 0x004AD2FA; program pointer is +1004, engine version 4.0.0.5, gtags (0,0)

OK, trying to attach a read in different places always gives me this error...

Trying in a much simpler project in my String Split test I get a crash with exception but it's a different one

An exception 0xC0000005 occurred in ACWIN.EXE at EIP = 0x004AD2FA; program pointer is +6, engine version 4.0.0.5, gtags (0,0)

function room_AfterFadeIn()
{
  String str = "this is a string";
  String spl[] = str.Split(" ");
  Label1.Text = "";
  for(int i=0; i<spl.Length; i++)
  {
    System.Log(eLogInfo, "'%s'",  spl[i]);
    Label1.Text = Label1.Text.Append(String.Format("'%s' , ",  spl[i]));
  }
  String a[];
  String joined = String.Join(a,  ",");
  System.Log(eLogInfo, "'%s'",  joined);
  Label1.Text = Label1.Text.Append(String.Format("\nJoined: '%s'",  joined));
}

OK, figured if I comment any GUI stuff I don't get crashes

[ericoporto]

What about making the local variables dynamically watcheable and having global variables require explicitly being watched? Plus some right click watch selection in the script editor.

And perhaps an array could show the first three or five elements and a ellipsis to signal it goes longer.

[ivan-mogilko]

Edit: uhm... I tried to manually add a variable in my project but got and error...

Please elaborate, what do you mean by "manually add a variable in project"? There's a context menu that add/remove variables for now, I mentioned this in the ticket. Did you add these variables using a context menu, or some other way? EDIT: added a more clear explanation in the ticket.

What about making the local variables dynamically watcheable and having global variables require explicitly being watched? Plus some right click watch selection in the script editor.

I am not going to do any of these in the first stage. Any automation may be added later if there's a wish for that, and showing all variables should be optional, as I imagine not everyone will like that. There's a number of things to be figured out for this. For instance, at the moment Editor does not know which variables are there, because it does not read this gathered data. So either this data should be available for the Editor too, or it should receive their list from the engine first, using another command.

And perhaps an array could show the first three or five elements and a ellipsis to signal it goes longer.

Whole array contents are not sent at the moment. This may be done though, but I need to figure out how to limit the size of tranferred memory (and also for strings).

[ivan-mogilko]

I tried testing the code you posted above, not commenting anything, and I do not get any exceptions. There are few result formatting mistakes, I will try to fix these soon.

If you are still getting these crashes, could you send me your full test project?

[ericoporto]

In the ags4distfx.zip project, you can add a breakpoint in the _distorter function and try to add any of the local variables and it will crash.

After your changes I don't get crashes anymore in my String Split program (which is simpler), but I still do get crashes in the Sandwalker. In the title room (1) try to throw a breakpoint in init_button_logo function and read any variable there. It should crash right away.

I noticed my Ags4VideoHD.zip project can't hit breakpoints, but I don't know if this was a change here or breakpoints were broken during video playback...

[ivan-mogilko]

I noticed my Ags4VideoHD.zip project can't hit breakpoints, but I don't know if this was a change here or breakpoints were broken during video playback...

Could you also test the latest ags4 branch? I noticed that breakpoints do not work in room scripts with the new compiler, but they work with old compiler, but I don't know since when.

[ericoporto]

Could you also test the latest ags4 branch? I noticed that breakpoints do not work in room scripts with the new compiler, but they work with old compiler, but I don't know since when.

I tested and it doesn't work in the latest ags4 branch too. The compiler doesn't make a difference. Edit: I also think I caught a new issue that the new video api is ignoring the eRepeat and not repeating the video.

[ivan-mogilko]

Alright, should look into these separately...

[ivan-mogilko]

So, I fixed couple of more mistakes. Because of these variables did not show up correctly when one script called another. Unfortunately, I was not able to reproduce any crashes... in my case the panel simply refuse to show any values.

There's something else that I forgot; I probably should make it so that if you switch to another level of a call stack (in the Editor), the context of memory inspection also change. I will try implementing this tomorrow.

[ivan-mogilko]

@ericoporto I fixed a mistake in the old compiler that caused breakpoints to not work in room scripts. But from my tests, there's no problem with the new compiler. I think there could be a confusion if you don't order "Rebuild all files" then the room scripts are not rebuilt when you switch compilers, so it looks like the problem repeats with another compiler. Switching compiler property must force Editor to rebuild all scripts...

[ericoporto]

Hey, I tried this and now all my test games are running correctly and both the breakpoints are being hit as trying to read a variable do not crash AGS anymore. I tried a few of my games and it appears they are all working correctly as is now.

If you want to merge as is and fix further issues later the way it is looks alright - I think one fix I will do after this is merged is to make the right click context menu of the watch panel to not have the "Remove" enabled if nothing is selected or you right click on blank space .

Like logging with the Log Panel the first time I used, this feature is also one thing that I will miss now everytime I try to test something in AGS after having used it, when using a version that doesn't have it. xD

[ivan-mogilko]

So... unless I introduced more new mistakes with the recent changes, the feature is mostly working in its current state.

The following are the remaining concerns and known issues:

1. Data is saved as an (optional) compiled script extension. It is saved only in Debug configuration, but I think that regardless it might be better to have it as a separate optional asset inside a game package. This is technically doable, but would require to change the game compilation workflow.
2. Somewhat related, I have to save declared imports along the "table of contents", although they are not part of script's memory. I must do that because memory inspector has to be aware of the imports type and kind (array, etc). But this is annoying, as this results in a large amount of duplicated entries across game scripts. I think that this could be resolved by saving this data in temporary files, and then merging them together in a single "toc" that mentions only imports. Again, this would require changes to compilation workflow.
3. Multidimensional arrays, I suspect they won't work at the moment, at least not dynamic multidimensional arrays (aka jagged arrays). This is not only this inspector's fault, but also an oversight in RTTI (I think I programmed RTTI before jagged arrays were implemented). I will have to address this separately, and begin with fixing RTTI, which should have means to describe array which has another array as its elements.
4. Memory inspector in the engine currently cannot deduce the size of the dynamic object, resolved from a handle (it cannot tell its type_id at that point... but this is related to how the memory resolving function is written). In practice this means that user may request any element of a dynamic array without boundary checks, and see memory contents past array's end. Possibly this may also lead to a program crash if offset is high enough. (added FIXME note in code)
5. Support nested expressions, such is - arrayname[ integervarname ]. I did not do that yet, being busy with fixing mistakes. May be addressed separately; but I think that would require simply a recursive call to "resolve memory" for each nested item.
6. Make "context" depend on a selected level of callstack. This means that if user switches to another item in a callstack window, the "watch variables" should react and update as if we looked from within a previous function. I think that this is doable, but would require extra work on both engine and editor side. The idea is to save the current callstack level and pass it as a part of the request. Engine should backtrack to the correct instance in the saved callstack. Backtracking script's stack position is what bothers me most though; this should be technically doable, would require saving previous stack pos in the "callstack" array, and maybe something else.
7. Save variables list on exit and restore on launch. May be done as a part of #2384.

I fear that I've lost much of a "steam" I had when starting this, so would rather make a "checkpoint" here, and leave the above for the future.

[ericoporto]

I think these remaining issues could be moved to a new ticket to keep track and this PR merged. Then later once each thing is tackled they could be split in their own issues - I think writing the issues down will require thinking/studying this so I don't think properly writing already would be easy, this is why I would suggest to just copy a paste to a new issue.

One other thing I noticed is if a variable that is a struct (not a managed struct), it's value will be "unknown data", instead of something like "(struct)". Its fields can still be accessed fine, I think it's just that there is nothing to say on the value of a struct itself (not its fields).