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

Support SteppingGranularity & instruction stepping #309

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Oct 26, 2023

This PR adds support for SteppingGranularity to the next and stepIn requests, e.g. from the VSCode disassembly view.

To do:

  • I'll add some tests tomorrow next week.

@colin-grant-work colin-grant-work marked this pull request as draft October 26, 2023 22:41
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.

I am delighted to see you brining this to the adapter.

src/GDBDebugSession.ts Outdated Show resolved Hide resolved
@colin-grant-work colin-grant-work changed the title Support SteppingGranularity & instructtion stepping Support SteppingGranularity & instruction stepping Oct 31, 2023
@jonahgraham
Copy link
Contributor

Battle of the formatters :-/

Who is battling? "yarn format" uses prettier, I have prettier installed in the IDE so it formats on save. Maybe we should add it to recommendations in extension.json?

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Oct 31, 2023

Who is battling? "yarn format" uses prettier, I have prettier installed in the IDE so it formats on save. Maybe we should add it to recommendations in extension.json?

Yeah, it's the battle between VSCode's default formatter and Prettier. It would likely be helpful to add Prettier to the recommendations & add

"editor.defaultFormatter": "esbenp.prettier-vscode"

to the settings.json. Then the only thing missing for a perfect set-up is for contributors to set other things as the default formatter elsewhere so having the Prettier plugin installed doesn't make formatting harder in other repos. Or to set Prettier as disabled in other workspaces and enabled here. I had things more nicely set up for a while, but I jump around a lot and use a lot of ad-hoc multi-root workspaces, so it got messy again.

The difficulty with Prettier is that it's so versatile, so setting it up is easy with the preference above, but disabling it means setting a bunch of language-specific things:

[typescript]: {editor.defaultFormatter: notPrettier1}
[javascript]: {editor.defaultFormatter: notPrettier1}
[markdown]: {editor.defaultFormmatter: notPrettier2}
[json]: {editor.defaultFormatter: notPrettier3}

@colin-grant-work colin-grant-work marked this pull request as ready for review October 31, 2023 20:02
@colin-grant-work
Copy link
Contributor Author

Alright, @jonahgraham, I seem to have finally managed to write tests that weren't foolishly specific to my machine, if you wouldn't mind taking a look.

@jonahgraham
Copy link
Contributor

Your change looks good to me, I have made a couple of commits to make the test code more generic. Please see commits for details as to why I suggest such changes.

  • 510fcf7 Fix warning in C code:
  • 8e964ea Use line tags instead of hard coded line numbers
  • 61bc8e5 make the line of code the same as opening brace to account for different gdb/gcc combinations

@colin-grant-work
Copy link
Contributor Author

Thanks for the fixes! Could you explain the significance of this part?

make the line of code the same as opening brace to account for different gdb/gcc combinations

I saw that comment / commit message in other test cases, but I didn't and still don't understand why that particular change is important.

@jonahgraham
Copy link
Contributor

For a function like this:

int method(void) {
  call_something();
}

some combinations of GCC/GDB versions consider line 1 (line with {) the first line of the method and some consider it line 2 (line with first instructions). So to avoid cases where we want to detect we are on the first source line of a method we place { and first code on same source line so we don't have to identify the differences in the tests.

If you want I can dig through my history for which combinations do which thing.

Is that change ok to you?

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Nov 3, 2023

Yeah, definitely OK - just wanted to understand better why it was needed. Thanks!

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.

Can you squash and rebase (and then merge if CI approves)?

@colin-grant-work colin-grant-work force-pushed the feature/instruction-stepping branch 3 times, most recently from 4a3154a to 20e1671 Compare November 3, 2023 16:40
@colin-grant-work
Copy link
Contributor Author

Sorry for the multiple pushes - was confused about where your commits lived, but I believe I've got them now.

@jonahgraham
Copy link
Contributor

Sorry for the multiple pushes - was confused about where your commits lived, but I believe I've got them now.

No worries - it is sort of weird that reviewers can push to contributor branches :-)

@colin-grant-work colin-grant-work merged commit 89cdd9c into eclipse-cdt-cloud:main Nov 3, 2023
4 checks passed
@colin-grant-work colin-grant-work deleted the feature/instruction-stepping branch November 3, 2023 17:19
jonahgraham added a commit to jonahgraham/cdt-gdb-vscode that referenced this pull request Nov 21, 2023
Adds support for these features/bug fixes in the adapter:

- A fix the newline handling for the UART
  - eclipse-cdt-cloud/cdt-gdb-adapter#318
- New feature of supporting stepping granularity
  - eclipse-cdt-cloud/cdt-gdb-adapter#309
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.

2 participants