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

Comments

Projects
None yet
9 participants
@dlsniper
Contributor

dlsniper commented May 24, 2017

This is a follow up of the discussion here: derekparker/delve#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 from Include certain NOP instructions when compiler optimizations are disabled to 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

This comment has been minimized.

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

This comment has been minimized.

Contributor

dlsniper commented May 26, 2017

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

This comment has been minimized.

Contributor

ianlancetaylor commented May 26, 2017

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

This comment has been minimized.

Contributor

dlsniper commented Jul 17, 2017

Could this please get the debugging label? Thank you.

@heschik

This comment has been minimized.

Contributor

heschik 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

This comment has been minimized.

Contributor

dr2chase commented Oct 20, 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 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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

aarzilli commented Nov 30, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment