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 registerLimit config option to specify the registers to list in Registers view #444

Closed
wants to merge 1 commit into from

Conversation

chenzhiy2001
Copy link
Contributor

In cases like #421, fetching unfetchable registers can corrupt the whole Registers view.

So I added registerLimit config option to let users specify what registers they want the debugger to automatically fetch, thus avoiding the issue.

@chenzhiy2001 chenzhiy2001 mentioned this pull request Sep 18, 2024
4 tasks
@GitMensch GitMensch linked an issue Sep 18, 2024 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Thank you for this nice PR!

Apart from the comments below:

  • please add an entry to the Changelog file
  • it is likely reasonable to note this option in the README, as well as pointing out how to get the numbers "(starting from zero)", which I guess is either info registers or interpreter mi2 "-data-list-register-values --skip-unavailable N".

package.json Outdated
Comment on lines 1146 to 1176
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert that missing EOL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. the formatter wrongly did this.

@@ -883,7 +883,7 @@ export class MI2 extends EventEmitter implements IBackend {
async getRegisterValues(): Promise<RegisterValue[]> {
if (trace)
this.log("stderr", "getRegisterValues");
const result = await this.sendCommand("data-list-register-values N");
const result = await this.sendCommand("data-list-register-values N"+ " " +this.registerLimit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

const result = await this.sendCommand("data-list-register-values N " + this.registerLimit);
but maybe we should use
const result = await this.sendCommand("data-list-register-values --skip-unavailable N " + this.registerLimit);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second one is indeed better since it avoids more unavaliable registers when this attribute works.

@chenzhiy2001 chenzhiy2001 force-pushed the master branch 2 times, most recently from 795cf71 to 9e641d2 Compare September 19, 2024 10:03
@GitMensch
Copy link
Collaborator

GitMensch commented Sep 19, 2024

spell check linter failed on "unfetchable", I've asked chatgpt4 for an adjusted text and it changed some other pieces as well, coming up with (after some hinting)

There is a Registers view in the VARIABLES view. As we fetch all registers at once, there can
be cases where a register that cannot be fetched causes the entire register request to fail,
corrupting the entire Registers output. If this happens, you might need to set the
`registerLimit` option to specify which registers you want the debugger to fetch
automatically.  
For example, to display only registers `rax` and `rip` in an x64 debug session, send the
command `-data-list-register-names` in the debug console of an active x64 debug session.
You will then receive a response containing an array starting with `["rax","rbx" ...]`.
In this array, the index of `rax` is 0 and `rip` is 16, so set the option as
`"registerLimit": "0 16"`. If you find the response text hard to navigate, you can paste
it into a browser's developer tools console and press enter to get an expandable response
object with array elements' indices explicitly displayed.

Copy link
Contributor Author

@chenzhiy2001 chenzhiy2001 left a comment

Choose a reason for hiding this comment

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

  • Reverted the deletion of EOL in package.json
  • Appended --skip-unavaliable after -data-list-register-names
  • Added an entry in CHANGELOG.md
  • Added an introduction of how to set registerLimit option in README.md

package.json Outdated
Comment on lines 1146 to 1176
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. the formatter wrongly did this.

@@ -883,7 +883,7 @@ export class MI2 extends EventEmitter implements IBackend {
async getRegisterValues(): Promise<RegisterValue[]> {
if (trace)
this.log("stderr", "getRegisterValues");
const result = await this.sendCommand("data-list-register-values N");
const result = await this.sendCommand("data-list-register-values N"+ " " +this.registerLimit);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second one is indeed better since it avoids more unavaliable registers when this attribute works.

@chenzhiy2001
Copy link
Contributor Author

The introduction paragraph in README.md is being swapped by the new version now.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 22.52%. Comparing base (1b9f13e) to head (bf77aa8).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
src/gdb.ts 0.00% 2 Missing ⚠️
src/lldb.ts 0.00% 2 Missing ⚠️
src/mago.ts 0.00% 2 Missing ⚠️
src/backend/mi2/mi2.ts 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   20.47%   22.52%   +2.05%     
==========================================
  Files          14       14              
  Lines        1807     1891      +84     
  Branches      392      416      +24     
==========================================
+ Hits          370      426      +56     
- Misses       1392     1465      +73     
+ Partials       45        0      -45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GitMensch
Copy link
Collaborator

GitMensch commented Sep 19, 2024

I've run the prettier stuff locally, that's the diff you need to apply to fix the failing CI

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 602bf5a..47db805 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,7 +22,8 @@ Versioning].
   enabled by default ([@JacquesLucke])
 - Suppress error for hover as the user may just play with the mouse ([@oltolm]).
 - solve the problem of failed parsing of containers ([@henryriley0])
-- Resolves #421 - Added `registerLimit` option to specify the registers to display to avoid corrupting the whole registers view - PR #444 ([@chenzhiy2001])
+- Fixes #421 - Added `registerLimit` option to specify the registers to
+  display - PR #444 ([@chenzhiy2001])
 
 ## [0.27.0] - 2024-02-07
 
@@ -244,6 +245,7 @@ Versioning].
 [@abussy-aldebaran]: https://github.com/abussy-aldebaran
 [@anshulrouthu]: https://github.com/anshulrouthu
 [@brownts]: https://github.com/brownts
+[@chenzhiy2001]: https://github.com/chenzhiy2001
 [@coldencullen]: https://github.com/ColdenCullen
 [@eamousing]: https://github.com/eamousing
 [@evangrayk]: https://github.com/evangrayk

@GitMensch GitMensch closed this Sep 19, 2024
@chenzhiy2001
Copy link
Contributor Author

Oops, I forgot to run npm run prettier-write-docs. Could you please reopen this pull request so that the correct modification of CHANGELOG.md show up here?

@GitMensch
Copy link
Collaborator

I have no clue why the close happened...

@chenzhiy2001
Copy link
Contributor Author

I have no clue why the close happened...

Sometimes Github gets really confusing. I also felt that.

@GitMensch
Copy link
Collaborator

It says "The master branch was force-pushed", please try to just push the change yourself, if the reopen still doesn't work afterwards, create a new PR (leaving the old PR reference in). See the suggested ChangeLog diff above.

GitMensch added a commit that referenced this pull request Sep 19, 2024
Add `registerLimit` config option to specify the registers to list in Registers view (continuation of PR #444)
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.

Ignore unfetchable registers
3 participants