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

Prevent stripping of format specifiers from -exec commands. (#1277) #1278

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

gareth-rees
Copy link
Contributor

This prevents MIEngine from stripping (what appear to be) format specifiers from debug console commands prefixed by -exec or backtick. Fixes #1277.

I'm afraid that I could not figure out how to write a test case, as there do not seem to be any tests cases for -exec commands, so I had nothing to go on. If someone can give me advice on writing a test case for this, I'd be happy to add one. In the mean time I tested it by hand; see the screenshot below in Visual Studio Code 1.64.2:

Screenshot from 2022-02-24 11-45-20

@gregg-miskelly
Copy link
Member

Thank you for taking the time to fix this. But this isn't the right spot to fix this problem. I believe what, you want to do is update StripFormatSpecifier to do:

            if (EngineUtils.IsConsoleExecCmd(exp, out string _, out string _))
            {
                return exp;
            }

@gareth-rees
Copy link
Contributor Author

This isn't the right spot to fix this problem. I believe what, you want to do is update StripFormatSpecifier

Thanks for reviewing. Your suggestion corresponds to my solution (2) in #1277, which I was not sure was safe, because AD7DebugSession.HandleEvaluateRequestAsync is not the only place where StripFormatSpecifier gets called. However, if you are confident that it's safe to modify StripFormatSpecifier like this, then I'm happy to give it a try, and accordingly I've updated the pull request to use your suggestion.

@ghost
Copy link

ghost commented Feb 25, 2022

CLA assistant check
All CLA requirements met.

@gareth-rees
Copy link
Contributor Author

The continuous integration failure does not seem to be related to this pull request — you can see the same failure on #1280.

@gregg-miskelly
Copy link
Member

@gareth-rees: your changes look good to me

@WardenGnaw Do you know what is going on with the PR failure?

If I am reading the log correctly, the response looks right:

 <--   C (setVariable-8): {"command":"setVariable","arguments":{"name":"myint","value":"39+nonexistingint","variablesReference":1000},"seq":8,"type":"request"}
 
 --> R (setVariable-8): {"type":"response","request_seq":8,"success":false,"command":"setVariable","message":"error: <user expression 0>:1:4: use of undeclared identifier 'nonexistingint'\n39+nonexistingint\n   ^\n","body":{"error":{"id":1107,"format":"error: <user expression 0>:1:4: use of undeclared identifier 'nonexistingint'\n39+nonexistingint\n   ^\n"}},"seq":113}

Here is the test link in case this is helpful:

currentFrame.GetVariable("myint").SetVariableValueExpectFailure("39+nonexistingint");

@gregg-miskelly
Copy link
Member

@gareth-rees: BTW: it looks like you still need to sign the CLA

@gareth-rees
Copy link
Contributor Author

@gareth-rees: BTW: it looks like you still need to sign the CLA

Yes, just waiting for my employer to agree. Should be able to sign it on Monday.

@WardenGnaw
Copy link
Member

@gareth-rees: your changes look good to me

@WardenGnaw Do you know what is going on with the PR failure?

If I am reading the log correctly, the response looks right:

 <--   C (setVariable-8): {"command":"setVariable","arguments":{"name":"myint","value":"39+nonexistingint","variablesReference":1000},"seq":8,"type":"request"}
 
 --> R (setVariable-8): {"type":"response","request_seq":8,"success":false,"command":"setVariable","message":"error: <user expression 0>:1:4: use of undeclared identifier 'nonexistingint'\n39+nonexistingint\n   ^\n","body":{"error":{"id":1107,"format":"error: <user expression 0>:1:4: use of undeclared identifier 'nonexistingint'\n39+nonexistingint\n   ^\n"}},"seq":113}

Here is the test link in case this is helpful:

currentFrame.GetVariable("myint").SetVariableValueExpectFailure("39+nonexistingint");

Something changed within the build system that is causing this test to fail. It is on my backlog to investigate what changed.
This is also failing during our nightly builds: https://github.com/microsoft/MIEngine/runs/5312098531?check_suite_focus=true

@gareth-rees
Copy link
Contributor Author

I've signed the CLA. Sorry about the delay!

@gregg-miskelly gregg-miskelly merged commit de6905b into microsoft:main Mar 3, 2022
@gregg-miskelly
Copy link
Member

@gareth-rees Thanks so much for taking the time to fix this!

@gareth-rees
Copy link
Contributor Author

@gregg-miskelly You're welcome. Thanks for the quick review and merge.

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.

Format specifiers are wrongly stripped from -exec commands
3 participants