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

Jump to bottom

cmd/compile: CL 305829 broke plan9/amd64 build #45372

Closed
fhs opened this issue Apr 3, 2021 · 18 comments
Closed

cmd/compile: CL 305829 broke plan9/amd64 build #45372

fhs opened this issue Apr 3, 2021 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9

Comments

@fhs
Copy link
Contributor

fhs commented Apr 3, 2021 •
edited

After CL 305829 (commit 3300390), the plan9/amd64 builder has been failing with this:

Building Go toolchain2 using go_bootstrap and Go toolchain1.
runtime: bad pointer in frame cmd/go/internal/load.loadImport at 0x281eec90: 0x2
fatal error: invalid pointer found on stack

See https://build.golang.org/log/62148fe37ac833f3b20c10296797908bca7a356c

It builds fine on my machine after I reverted the change.

CC @0intro

@gopherbot add labels OS-Plan9, NeedsFix
@gopherbot gopherbot added NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 labels Apr 3, 2021
@fhs
Copy link
Contributor Author

fhs commented Apr 4, 2021

CC @thanm

@thanm
Copy link
Contributor

thanm commented Apr 4, 2021

Thanks.

The change in question seems fairly benign (since according to the Go ABI0 calling convention, any scratch register can be used in such situations).

Is there a plan9 expert who could advise me as to whether there this might be breaking some sort of Plan9 rule regarding register use?

@thanm thanm self-assigned this Apr 4, 2021
@fhs
Copy link
Contributor Author

fhs commented Apr 4, 2021

CC @millerresearch

@millerresearch
Copy link
Contributor

millerresearch commented Apr 5, 2021 •
edited

CC @millerresearch

Sorry, amd64 is outside my zone of expertise, and I don't have an amd64 Plan 9 machine to try to reproduce this on.

I wonder if there is anything in the _amd64.s files in the runtime which is touching that register?

@thanm
Copy link
Contributor

thanm commented Apr 5, 2021

I wonder if there is anything in the _amd64.s files in the runtime which is touching that register?

That was the first thing I looked for. Can't see anything specific to plan9 there.

I'll poke around some more.

@thanm
Copy link
Contributor

thanm commented Apr 5, 2021

I can't seem to create a gomote to work on this problem -- maybe something is wrong with the builder?

Output from gomote create:

$ gomote create plan9-amd64-9front
# host type "host-plan9-amd64-0intro" is not elastic; 0 of 1 machines connected, 0 busy
# still creating plan9-amd64-9front after 5s; 0 requests ahead of you
# still creating plan9-amd64-9front after 10s; 0 requests ahead of you
...
# still creating plan9-amd64-9front after 15m50s; 0 requests ahead of you
# still creating plan9-amd64-9front after 15m55s; 0 requests ahead of you
# still creating plan9-amd64-9front after 16m0s; 0 requests ahead of you

Scheduler state doesn't seem to show anything else running:
Screenshot from 2021-04-05 08-54-56

@0intro, is this something you can help with? It is unlikely I'll be able to investigate this problem if I can't triage it via gomote/builder.

@0intro
Copy link
Member

0intro commented Apr 5, 2021

I can't seem to create a gomote to work on this problem -- maybe something is wrong with the builder?

It should be fixed now. Please try again.

@thanm
Copy link
Contributor

thanm commented Apr 5, 2021

Thanks, yes, "gomote create" seems to have worked now.

How exactly do we run "make.bash" on our plan9 builders? I can't seem to see a copy of bash in any of the usual places (/bin/bash, /usr/local/bin/bash, etc). Thanks.

@0intro
Copy link
Member

0intro commented Apr 5, 2021

How exactly do we run "make.bash" on our plan9 builders? I can't seem to see a copy of bash in any of the usual places (/bin/bash, /usr/local/bin/bash, etc). Thanks.

On Plan 9, it's called make.rc.

@millerresearch
Copy link
Contributor

I fired up a copy of 9legacy 9k flavour of Plan 9 on an amd64 machine and I see the same error. So it's not specific to 9front.

@0intro
Copy link
Member

0intro commented Apr 5, 2021

The builder was historically called 9front, because it used to run 9front during a year approximately, but it has been running 9legacy 9k since October 2015.

@millerresearch
Copy link
Contributor

has been running 9legacy 9k since October 2015.

Maybe time to rename it then?

@0intro
Copy link
Member

0intro commented Apr 5, 2021

Maybe time to rename it then?

Sure. I've just sent CL 307409.

@gopherbot
Copy link

Change https://golang.org/cl/307469 mentions this issue: cmd/compile: fix for zerorange on plan9-amd64

@gopherbot
Copy link

Change https://golang.org/cl/307829 mentions this issue: cmd/compile/internal/amd64: follow-on regabi fix for amd64 zerorange

@thanm
Copy link
Contributor

thanm commented Apr 6, 2021

plan9-amd64 builder seems to be hung on trybot/slowbot run... currently at 2 hours 21 mins. Maybe some sort of machine problem? Does it need to be rebooted?

Screenshot from 2021-04-06 18-55-10

@0intro
Copy link
Member

0intro commented Apr 7, 2021

It should be fixed now.

@thanm
Copy link
Contributor

thanm commented Apr 7, 2021

Thanks.

gopherbot pushed a commit that referenced this issue Apr 12, 2021
@thanm
cmd/compile/internal/amd64: follow-on regabi fix for amd64 zerorange
c26f954 
This patch provides a better long-term fix for the compiler's
zerorange() helper function to make it generate code friendly to the
register ABI.

CL 305829 did part of the work, but didn't properly handle the case
where the compiler emits a REP.STOSQ sequence; this patch changes the
REP code to make sure it doesn't clobber any incoming register
parameter values.

Also included is a test that is specifically written to trigger
the REP emit code in the compiler (prior to this, this code was
not being hit on linux/amd64 all.bash).

Updates #45372.
Updates #40724.

Change-Id: Iaf1c4e709e98eda45cd6f3aeebda0fe9160f1f42
Reviewed-on: https://go-review.googlesource.com/c/go/+/307829
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Apr 7, 2022
@rsc rsc unassigned thanm Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fhs @0intro @gopherbot @thanm @millerresearch and others