-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
/cc @eliasnaur @rasky |
Disabling branchelim pass on amd64 breaks a bunch of codegen tests. Should we revert CL for now? |
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? |
I remember that for some broken optimization, instead of reverting commit, we added something like return // disabled until issue... is fixed |
cc @randall77 @cherrymui for decision. |
The default is that the offending CL should be rolled back. 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. |
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. |
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? |
"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.
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. |
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. |
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. |
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.
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. |
Reported at https://go-review.googlesource.com/c/go/+/98695
go get -u golang.org/x/exp/shiny/iconvg
results in
on master.
It was bisected to CL 98695
The text was updated successfully, but these errors were encountered: