Skip to content
This repository was archived by the owner on Jul 15, 2023. It is now read-only.

Add stacktrace paging support to debug adapter - #2080

Merged
Ramya Rao (ramya-rao-a) merged 4 commits into
microsoft:masterfrom
brycekahle:stacktrace-paging
Nov 14, 2018
Merged

Add stacktrace paging support to debug adapter#2080
Ramya Rao (ramya-rao-a) merged 4 commits into
microsoft:masterfrom
brycekahle:stacktrace-paging

Conversation

@brycekahle

Copy link
Copy Markdown
Contributor

Fixes #946

This still has a maximum depth of 50. Delve has no way to specify an infinite max depth, or query the length of the stacktrace.

@brycekahle

Copy link
Copy Markdown
Contributor Author

AFAICT the CI failures are not related to my changes. Perhaps someone can re-run them?

@ramya-rao-a

Copy link
Copy Markdown
Contributor

Bryce Kahle (@brycekahle) Like you said, this just increases the depth from 20 to 50

How about adding a setting to the debug configuration so that the user can set the depth instead?

@brycekahle

Copy link
Copy Markdown
Contributor Author

Ramya Rao (@ramya-rao-a) I've added a setting for the maximum stack trace depth.

@ramya-rao-a Ramya Rao (ramya-rao-a) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Bryce Kahle (@brycekahle)

I have pushed a commit to change the default to 20 to keep it consistent with current behavior. I am not sure if increasing it to 50 will cause perf issues, therefore retaining it at 20

Also reverted the change made to goDebugConfiguration.ts file. That change only works if the new stackTraceDepth is part of both the launch.json config and the normal settings. Since we are adding it only to the former, the fallback won't work

@ramya-rao-a
Ramya Rao (ramya-rao-a) merged commit 9277eb1 into microsoft:master Nov 14, 2018
@brycekahle
Bryce Kahle (brycekahle) deleted the stacktrace-paging branch November 14, 2018 04:50
@brycekahle

Copy link
Copy Markdown
Contributor Author

Ramya Rao (@ramya-rao-a) FYI changing this to 20 by default, will not allow any paging unless the setting is changed. VS Code uses a page size of 20, and by default we never indicate there are more than 20 frames (via totalFrames).

Perhaps it is worth filing an issue on delve to have better support for stack frame paging?

@ramya-rao-a

Ramya Rao (ramya-rao-a) commented Nov 14, 2018

Copy link
Copy Markdown
Contributor

Perhaps it is worth filing an issue on delve to have better support for stack frame paging?

For delve, looks like something similar has already been discussed. See https://github.com/derekparker/delve/issues/989 due to which the default depth for delve is now 50

FYI changing this to 20 by default, will not allow any paging unless the setting is changed

Good point. Given that delve itself has made the default to 50 and that this won't affect normal programs that don't usually go that deep, keeping the default as 50 should be ok

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants