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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: optimization around runtime.throw breaks delve DAP #62523

Closed
hyangah opened this issue Sep 8, 2023 · 6 comments
Closed

cmd/compile: optimization around runtime.throw breaks delve DAP #62523

hyangah opened this issue Sep 8, 2023 · 6 comments
Assignees
@thanm
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Go1.22

Comments

@hyangah
Copy link
Contributor

hyangah commented Sep 8, 2023

New issue in the current tip/1.22.

From @aarzilli : go-delve/delve#3455 (comment)

go-delve/delve#2616 description explains how delve is currently extracting the reason info.

From @thanm : look into extending the existing x/debug/dwtest tests to catch this type of regression.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 8, 2023
@thanm thanm self-assigned this Sep 8, 2023
@thanm
Copy link
Contributor

thanm commented Sep 8, 2023

I dug into this a bit. In fact the DWARF for runtime.throw looks fine, I don't think that is the issue. What appears to be different is this CL: http://go.dev/cl/c/go/+/390420, which is changing the runtime to differentiate between 'user' throws and 'system' throws. As a result of this CL, runtime.newproc1 is invoking 'runtime.fatalinstead ofruntime.throw`.

It sounds as though any special treatment that Delve has for 'runtime.throw' may need to be extended to runtime.fatal as well.

@thanm thanm mentioned this issue Sep 8, 2023
Merged
@aarzilli
Copy link
Contributor

aarzilli commented Sep 8, 2023

I don't think that we do anything special for either, it's just reading the s argument, it's the same code for both and it's probably broken for both of them. I haven't bisected the problem but I would expect this to be caused by a compiler change.

Looking at this more it looks like the problem only happens when the program is compiled with -gcflags=all=-N -l (is -N applied to the runtime now?) where the location for s for runtime.fatal is DW_OP_call_frame_cfa (and nothing else). With optimizations enabled it gets a correct location list.

@hyangah hyangah added this to the Go1.22 milestone Sep 8, 2023
@thanm
Copy link
Contributor

thanm commented Sep 8, 2023

Bisecting seems to point the finger at https://go-review.googlesource.com/c/go/+/521699, which makes sense based on the commit message. I'll look a bit more tomorrow.

@thanm
Copy link
Contributor

thanm commented Sep 11, 2023

When building the runtime with "-gcflags="all=-l -N", it looks as though when we execute this code here:

https://go.googlesource.com/go/+/fbcf43c60ba5170309a238b0e42fd5879d419776/src/cmd/compile/internal/dwarfgen/dwarf.go#146

At that line there is a check made to see if "base.Ctxt.Flag_optimize" is set, this check is failing, which causes the code to drop back down into the ABI0 case. The problem from CL 521699 appears to be in this line here:

https://go.googlesource.com/go/+/fbcf43c60ba5170309a238b0e42fd5879d419776/src/cmd/compile/internal/base/flag.go#336

which sets "base.Flag.N" to 0, but doesn't correspondingly update base.Ctxt.Flag_optimize (which was set previously on line 274 based on the original value of -N).

I'll send a CL. Also kind of makes me wonder whether we should still have both "base.Flag.N" and "base.Ctxt.Flag_optimize"; I think the latter was important back before big rsc refactor.

@gopherbot
Copy link

Change https://go.dev/cl/527319 mentions this issue: cmd/compile/internal/base: keep Ctxt.Flag_optimize in sync with Flag.N

@gopherbot
Copy link

Change https://go.dev/cl/526836 mentions this issue: dwtest: add explicit testpoint for runtime.throw

gopherbot pushed a commit that referenced this issue Sep 12, 2023
@thanm
cmd/compile/internal/base: keep Ctxt.Flag_optimize in sync with Flag.N
2399302 
This patch fixes an inconsistency in compiler flag handling introduced
accidentally in CL 521699. In the compiler we have both base.Flag.N
(which records whether the user has supplied the "-N" flag to disable
optimization) and base.Ctxt.Flag_optimize (which tracks whether
optimization is turned on). In this case Flag.N was updated without a
corresponding change to Ctxt.Flag_optimize, which led to problems with
DWARF generation for the runtime.

This CL doesn't include a regression test; a test will be added later
in the x/debug repo in a subsequent CL.

Updates #62523.

Change-Id: I0c383bb43ec0a0e7c12e7e2852c0590731416d6f
Reviewed-on: https://go-review.googlesource.com/c/go/+/527319
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thanm thanm

Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
4 participants
@aarzilli @hyangah @gopherbot @thanm