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

cmd/compile: include certain NOP instructions when compiler optimizations are disabled #20487

Open
dlsniper opened this issue May 24, 2017 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dlsniper
Copy link
Contributor

This is a follow up of the discussion here: https://github.com/derekparker/delve/issues/840

What version of Go are you using (go version)?

go version go1.8.3 windows/amd64

What did you do?

I've tried to debug the following app: https://play.golang.org/p/74ZR5IYhjn
I've set a breakpoint on line

a := TableModel{} // b demo.go:17

What did you expect to see?

I was expecting to get the debugger to stop there.

What did you see instead?

The debugger did not stopped there.

From the discussion I understand the reason on why the debugger doesn't stop as expected but as I've mentioned there, I think that's a problem of expectations.
From a user's point of view, the debugger should stop there, it's a variable declaration and initialization. There shouldn't be any reason for it not to stop.
As such, I've decided to open this issue to have a discussion and see if this could make its way in 1.10 and help manage the expectations of the users better.

Thank you.

@bradfitz bradfitz changed the title Include certain NOP instructions when compiler optimizations are disabled cmd/compile: include certain NOP instructions when compiler optimizations are disabled May 24, 2017
@bradfitz bradfitz added this to the Go1.10 milestone May 24, 2017
@slrz
Copy link

slrz commented May 26, 2017

Alessandro's suggestion (from the delve issue) to do what GDB does and put the breakpoint at the next possible instruction seems like a sensible solution and doesn't require any compiler changes to implement.

@dlsniper
Copy link
Contributor Author

I agree, it's one possible solution.
But as I've said, it's about management of expectations. People used to GDB, and other languages that have similar "issues", might be used to this but they are not all people that use debuggers.
I'm happy to discuss this and further get to understand how it's best solved.

@ianlancetaylor
Copy link
Contributor

For the record, GCC at -O0 emits a nop for an empty instruction specifically so that people can set a breakpoint on that line. In other words, what this issue is requesting.

@dlsniper
Copy link
Contributor Author

Could this please get the debugging label? Thank you.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 18, 2017
@heschi
Copy link
Contributor

heschi commented Oct 19, 2017

I'm ambivalent about this. I understand the desire, but I don't think it's going to be easy to get the compiler to emit an instruction for every source line.

In the specific example given, the declaration does generate an SSA value, a VarDef, that survives to the end. It would be simple enough to generate a NOP for those. But if you do a simple transformation and change

a := TableModel{}

to

var a TableModel 
a = TableModel{}

then in the end you get a VarDef for the first line and nothing for the second. The values are eliminated by the few optimization passes that still run with -N.

A few options I see:

  • As above, generate NOPs for whatever happens to make it through optimization. This will change over time, probably only getting smaller, as the compiler gets better. But we can at least cover VarDefs.

  • Have -N disable even more optimizations. I don't have a solid understanding of how hard this would be. Decomposition seems to be required for some reason, but a bunch of the generic rules could probably go.

    {name: "decompose user", fn: decomposeUser, required: true},
    {name: "opt", fn: opt, required: true}, // TODO: split required rules and optimizing rules
    {name: "zero arg cse", fn: zcse, required: true}, // required to merge OpSB values
    {name: "opt deadcode", fn: deadcode, required: true}, // remove any blocks orphaned during opt
    {name: "generic cse", fn: cse},
    {name: "phiopt", fn: phiopt},
    {name: "nilcheckelim", fn: nilcheckelim},
    {name: "prove", fn: prove},
    {name: "loopbce", fn: loopbce},
    {name: "decompose builtin", fn: decomposeBuiltIn, required: true},
    {name: "dec", fn: dec, required: true},
    {name: "late opt", fn: opt, required: true}, // TODO: split required rules and optimizing rules

    If CSE is required, it might not be possible to get instructions for everything.

  • Synthesize NOPs at each line transition. Easy enough, but silly; you'd end up having to step through blank lines and curly braces. Doesn't make sense.

Separately, and this is just personal opinion, as a debugger user I think I'd actually prefer not to have the NOPs; if a line of code genuinely doesn't do anything, why should I have to step through it? But maybe that's just because I understand compilation better than average.

cc @aarzilli @derekparker @dr2chase

@dr2chase
Copy link
Contributor

I would view this not as "disable optimization" but instead "remember the original structure of the code and insert missing NOPs at the very end". I.e., remember every non-blank line, figure out if the blank line represents the tail of a block or the head of a block, and emit properly numbered NOPs attached to the head/tail of the correct block. Not sure this can happen for 1.10 because we have bigger fish to fry, but if someone wants to try to figure out how to do this in the next week maybe I can help. Information might also be missing by the time we get to SSA, too.

@dlsniper
Copy link
Contributor Author

dlsniper commented Nov 6, 2017

I would view this not as "disable optimization" but instead "remember the original structure of the code and insert missing NOPs at the very end". I.e., remember every non-blank line, figure out if the blank line

I believe that it would be sufficient to have this only for the original purpose, mainly the assignment cases, as this is the most frequent issue that I see reported / talking with people. Having all blank lines might be a bit too much in terms of usage since it it's pretty well understood that an empty line is an empty line in Go or other language thus people avoid putting breakpoints there (we might also be able to control this from the application side and prevent the user to set breakpoints on truly empty lines).

Unfortunately this looks as a pretty complex task for someone with 0 knowledge of this Go subsystem to contribute to it, the best I can do is try and report how users interact with their debugger and Go code.

Hope this helps.
Thank you.

@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@aarzilli
Copy link
Contributor

One possible way to do this would be to change gc.(*state).stmt to start a new block every time there is a line change, when compiling with -N, since downstream code takes care of always emitting JMPs for empty blocks (on -N builds). @heschik notes that this might make the compiler too slow (even for -N) maybe will make -N and optimized builds diverge too much.

@griesemer griesemer modified the milestones: Go1.11, Go1.12 Jun 27, 2018
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Triage Backlog
Development

No branches or pull requests