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

Add stacktrace paging support to debug adapter #2080

Merged
merged 4 commits into from
Nov 14, 2018
Merged

Add stacktrace paging support to debug adapter #2080

merged 4 commits into from
Nov 14, 2018

Conversation

brycekahle
Copy link
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
Contributor Author

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

@ramya-rao-a
Copy link
Contributor

@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
Contributor Author

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

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Thanks @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-a merged commit 9277eb1 into microsoft:master Nov 14, 2018
@brycekahle brycekahle deleted the stacktrace-paging branch November 14, 2018 04:50
@brycekahle
Copy link
Contributor Author

@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
Copy link
Contributor

ramya-rao-a commented Nov 14, 2018

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