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

Add special case for MIParser to parse 'objects' with values but no keys #313

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

XingMicrochip
Copy link
Contributor

@XingMicrochip XingMicrochip commented Nov 1, 2023

@jonahgraham This PR is for #311. I added a special case in MIParser to parse objects that are not formatted as name-value pairs. I have tested this change from the IDE in our application on both Windows and Linux and it solved the application hanging issue and the script was attached to the breakpoint fine. I added a test case.

The old object parsing flow is not affected. This change just extends it.

@jonahgraham
Copy link
Contributor

This looks promising. It looks like one of the tests failed, I expect it was an intermittent failure so I restarted the tests. Once tests pass I will have a closer look.

@jonahgraham
Copy link
Contributor

sorry, I didn't get to looking at this today, I'll try tomorrow.

Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change works for the inputs provided, but falls over fairly easily if anything more complicated is put in the "script" block. This is because "handleString" can't be used to parse a "c-string" which is a C-style string.

Consider an input where the quoted string contains control characters, like }:

            '+message,bkpt={number="1",type="breakpoint",thread-groups=["i1"],script={"p }123","p 321","p 789"}}'

Also, it is generally bad form to drop the leading and trailing " without verifying that it is a "-quoted string.

This is why in #311 (comment) I recommended checking if there a a value before doing a key=value parse. e.g. call handleValue() first to see if the next token is a value. Note that the handleValue has a default case that isn't suitable for this use case so some small refactoring will be needed around that.

@XingMicrochip
Copy link
Contributor Author

XingMicrochip commented Nov 2, 2023

I pushed a fix that treats name-value pairs and just values differently that works on the counterexample in your comment above. If there is a " that follows the opening {, then it is just a list of values. Otherwise the object has name-value pairs. This is what my second commit was based on, and I think this implication is correct.

src/MIParser.ts Outdated
c = this.next();
let key = 0;
while (c !== '}') {
let value = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider using handleCString to parse a c-string here as this code still will fail if there is an embedded escaped " in the value. Also handleCString will also unescape the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change.

Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is theoretically possible for other types of messages to show up, in practice I don't know if it happens, so we can defer fixing those cases until we have a concrete example using handleValue instead of just checking if it is a c-string

e.g these cases:

script={["v1","v2"}}
script={{key="value"}}
script={"cstring",key="value"}

@jonahgraham jonahgraham merged commit 71a4f46 into eclipse-cdt-cloud:main Nov 3, 2023
4 checks passed
@jonahgraham
Copy link
Contributor

Thank you @XingMicrochip - I will publish this and an updated cdt-gdb-vscode soon.

jonahgraham added a commit to jonahgraham/cdt-gdb-vscode that referenced this pull request Nov 3, 2023
@jonahgraham
Copy link
Contributor

I have tagged cdt-gdb-vscode and tested the vsix with this:

script:

b main
commands
print "Arrived at main"
end

and this in the launch.json:

      "initCommands": ["source ${workspaceFolder}/script"],

I can see when running that I get the following in the Debug Console:

$1 = "Arrived at main"

@jonahgraham jonahgraham linked an issue Nov 3, 2023 that may be closed by this pull request
@XingMicrochip
Copy link
Contributor Author

XingMicrochip commented Nov 6, 2023

I think v0.0.106 might be broken. I just downloaded it from https://open-vsx.org/api/eclipse-cdt/cdt-gdb-vscode/0.0.106/file/eclipse-cdt.cdt-gdb-vscode-0.0.106.vsix and integrated it into our IDE. We couldn't launch the debugger with the same launch configuration. The last version we used was v0.0.103. Could there be something introduced between v0.0.103 and v0.0.106 that broke the adapter? I also noticed the adapter source files are now bundled. It looks like this now, while it used to be that all the source files are seperated:
image

@jonahgraham
Copy link
Contributor

Yes, the packaging of cdt-gdb-vscode has changed since 0.0.103 - see eclipse-cdt-cloud/cdt-gdb-vscode#96

Can you open a bug in https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/ with what you see. You are not the only one to experience problems with this change, but I cannot reproduce the current problem (I fixed an earlier one)

It looks like this now, while it used to be that all the source files are seperated:

BTW That change is the main part of eclipse-cdt-cloud/cdt-gdb-vscode#96

@XingMicrochip
Copy link
Contributor Author

I created an issue here: eclipse-cdt-cloud/cdt-gdb-vscode#111.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIParser hangs on nested objects
2 participants