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: branchelim broke golang.org/x/exp/shiny/ on amd64 #24395

Closed
TocarIP opened this issue Mar 14, 2018 · 12 comments
Closed

cmd/compile: branchelim broke golang.org/x/exp/shiny/ on amd64 #24395

TocarIP opened this issue Mar 14, 2018 · 12 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@TocarIP
Copy link
Contributor

TocarIP commented Mar 14, 2018

Reported at https://go-review.googlesource.com/c/go/+/98695

go get -u golang.org/x/exp/shiny/iconvg

results in

../../gopath/src/golang.org/x/exp/shiny/iconvg/buffer.go:190:19: internal compiler error: Flag* ops should never make it to codegen v16 = FlagLT_ULT <flags>

goroutine 4 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
        /localdisk/itocar/mainline_golang/go/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0xc5409c, 0x2c, 0xc001096670, 0x1, 0x1)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/gc/subr.go:182 +0x1f7
cmd/compile/internal/gc.(*ssafn).Fatalf(0xc0007cdbf0, 0xae2300000194, 0xc5409c, 0x2c, 0xc001096670, 0x1, 0x1)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/gc/ssa.go:5326 +0x67
cmd/compile/internal/ssa.(*Value).Fatalf(0xc00092c700, 0xc5409c, 0x2c, 0xc001096670, 0x1, 0x1)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/ssa/value.go:268 +0x77
cmd/compile/internal/amd64.ssaGenValue(0xc001154540, 0xc00092c700)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/amd64/ssa.go:1029 +0x4dca
cmd/compile/internal/gc.genssa(0xc000b7b400, 0xc0003960a0)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/gc/ssa.go:4681 +0x425
cmd/compile/internal/gc.compileSSA(0xc0003a1500, 0x0)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/gc/pgen.go:267 +0x176
cmd/compile/internal/gc.compileFunctions.func2(0xc00091afc0, 0xc0003c3f30, 0x0)
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/gc/pgen.go:309 +0x49
created by cmd/compile/internal/gc.compileFunctions
        /localdisk/itocar/mainline_golang/go/src/cmd/compile/internal/gc/pgen.go:307 +0x11a

on master.
It was bisected to CL 98695

@TocarIP
Copy link
Contributor Author

TocarIP commented Mar 14, 2018

/cc @eliasnaur @rasky

@TocarIP
Copy link
Contributor Author

TocarIP commented Mar 14, 2018

Disabling branchelim pass on amd64 breaks a bunch of codegen tests. Should we revert CL for now?

@ALTree
Copy link
Member

ALTree commented Mar 14, 2018

If a CL broke branchelim I would revert the CL, not disable branchelim. Or what you mean is that the CL is actually correct but it exposed a latent bug in branchelim, so it's branchelim that is at fault and it should be disabled?

@TocarIP
Copy link
Contributor Author

TocarIP commented Mar 14, 2018

I remember that for some broken optimization, instead of reverting commit, we added something like

return // disabled until issue... is fixed

@ALTree
Copy link
Member

ALTree commented Mar 14, 2018

cc @randall77 @cherrymui for decision.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 14, 2018
@randall77
Copy link
Contributor

The default is that the offending CL should be rolled back.
If you know what the problem is and can fix it promptly, that's ok also.
Generally we shouldn't be disabling entire passes because a CL broke that pass.

If there is a bug in a pass which is only triggered when the CL goes in, that's the CL's fault. Not fair, maybe, but necessary to keep the build green.

If there is a bug in a pass which the CL revealed, but the bug can trigger even if the CL is reverted, then disabling the pass is probably the right move.

@bradfitz bradfitz added this to the Go1.11 milestone Mar 15, 2018
@ALTree
Copy link
Member

ALTree commented Mar 15, 2018

The CL was reverted (see 100755), and there is no evidence that the whole pass is at fault, so I think we can close this.

@ALTree ALTree closed this as completed Mar 15, 2018
@rasky
Copy link
Member

rasky commented Mar 15, 2018

Sorry for that, I'll look into it.

Does breaking shiny qualify as "breaking the build"? What should I test in future to make sure that, if it passes, the CL doesn't get reverted but instead a bug is opened for followup fixes?

@ALTree
Copy link
Member

ALTree commented Mar 15, 2018

"Breaking the build" in general means builders (https://build.golang.org) failures... but for codegen CLs the compiler crashing on any kind of valid Go code is pretty much "breaking the build", I fear.

make sure that [..] the CL doesn't get reverted but instead a bug is opened for followup fixes

Compiler crashes on any valid Go code almost always get reverted, unless the issue is obvious and can be fixed immediately in a follow-up CL.

@rasky
Copy link
Member

rasky commented Mar 15, 2018

Compiler crashes on any valid Go code almost always get reverted

That's quite aggressive, especially since we're very early in the development cycle. The issue was obvious to me in this case. Anyway, I just mailed the CL (https://go-review.googlesource.com/#/c/100935/).

BTW, for everybody's entertainment, this bug actually highlighted a bug in the shiny's code :) I've sent a CL for that as well.

@ALTree
Copy link
Member

ALTree commented Mar 15, 2018

That's quite aggressive, especially since we're very early in the development cycle.

The problem is that there are many people working on the repository daily, and no one wants to work with a compiler that is known to be subtly broken. Even more so if you consider that the tip compiler compiles itself and then everything else in the repository. That's why CLs that have demonstrably broken the compiler usually get reverted very quickly.

@randall77
Copy link
Contributor

Does breaking shiny qualify as "breaking the build"? What should I test in future to make sure that, if it passes, the CL doesn't get reverted but instead a bug is opened for followup fixes?

Unfortunately, there's no easy way to detect this beforehand, as shiny isn't compiled by all.bash. Our test suite will never be perfect, and as in this case bugs will be detected post-checkin through some ad-hoc process.

That's quite aggressive, especially since we're very early in the development cycle. The issue was obvious to me in this case. Anyway, I just mailed the CL (https://go-review.googlesource.com/#/c/100935/).

If you can make a fix in a timely manner, that's ok also. You just have to beat the reverters to it. It's ok to say "hold up on that revert, I have a fix coming in the next hour".

Don't get too hung up on having to revert. It happens to the best of us. Lord knows I've reverted quite a few of my changes. It's a bit more work but it reduces the window where someone else synced to a broken compiler, tried to do some work, and got failures than ended up unrelated to the work they were doing.

@golang golang locked and limited conversation to collaborators Mar 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants