Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AGS 4: Watch script variables at runtime #2430

Draft
wants to merge 20 commits into
base: ags4
Choose a base branch
from

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented May 19, 2024

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**

ags4-memorywatch-draft4

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...

  1. Global variables need only their address in script's global memory, name and type.
  2. Imported global variables are marked as such at TOC generation, and their actual address is resolved on the fly when retrieving variable value.
  3. 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. 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. 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. 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 Editor: remember breakpoints in workspace state #2384).
  15. PARTIAL Minor GUI improvements may be wanted, but these may addressed separately afterwards.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented May 30, 2024

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
Copy link
Member

ericoporto commented Jun 2, 2024

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));
}

image

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

image

@ericoporto
Copy link
Member

ericoporto commented Jun 2, 2024

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
Copy link
Contributor Author

ivan-mogilko commented Jun 2, 2024

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
Copy link
Contributor Author

ivan-mogilko commented Jun 2, 2024

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
Copy link
Member

ericoporto commented Jun 2, 2024

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
Copy link
Contributor Author

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
Copy link
Member

ericoporto commented Jun 2, 2024

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
Copy link
Contributor Author

Alright, should look into these separately...

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Jun 2, 2024

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
Copy link
Contributor Author

ivan-mogilko commented Jun 3, 2024

@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
Copy link
Member

ericoporto commented Jun 4, 2024

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

This is just a cheap way of marking extensions as "optional", which may be skipped without loosing actual game feature.
* F2 starts editing selected item.
* Ensure there's always 1 empty item in the end, which user may click to edit or use F2 on.
* Automatically remove multiple trailing empty items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ags 4 related to the ags4 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants