rf icon indicating copy to clipboard operation
rf copied to clipboard
verified publisher icon rsc

rf: GoCompiledFiles is empty when running rf on the runtime package

Open mknyszek opened this issue 4 years ago • 4 comments

When trying to run rf on the runtime package I got a bunch of failures. I dug down and noticed that GoCompiledFiles (a field that appears in the JSON output of go list when -compiled is passed) is empty when trying to use rf in std.

I worked around this with the following patch:

diff --git a/refactor/snap.go b/refactor/snap.go
index fe543ed..ae0ffbe 100644
--- a/refactor/snap.go
+++ b/refactor/snap.go
@@ -307,7 +307,22 @@ func (r *Refactor) Load() (*Snapshot, error) {

                // Set up for loading from source code.
                p.Export = "" // Remember NOT to load from export data.
-               for _, name := range jp.CompiledGoFiles {
+               files := jp.CompiledGoFiles
+               lowLevelPkgs := map[string]struct{}{
+                       "runtime/internal/math": struct{}{},
+                       "internal/bytealg":      struct{}{},
+                       "runtime":               struct{}{},
+               }
+               if _, ok := lowLevelPkgs[p.PkgPath]; ok {
+                       // For any number of reasons, `go list` doesn't
+                       // actually return a populated CompiledGoFiles
+                       // for any of these packages, preventing anything
+                       // useful from being done. Use GoFiles because
+                       // it's already filtered out all the right arch-specific
+                       // files and that's good enough for the runtime.
+                       files = jp.GoFiles
+               }
+               for _, name := range files {
                        if strings.HasSuffix(name, ".s") { // surprise!
                                continue
                        }

AFAICT GoFiles does already filter by GOOS and GOARCH, so I'm not totally sure what the benefit of using GoCompiledFiles over GoFiles is in general.

@prattmic noted that this problem disappears if the -export flag is removed, but then he encountered

../crypto/ecdsa/ecdsa.go:123:19: cannot use r (variable of type *big.Int) as *big.Int value in argument to b.AddASN1BigInt
../crypto/ecdsa/ecdsa.go:124:19: cannot use s (variable of type *big.Int) as *big.Int value in argument to b.AddASN1BigInt
rf: errors found after script

when trying to run rf on the runtime. Not sure yet what's going wrong there. I'll dig deeper a little deeper later, but first I wanted to just get this down in writing.

mknyszek avatar Apr 01 '21 22:04 mknyszek

Some observations I've made:

CompiledGoFiles works fine for my system-install GOROOT:

$ go list -export -compiled -f '{{.CompiledGoFiles}}' runtime
[alg.go arena.go atomic_pointer.go cgo.go cgo_mmap.go cgo_sigaction.go cgocall.go cgocallback.go cgocheck.go chan.go checkptr.go compiler.go complex.go corehelper_linux.go cpuflags.go cpuflags_amd64.go cpuprof.go cputicks.go debug.go debugcall.go debuglog.go debuglog_off.go defs_linux_amd64.go env_posix.go error.go extern.go fastlog2.go fastlog2table.go float.go googleexit.go hash64.go heapdump.go histogram.go iface.go lfstack.go lfstack_64bit.go lock_futex.go lockrank.go lockrank_off.go malloc.go map.go map_fast32.go map_fast64.go map_faststr.go mbarrier.go mbitmap.go mcache.go mcentral.go mcheckmark.go mem_linux.go metrics.go mfinal.go mfixalloc.go mgc.go mgcmark.go mgcscavenge.go mgcstack.go mgcsweep.go mgcwork.go mheap.go mpagealloc.go mpagealloc_64bit.go mpagecache.go mpallocbits.go mprof.go mranges.go msan0.go msize.go mspanset.go mstats.go mwbbuf.go nbpipe_pipe2.go netpoll.go netpoll_epoll.go os_linux.go os_linux_generic.go os_linux_noauxv.go os_linux_x86.go os_nonopenbsd.go panic.go plugin.go preempt.go preempt_nonwindows.go print.go proc.go profbg.go profbuf.go proflabel.go pstats.go race0.go rdebug.go relax_stub.go runtime.go runtime1.go runtime2.go runtime_boring.go rwmutex.go select.go sema.go signal_amd64.go signal_linux_amd64.go signal_unix.go sigqueue.go sigqueue_note.go sigtab_linux_generic.go sizeclasses.go slice.go softfloat64.go stack.go string.go stubs.go stubs2.go stubs3.go stubs_amd64.go stubs_linux.go symtab.go sys_nonppc64x.go sys_x86.go time.go time_nofake.go timestub.go timestub2.go trace.go traceback.go type.go typekind.go utf8.go vdso_elf64.go vdso_linux.go vdso_linux_amd64.go write_err.go asm.s asm_amd64.s duff_amd64.s memclr_amd64.s memmove_amd64.s preempt_amd64.s rt0_linux_amd64.s sys_linux_amd64.s]

But does not work when set to my dev repo (the one I want to refactor):

$ GOROOT=~/src/go go list -export -compiled -f '{{.CompiledGoFiles}}' runtime
# internal/abi
compile: version "devel +87c6fa4f47 Wed Mar 31 20:28:39 2021 +0000" does not match go tool version "go1.16.2"
# runtime/internal/sys
compile: version "devel +87c6fa4f47 Wed Mar 31 20:28:39 2021 +0000" does not match go tool version "go1.16.2"
# internal/cpu
compile: version "devel +87c6fa4f47 Wed Mar 31 20:28:39 2021 +0000" does not match go tool version "go1.16.2"
# runtime/internal/atomic
compile: version "devel +87c6fa4f47 Wed Mar 31 20:28:39 2021 +0000" does not match go tool version "go1.16.2"
[]

But does work again if I also use the go tool from that dev GOROOT:

$ GOROOT=~/src/go ~/src/go/bin/go list -export -compiled -f '{{.CompiledGoFiles}}' runtime
[alg.go atomic_pointer.go cgo.go cgo_mmap.go cgo_sigaction.go cgocall.go cgocallback.go cgocheck.go chan.go checkptr.go compiler.go complex.go cpuflags.go cpuflags_amd64.go cpuprof.go cputicks.go debug.go debugcall.go debuglog.go debuglog_off.go defs_linux_amd64.go env_posix.go error.go extern.go fastlog2.go fastlog2table.go float.go hash64.go heapdump.go histogram.go iface.go lfstack.go lfstack_64bit.go lock_futex.go lockrank.go lockrank_off.go malloc.go map.go map_fast32.go map_fast64.go map_faststr.go mbarrier.go mbitmap.go mcache.go mcentral.go mcheckmark.go mem_linux.go metrics.go mfinal.go mfixalloc.go mgc.go mgcmark.go mgcscavenge.go mgcstack.go mgcsweep.go mgcwork.go mheap.go mpagealloc.go mpagealloc_64bit.go mpagecache.go mpallocbits.go mprof.go mranges.go msan0.go msize.go mspanset.go mstats.go mwbbuf.go nbpipe_pipe2.go netpoll.go netpoll_epoll.go os_linux.go os_linux_generic.go os_linux_noauxv.go os_linux_x86.go os_nonopenbsd.go panic.go plugin.go preempt.go preempt_nonwindows.go print.go proc.go profbuf.go proflabel.go race0.go rdebug.go regabidefer_off.go relax_stub.go runtime.go runtime1.go runtime2.go rwmutex.go select.go sema.go signal_amd64.go signal_linux_amd64.go signal_unix.go sigqueue.go sigqueue_note.go sigtab_linux_generic.go sizeclasses.go slice.go softfloat64.go stack.go string.go stubs.go stubs2.go stubs3.go stubs_amd64.go stubs_linux.go symtab.go sys_nonppc64x.go sys_x86.go time.go time_nofake.go timestub.go timestub2.go tls_stub.go trace.go traceback.go type.go typekind.go utf8.go vdso_elf64.go vdso_linux.go vdso_linux_amd64.go write_err.go asm.s asm_amd64.s duff_amd64.s memclr_amd64.s memmove_amd64.s preempt_amd64.s rt0_linux_amd64.s sys_linux_amd64.s]

So maybe the answer here is that we need to temporarily add the dev go tool to PATH when running rf, or have an option to specify which go tool to use?

prattmic avatar Apr 02 '21 14:04 prattmic

That does more-or-less work:

PATH=~/src/go/bin:$PATH ~/src/rf/rf -diff 'mv findrunnable findRunnable'
diff old/runtime/proc.go new/runtime/proc.go
--- old/runtime/proc.go
+++ new/runtime/proc.go
@@ -2590,7 +2590,7 @@
 
 // Finds a runnable goroutine to execute.
 // Tries to steal from other P's, get g from local or global queue, poll network.
-func findrunnable() (gp *g, inheritTime bool) {
+func findRunnable() (gp *g, inheritTime bool) {
        _g_ := getg()
 
        // The conditions here and in handoffp must agree: if
@@ -3161,7 +3161,7 @@
                // if checkTimers added a local goroutine via goready.
        }
        if gp == nil {
-               gp, inheritTime = findrunnable() // blocks until work is available
+               gp, inheritTime = findRunnable() // blocks until work is available
        }
 
        // This thread is going to run a goroutine and is not spinning anymore,
../crypto/ecdsa/ecdsa.go:123:19: cannot use r (variable of type *big.Int) as *big.Int value in argument to b.AddASN1BigInt
../crypto/ecdsa/ecdsa.go:124:19: cannot use s (variable of type *big.Int) as *big.Int value in argument to b.AddASN1BigInt
rf: errors found after script

There are two caveats:

  1. Michael didn't mention this above, but it is also necessary to drop -test from rf's go list, otherwise it chokes on LOST runtime [internal/cpu.test] and fails type-checking.
  2. I am still getting the strange "cannot use *big.Int as *big.Int error".

prattmic avatar Apr 02 '21 14:04 prattmic

I am still getting the strange "cannot use *big.Int as *big.Int error".

I would guess what's happening is package A imports package B and they both import "math/big", but the go/types.Importer is giving them separate *types.Package instances. I'd suspect one of them is getting the version with in-package _test.go files, and the other is getting it without.

mdempsky avatar Apr 02 '21 16:04 mdempsky

I'd suspect one of them is getting the version with in-package _test.go files, and the other is getting it without.

That's https://github.com/rsc/rf/issues/9 :)

have an option to specify which go tool to use?

In general, the way for tools to use a specific Go version/install is to set up $PATH when invoking them. See https://github.com/golang/go/issues/44329, for example.

mvdan avatar Apr 02 '21 17:04 mvdan