Add stacktrace dump and better error messages on EXC_BAD_ACCESS panics #2904
Conversation
|
@ramya-rao-a PTAL |
|
@jhendrixMSFT Can you take another look here? |
src/debugAdapter/goDebug.ts
Outdated
| // Get current goroutine | ||
| // Debugger may be stopped at this point but we still can (and need) to obtain state and stacktrace | ||
| let goroutineId = 0; | ||
| this.delve.call<DebuggerState | CommandOut>('State', [{ NonBlocking: true }], (err, out) => { |
There was a problem hiding this comment.
I tested this out on macOS, unfortunately dlv doesn't report the correct go routine in this case (I see the same behavior using vscode and dlv) so I think we can remove this. What it means though is if the panic is on a go routine other than zero you won't get the correct stack trace. If we still want to include the stack trace then I think we should add a message indicating that it might not be correct. :(
There was a problem hiding this comment.
I would leave it just for diagnose purposes with a warning message saying that the stacktrace might differ from the expected goroutine
I tested this out on macOS, unfortunately dlv doesn't report the correct go routine in this case (I see the same behavior using vscode and dlv)
This is probably related to the original dlv issue, or maybe it is a different issue that might be actionable, nevertheless, that is still something that should be discussed upstream, unfortunately
lggomez
force-pushed
the
fix_1903
branch
3 times, most recently
from
cbcfa9c to
cd6485b
Compare
src/debugAdapter/goDebug.ts
Outdated
| logError(message + ' - ' + errorMessage); | ||
|
|
||
| if (err.toString() === 'bad access') { | ||
| logError('WARNING: this stack might not be from the expected active goroutine'); |
There was a problem hiding this comment.
I think this might suffice for now, what do you think @jhendrixMSFT?
There was a problem hiding this comment.
I would reword this to "...the faulting goroutine".
I also wonder if it would just be better to omit the stack trace entirely given that it might not be accurate (we could add it later if there's sufficient feedback). @ramya-rao-a do you have an opinion about this?
src/debugAdapter/goDebug.ts
Outdated
|
|
||
| logError(message + ' - ' + errorMessage); | ||
|
|
||
| if (err.toString() === 'bad access') { |
There was a problem hiding this comment.
The same if check exists a few lines above, so consider moving this log statement to the previous if block
There was a problem hiding this comment.
I don't like the duplicate if either but I did it so the warning comes right before the stacktrace and not before the errorMessage:
Current:
Simplified:
I don't think it is that much of an issue so if you're fine with that I can go on with the single if version
There was a problem hiding this comment.
Understood, in that case let's keep the current order
| }); | ||
|
|
||
| // Get goroutine stacktrace | ||
| const stackTraceIn = { id: goroutineId, depth: this.delve.stackTraceDepth }; |
There was a problem hiding this comment.
The goroutineId here will always be 0 as this code path is not inside the callback provided to the delve call a few lines above.
There was a problem hiding this comment.
You can try
try {
const stateCallResult: DebuggerState | CommandOut = await this.delve.callPromise('State', [{ NonBlocking: true }]);
goroutineId = this.delve.isApiV1 ? (<CommandOut>stateCallResult).State.currentGoroutine.id : (<DebuggerState>stateCallResult).currentGoroutine.id;
} catch (error) {
// log error?
}There was a problem hiding this comment.
@jhendrixMSFT @ramya-rao-a I think I solved the issue of state returning always (?) zero. The gist of the debugger state is that there seems to be no currentGoroutine in this scenario so we have to default to the currentThread goroutineId:
This is the apicall response:
{
"Running": false,
"currentThread": {
"id": 5142829,
"pc": 17138024,
"file": "/Users/lgomez/DEV/go/test_1903/main.go",
"line": 5,
"function": {
"name": "main.oops",
"value": 17138000,
"type": 0,
"goType": 0,
"optimized": false
},
"goroutineID": 1,
"ReturnValues": null
},
"Threads": [
{
"id": 5142829,
"pc": 17138024,
"file": "/Users/lgomez/DEV/go/test_1903/main.go",
"line": 5,
"function": {
"name": "main.oops",
"value": 17138000,
"type": 0,
"goType": 0,
"optimized": false
},
"goroutineID": 1,
"ReturnValues": null
},
{
"id": 5142873,
"pc": 140734907268914,
"file": "",
"line": 0,
"goroutineID": 0,
"ReturnValues": null
},
{
"id": 5142874,
"pc": 140734907267178,
"file": "",
"line": 0,
"goroutineID": 0,
"ReturnValues": null
},
{
"id": 5142875,
"pc": 140734907267178,
"file": "",
"line": 0,
"goroutineID": 0,
"ReturnValues": null
},
{
"id": 5142876,
"pc": 140734907267178,
"file": "",
"line": 0,
"goroutineID": 0,
"ReturnValues": null
}
],
"NextInProgress": false,
"exited": false,
"exitStatus": 0,
"When": ""
}And this is the handled panic with the updated behavior:
This is on mac, I´ll try this at home later to see if the response is different on linux
There was a problem hiding this comment.
I'm not getting the currentGoroutine on linux either, i'm trying other goroutine examples and i'm even reproducing #2563 while at it so I'm willing to say this is our best bet for now. We're supposed to be getting this field but we can follow up on that issue since it is better suited for its root cause
|
cc @quoctruong to try out the changes here |
|
@ramya-rao-a any other concerns? |
lggomez
force-pushed
the
fix_1903
branch
2 times, most recently
from
c74b73d to
46fe64c
Compare
Usability workaround for microsoft#1903
|
@andrewchoi5 I decided to reset and rebase my changes to current master since there were many merge conflicts to solve (the branch was a bit stale and several changes entered into the debug adapter), would you care to check again please? |
* master: goLanguageServer: set completion follow up command from middleware (microsoft#3084) Add stacktrace dump and better error messages on EXC_BAD_ACCESS panics (microsoft#2904) Address mismatch on path separators in debug config (microsoft#2010) (microsoft#3108) Include the link to release note/package overview in the update prompt, and update gopls default version (microsoft#3041) bug_report.md: Fix "architecture" typo. (microsoft#3095) telemetry.ts: send telemetry only if aiKey is not an empty string(microsoft#3091)
…sync microsoft/vscode-go@d53b1b3 * 'master' of https://github.com/microsoft/vscode-go: goLanguageServer: set completion follow up command from middleware (microsoft#3084) Add stacktrace dump and better error messages on EXC_BAD_ACCESS panics (microsoft#2904) Address mismatch on path separators in debug config (microsoft#2010) (microsoft#3108)
Merge branch 'master' of https://github.com/microsoft/vscode-go@d53b1b3 * 'master' of https://github.com/microsoft/vscode-go: goLanguageServer: set completion follow up command from middleware (microsoft#3084) Add stacktrace dump and better error messages on EXC_BAD_ACCESS panics (microsoft#2904) Address mismatch on path separators in debug config (microsoft#2010) (microsoft#3108) Created by `git pull --no-ff --log upstream master` Change-Id: Id38768f3ec1bd01fa81325978f51f314fc1c08cb GitHub-Last-Rev: 3a8de3f GitHub-Pull-Request: #17 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/224240 Reviewed-by: Rebecca Stambler <[email protected]>
Fixes #1903