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

debug: warn users when build flags strip debug info #182

Closed
stamblerre opened this issue Jun 7, 2020 · 17 comments
Closed

debug: warn users when build flags strip debug info #182

stamblerre opened this issue Jun 7, 2020 · 17 comments
Assignees
Labels
Debug Issues related to the debugging functionality of the extension. Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@stamblerre
Copy link
Contributor

This is copied from the original issue: microsoft/vscode-go#2783.

The user had set --ldflags "-s" as build flags, which were stripping debug information. We should surface an error message when users try to debug binaries built with these flags.

@stamblerre stamblerre added Debug Issues related to the debugging functionality of the extension. NeedsFix The path to resolution is known, but the work has not been done. labels Jun 7, 2020
@hyangah hyangah added this to the Backlog milestone Aug 7, 2020
@hyangah
Copy link
Contributor

hyangah commented Aug 28, 2020

-w and other future flags go will add (golang/go#38777) as well.
Another approach is to process the stderr and find error messages like
could not launch process: could not open debug info, but that's gross and not reliable.

cc @suzmue

@polinasok
Copy link
Contributor

polinasok commented Nov 30, 2020

I am not getting any issues with just "-s". E.g. with just dlv:

$ dlv debug issue129.go --build-flags="--ldflags '-s'"
Type 'help' for list of commands.
(dlv) b main.main
Breakpoint 1 set at 0x10c4f13 for main.main() ./foo.go:40
(dlv) breakpoints
Breakpoint runtime-fatal-throw at 0x1033d10 for runtime.fatalthrow() /usr/local/go/src/runtime/panic.go:1162 (0)
Breakpoint unrecovered-panic at 0x1033d80 for runtime.fatalpanic() /usr/local/go/src/runtime/panic.go:1189 (0)
	print runtime.curg._panic.arg
Breakpoint 1 at 0x10c4f13 for main.main() ./foo.go:40 (0)

But I am seeing a similar failure with "-s -w":

$ dlv debug issue129.go --build-flags="--ldflags '-s -w'"
could not launch process: decoding dwarf section info at offset 0x0: too short

In vscode the main indicator that launching a debug session failed is the fact that there is no Debug toolbar (with pause, step, etc), which appears and almost immediately disappears. If all the logging is disabled the following will appear in the DEBUG CONSOLE:

API server listening at: 127.0.0.1:5911
could not launch process: decoding dwarf section info at offset 0x0: too short
Process exiting with code: 1

It might be awkward to parse stderr for messages, but we definitely know when we get an error code from the process and could do better reporting there:

logError('Process exiting with code: ' + code);

So is our goal here to (1) proactively parse the build flags, recognize problematic ones and warn the users as they fill out the launch configuration or launch the debug session (e.g. in case it succeeds but might result in unexpected odd behavior) or (2) do a better job channeling the launch/attach failure messages from dlv when the session doesn't start? I vote for (2). I think we should leave flag parsing and sanity checking to dlv.

cc @DavidLangworthy

@hyangah
Copy link
Contributor

hyangah commented Nov 30, 2020

Thanks @polinasok for looking into it. As we discussed offline, I agree that 2 is favorable if we do anything for this issue - because what flags are acceptable or compatible is primarily a business between dlv and go, and there are many other ways to configure build settings to interfere with delve and we will never cover those similarly rare corner cases.
And the user experience the option 2 will be also consistent with handling of problematic attach type request workflows that use a binary built with incomplete debug info.

We also discussed utilizing a better UI element (e.g. show message popup, or report as an error, etc) in the future once we start using dlv dap - Since the initialization flow will be changed with the dlv dap mode, I am not 100% sure but it seems that if dlv dap responds an error for such failed launch/attach requests (but not kill itself yet) etc, vscode can popup an error message window and start the usual tear down approach if necessary.

@hyangah hyangah added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Nov 30, 2020
@polinasok
Copy link
Contributor

polinasok commented Dec 2, 2020

For full transparency, we kind of entered a gray territory of making flags our business when removing gcflags not to conflict with dlv's defaults (#117). However, in that case it was all or nothing. We don't actually parse and analyze the specific flags, but just ignore them altogether and rely on dlv to supply its own version. In case of ldflags, there are no defaults in dlv, so we can't follow the same logic.

If the user supplies something that completely breaks debugging (e.g. -w), dlv very clearly reports back that there was a problem by failing to launch the program. As I described above, this error message is reflected in the DEBUG CONSOLE, but can be hard to spot if there is a lot of logging around it.

When suggesting that we improve error reporting here, I was thinking we should make sure to have a pop-up with an error message for better visibility. The only way I know how to trigger this is with an ErrorResponse. OutputEvent doesn't seem to have a way to trigger pop-ups. But looking into this more closely, I see that the current adapter already does the right thing because it returns an ErrorResponse with showUser in response to the LaunchRequest.

To client: {"seq":55,"type":"response","request_seq":2,"command":"launch","success":false,"message":"Failed to continue: Check the debug console for details.","body":{"error":{"id":3000,"format":"Failed to continue: Check the debug console for details.","showUser":true}}}

But I see no pop-ups for this error!

Compare this to the case where dlv fails because you have a syntax error. You get 2 pop-ups - one at the top because this is a launch failure and one on the bottom because of showUser.

image

I am not sure why there is such a difference.

In any case, I checked and dlv dap does the right thing here and it can actually put the error message into the pop-up as opposed to relying on the DEBUG CONSOLE for details.

2020-12-01T15:23:12-08:00 debug layer=dap [-> to client]{"seq":0,"type":"response","request_seq":2,"success":false,"command":"launch","message":"Failed to launch","body":{"error":{"id":3000,"format":"Failed to launch: could not launch process: decoding dwarf section info at offset 0x0: too short"}}}

image

@polinasok
Copy link
Contributor

polinasok commented Dec 2, 2020

@aarzilli Should dlv always fail with --ldflags "-s"?
https://golang.org/cmd/link/ says "Omit the symbol table and debug information". go-delve/delve#1485 mentions related failures. But golang/go#7793 suggests that -s is no longer the way to strip debug info. I was able to start a debug session with it - see above. What am I missing?

@polinasok polinasok added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 2, 2020
@polinasok
Copy link
Contributor

polinasok commented Dec 2, 2020

I see what the issue is in the current adapter. We send two launch responses. One successful when dlv launches (we do get API server listening at) and one duplicate error response when delve encounters a fatal error launching the process. The second one is a dup and it is ignored, so it triggers no error pop-ups:

To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"API server listening at: 127.0.0.1:3149\n"}}
API server listening at: 127.0.0.1:3149
To client: {"seq":0,"type":"event","event":"initialized"}
InitializeEvent
===> To client: {"seq":0,"type":"response","request_seq":2,"command":"launch","success":true}
From client: setBreakpoints({"source":{"name":"foo.go","path":"/Users/polina/go/src/foo/foo.go"},"lines":[28],"breakpoints":[{"line":28}],"sourceModified":false})
SetBreakPointsRequest
To client: {"seq":0,"type":"event","event":"output","body":{"category":"stderr","output":"could not launch process: decoding dwarf section info at offset 0x0: too short\n"}}
could not launch process: decoding dwarf section info at offset 0x0: too short
Process exiting with code: 1
===> To client: {"seq":55,"type":"response","request_seq":2,"command":"launch","success":false,"message":"Failed to continue: Check the debug console for details.","body":{"error":{"id":3000,"format":"Failed to continue: Check the debug console for details.","showUser":true}}}

@polinasok polinasok added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 2, 2020
@polinasok
Copy link
Contributor

With the legacy adapter, the launch/attach failure details were hidden due a bug in the sequence of responses. We have now switched to dlv-dap which treats stripped debug info as a clear failure to launch a debug session with a pop-up that's the same as what dlv cli would respond with:
image

@polinasok
Copy link
Contributor

polinasok commented Mar 16, 2022

To answer my own question earlier, it appears that -s does not actually strip the binary on macOS.

On Linux

$ dlv debug helloworld.go --build-flags="--ldflags '-s'"
could not launch process: could not open debug info
$ dlv debug helloworld.go --build-flags="--ldflags '-w'"
could not launch process: could not open debug info

On MacOS

$ go build increment.go; ls -l increment
...  1882000 Mar 15 21:50 increment
$ go build --ldflags '-s' increment.go; ls -l increment
...  1882000 Mar 15 21:50 increment
$ go build --ldflags '-w' increment.go; ls -l increment
...  1386384 Mar 15 21:51 increment
$ dlv debug increment.go --build-flags="--ldflags '-w'"
could not launch process: decoding dwarf section info at offset 0x0: too short
$ dlv debug increment.go --build-flags="--ldflags '-s'"
Type 'help' for list of commands.
(dlv) 

For reference:

$ go tool link
...
  -s	disable symbol table
...
  -w	disable DWARF generation

@aarzilli
Copy link
Contributor

Looks like a linker bug.

@polinasok
Copy link
Contributor

@aarzilli Looking at this and dlv repo, there are many issues where confused users don't understand what "decoding dwarf section info" errors mean. "could not open debug info" is slightly better, but still confuses people. I was about to add an FAQ entry, debating if it belongs in vscode-go docs and/or dlv docs, but now I am thinking that perhaps it would be better to somehow print more informative error messages in such cases, pointing out that users must not use -s -w link flags or use go run? WDYT?

@aarzilli
Copy link
Contributor

I'm skeptical that it will help, but we can try.
Regarding the skepticism: manually specifying -s -w and then getting a debugger error should already clue you in into what's happening and googling the error message will also lead to the explanation, so I don't think it'll work. See also the endless stream of people befuddled by the error message about Rosetta.

@hyangah
Copy link
Contributor

hyangah commented Mar 16, 2022

IMO an FAQ entry in vscode-go docs is better in this case because
VSCode Go's build flag setting forwarding is often hidden to the users and
that's often the culprit of the confusion. If a message from dlv dap layer
is printed in the DEBUG CONSOLE or the popup message when it detects
the use of incompatible settings, that will be a plus too.

On the other hand, When users of the command line tool dlv use those
incompatible flags (either by explicitly asking dlv to use them or building a
binary with them), I guess they should know what they are doing most likely.

@polinasok
Copy link
Contributor

VSCode Go's build flag setting forwarding is often hidden to the users and that's often the culprit of the confusion

What do you mean?

@hyangah
Copy link
Contributor

hyangah commented Mar 30, 2022

VSCode Go's build flag setting forwarding is often hidden to the users and that's often the culprit of the confusion

What do you mean?

All the configuration massaging in src/goDebugConfiguration.ts and the future change described in #128.

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/396882 mentions this issue: debugging.md: add FAQ on debugging binaries with missing debug info

@polinasok
Copy link
Contributor

polinasok commented Mar 31, 2022

After dlv change lands, the error will look like so:
image

gopherbot pushed a commit that referenced this issue Mar 31, 2022
Updates #182

Change-Id: I056edb1056499d714ae303988366780fa5620cdc
GitHub-Last-Rev: 52890f1
GitHub-Pull-Request: #2152
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/396882
Trust: Jamal Carvalho <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
@polinasok
Copy link
Contributor

The above error change was merged. The documentation has been updated.

@golang golang locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Debug Issues related to the debugging functionality of the extension. Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants