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: closure miscompilation in gopher-lua benchmark #59680

Closed
mknyszek opened this issue Apr 17, 2023 · 10 comments
Closed

cmd/compile: closure miscompilation in gopher-lua benchmark #59680

mknyszek opened this issue Apr 17, 2023 · 10 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Apr 17, 2023

CL 484860 broke the x/benchmarks builder: https://build.golang.org/?repo=golang.org%2fx%2fbenchmarks

Failure: https://build.golang.org/log/11803116ada45552b52ed62dd86f4255cdba5d87

What's happening is this call produces a closure that appears to be inlined into a compiler-generated init function. The closure it produces later gets installed here.

The end result is this function ends up dereferencing a nil pointer.

Reproducer instructions:

git clone https://go.googlesource.com/benchmarks
go build ./benchmarks/sweet/benchmarks/gopher-lua
./gopher-lua k-nucleotide.txt input.txt

k-nucleotide.txt
input.txt

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 17, 2023
@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Apr 17, 2023
@mknyszek mknyszek added this to the Go1.21 milestone Apr 17, 2023
@mknyszek
Copy link
Contributor Author

CC @golang/compiler

Perhaps specifically @thanm @mdempsky?

@mknyszek mknyszek changed the title cmd/compile: closure miscompilation in Sweet benchmarks cmd/compile: closure miscompilation in gopher-lua benchmark Apr 17, 2023
@mknyszek mknyszek removed release-blocker Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) labels Apr 17, 2023
@mknyszek
Copy link
Contributor Author

Revert: https://go.dev/cl/485498

gopherbot pushed a commit that referenced this issue Apr 17, 2023
…closures"

This reverts commit f8162a0.

Reason for revert: #59680

Change-Id: I91821c691a2d019ff0ad5b69509e32f3d56b8f67
Reviewed-on: https://go-review.googlesource.com/c/go/+/485498
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
@thanm thanm self-assigned this Apr 17, 2023
@thanm
Copy link
Contributor

thanm commented Apr 17, 2023

I'll take a look tomorrow.

@thanm
Copy link
Contributor

thanm commented Apr 17, 2023

One thing that isn't immediately clear -- we've already had two attempts for the original CL to enable more inlining of functions with closures (e.g. CL 479095. Did this particular problem not manifest the first time around? Or it was there but we just didn't pick up on it? It would be helpful to understand that get a sense for the scope of the situation.

@thanm
Copy link
Contributor

thanm commented Apr 19, 2023

I spent some tim debugging this.

The problem appears to relate to how the closure is being handled, or at least that's what it appears.

The offending function (in which crash is taking place) is golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1, which looks like this:

Dump of assembler code for function golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1:
   0x00000000004fed20 <+0>:	cmp    0x10(%r14),%rsp
   0x00000000004fed24 <+4>:	jbe    0x4fed44 <golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1+36>
   0x00000000004fed26 <+6>:	push   %rbp
   0x00000000004fed27 <+7>:	mov    %rsp,%rbp
   0x00000000004fed2a <+10>:	sub    $0x8,%rsp
   0x00000000004fed2e <+14>:	mov    0x8(%rdx),%rcx
   0x00000000004fed32 <+18>:	mov    (%rcx),%rcx
   0x00000000004fed35 <+21>:	mov    0x10(%rcx),%rax
   0x00000000004fed39 <+25>:	call   0x4fe780 <golang.org/x/benchmarks/sweet/benchmarks/internal/driver.ReadRSS>
=> 0x00000000004fed3e <+30>:	add    $0x8,%rsp
   0x00000000004fed42 <+34>:	pop    %rbp
   0x00000000004fed43 <+35>:	ret    
   0x00000000004fed44 <+36>:	call   0x465f40 <runtime.morestack>
   0x00000000004fed49 <+41>:	jmp    0x4fed20 <golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1>
End of assembler dump.

which corresponds to a source function at line 48:

            func() (uint64, error) {
		return ReadRSS(b.pid)
	}

In the assembly above, register $rdx is the closure pointer; the code loads up the value of "b" and then uses that to pick out b.pid.

I stopped in the debugger and dumped the closure pointer:

Thread 10 "lua.exe" hit Breakpoint 11, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1 (~r0=<optimized out>, ~r1=...) at /usr/local/google/home/thanm/gobugs/59680/benchmarks/sweet/benchmarks/internal/driver/driver.go:48
(gdb) x/8 $rdx
0xc000030010:	0x004fed20	0x00000000	0x0018ac98	0x000000c0
0xc000030020:	0x0059272d	0x00000000	0x0000000a	0x00000000
(gdb)

The first word is ok (points to the correct function) but the value of "b" is garbage -- it's printing as 0xc00018ac98 instead of the correct value of 0xc0001a62c0... thus the actual fault comes at the point where the code loads up "b.pid".

Unsure as to how/why the closure is going bad. Maybe an escape analysis thing?

@aclements
Copy link
Member

For reference, here's line 48, the closure that's crashing.

I think you're right this is an escape analysis issue.

At 94850c6 (just before the latest revert of this ill-fated CL), the function that constructs the closure looks like:

golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3 STEXT size=134 args=0x8 locals=0x18 funcid=0x0 align=0x0
        0x0000 00000 (…/driver.go:47) TEXT     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3(SB), ABIInter
nal, $24-8
        0x0000 00000 (…/driver.go:47) CMPQ     SP, 16(R14)
        0x0004 00004 (…/driver.go:47) PCDATA   $0, $-2
        0x0004 00004 (…/driver.go:47) JLS      114
        0x0006 00006 (…/driver.go:47) PCDATA   $0, $-1
        0x0006 00006 (…/driver.go:47) PUSHQ    BP
        0x0007 00007 (…/driver.go:47) MOVQ     SP, BP
        0x000a 00010 (…/driver.go:47) SUBQ     $16, SP
        0x000e 00014 (…/driver.go:47) FUNCDATA $0, gclocals·wgcWObbY2HYnK2SU/U22lA==(SB)
        0x000e 00014 (…/driver.go:47) FUNCDATA $1, gclocals·J5F+7Qw7O7ve2QcWC7DpeQ==(SB)
        0x000e 00014 (…/driver.go:47) FUNCDATA $2, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.stkobj(SB
)
        0x000e 00014 (…/driver.go:47) FUNCDATA $5, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.arginfo1(
SB)
        0x000e 00014 (…/driver.go:47) MOVQ     AX, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP)
        0x0013 00019 (…/driver.go:48) LEAQ     type:noalg.struct { F uintptr; golang.org/x/benchmarks/sweet/benchmarks/internal/
driver.b **golang.org/x/benchmarks/sweet/benchmarks/internal/driver.B }(SB), AX
        0x001a 00026 (…/driver.go:48) PCDATA   $1, $0
        0x001a 00026 (…/driver.go:48) CALL     runtime.newobject(SB)
        0x001f 00031 (…/driver.go:48) LEAQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1(SB), CX
        0x0026 00038 (…/driver.go:48) MOVQ     CX, (AX)
        0x0029 00041 (…/driver.go:48) PCDATA   $0, $-2
        0x0029 00041 (…/driver.go:48) CMPL     runtime.writeBarrier(SB), $0
        0x0030 00048 (…/driver.go:48) JEQ      63
        0x0032 00050 (…/driver.go:48) CALL     runtime.gcWriteBarrier1(SB)
        0x0037 00055 (…/driver.go:48) LEAQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP), CX
        0x003c 00060 (…/driver.go:48) MOVQ     CX, (R11)
        0x003f 00063 (…/driver.go:48) LEAQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP), CX
        0x0044 00068 (…/driver.go:48) MOVQ     CX, 8(AX)
        0x0048 00072 (…/driver.go:48) PCDATA   $0, $-1
        0x0048 00072 (…/driver.go:48) MOVQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP), CX
        0x004d 00077 (…/driver.go:48) TESTB    AL, (CX)
        0x004f 00079 (…/driver.go:48) PCDATA   $0, $-2
        0x004f 00079 (…/driver.go:48) CMPL     runtime.writeBarrier(SB), $0
        0x0056 00086 (…/driver.go:48) JEQ      104
        0x0058 00088 (…/driver.go:48) CALL     runtime.gcWriteBarrier2(SB)
        0x005d 00093 (…/driver.go:48) MOVQ     AX, (R11)
        0x0060 00096 (…/driver.go:48) MOVQ     88(CX), DX
        0x0064 00100 (…/driver.go:48) MOVQ     DX, 8(R11)
        0x0068 00104 (…/driver.go:48) MOVQ     AX, 88(CX)
        0x006c 00108 (…/driver.go:51) ADDQ     $16, SP
        0x0070 00112 (…/driver.go:51) POPQ     BP
        0x0071 00113 (…/driver.go:51) RET
        0x0072 00114 (…/driver.go:51) NOP
        0x0072 00114 (…/driver.go:47) PCDATA $1, $-1
        0x0072 00114 (…/driver.go:47) PCDATA $0, $-2
        0x0072 00114 (…/driver.go:47) MOVQ   AX, 8(SP)
        0x0077 00119 (…/driver.go:47) CALL   runtime.morestack_noctxt(SB)
        0x007c 00124 (…/driver.go:47) MOVQ   8(SP), AX
        0x0081 00129 (…/driver.go:47) PCDATA $0, $-1
        0x0081 00129 (…/driver.go:47) JMP    0
        0x0000 49 3b 66 10 76 6c 55 48 89 e5 48 83 ec 10 48 89  I;f.vlUH..H...H.
        0x0010 44 24 20 48 8d 05 00 00 00 00 e8 00 00 00 00 48  D$ H...........H
        0x0020 8d 0d 00 00 00 00 48 89 08 83 3d 00 00 00 00 00  ......H...=.....
        0x0030 74 0d e8 00 00 00 00 48 8d 4c 24 20 49 89 0b 48  t......H.L$ I..H
        0x0040 8d 4c 24 20 48 89 48 08 48 8b 4c 24 20 84 01 83  .L$ H.H.H.L$ ...
        0x0050 3d 00 00 00 00 00 74 10 e8 00 00 00 00 49 89 03  =.....t......I..
        0x0060 48 8b 51 58 49 89 53 08 48 89 41 58 48 83 c4 10  H.QXI.S.H.AXH...
        0x0070 5d c3 48 89 44 24 08 e8 00 00 00 00 48 8b 44 24  ].H.D$......H.D$
        0x0080 08 e9 7a ff ff ff                                ..z...
        rel 22+4 t=14 type:noalg.struct { F uintptr; golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b **golang.org/x/benchmarks/sweet/benchmarks/internal/driver.B }+0
        rel 27+4 t=7 runtime.newobject+0
        rel 34+4 t=14 golang.org/x/benchmarks/sweet/benchmarks/internal/driver.init.func3.1+0
        rel 43+4 t=14 runtime.writeBarrier+-1
        rel 51+4 t=7 runtime.gcWriteBarrier1+0
        rel 81+4 t=14 runtime.writeBarrier+-1
        rel 89+4 t=7 runtime.gcWriteBarrier2+0
        rel 120+4 t=7 runtime.morestack_noctxt+0

The closure itself is being heap allocated (+0x001a). But, if I'm reading this right, we're putting a stack address (&b, copied from the argument to the stack at +0x000e) into the second word of that closure (+0x003f and +0x0044). Since the closure clearly escapes the function, that address is going to go bad. (I'm not sure why we're putting &b in rather than just copying b.)

This only happens in the version of DoDefaultAvgRSS.func1 that gets inlined into init. The non-inlined version is fine:

golang.org/x/benchmarks/sweet/benchmarks/internal/driver.DoDefaultAvgRSS.func1 STEXT size=133 args=0x8 locals=0x18 funcid=0x0 align=0x0
        0x0000 00000 (…/driver.go:47) TEXT     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.DoDefaultAvgRSS.func1(SB), ABIInternal, $24-8
        0x0000 00000 (…/driver.go:47) CMPQ     SP, 16(R14)
        0x0004 00004 (…/driver.go:47) PCDATA   $0, $-2
        0x0004 00004 (…/driver.go:47) JLS      111
        0x0006 00006 (…/driver.go:47) PCDATA   $0, $-1
        0x0006 00006 (…/driver.go:47) PUSHQ    BP
        0x0007 00007 (…/driver.go:47) MOVQ     SP, BP
        0x000a 00010 (…/driver.go:47) SUBQ     $16, SP
        0x000e 00014 (…/driver.go:47) FUNCDATA $0, gclocals·wgcWObbY2HYnK2SU/U22lA==(SB)
        0x000e 00014 (…/driver.go:47) FUNCDATA $1, gclocals·J5F+7Qw7O7ve2QcWC7DpeQ==(SB)
        0x000e 00014 (…/driver.go:47) FUNCDATA $5, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.DoDefaultAvgRSS.func1.arginfo1(SB)
        0x000e 00014 (…/driver.go:47) FUNCDATA $6, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.DoDefaultAvgRSS.func1.argliveinfo(SB)
        0x000e 00014 (…/driver.go:47) PCDATA   $3, $1
        0x000e 00014 (…/driver.go:47) MOVQ     AX, golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP)
        0x0013 00019 (…/driver.go:47) PCDATA   $3, $-1
        0x0013 00019 (…/driver.go:48) LEAQ     type:noalg.struct { F uintptr; golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b *golang.org/x/benchmarks/sweet/benchmarks/internal/driver.B }(SB), AX
        0x001a 00026 (…/driver.go:48) PCDATA   $1, $0
        0x001a 00026 (…/driver.go:48) CALL     runtime.newobject(SB)
        0x001f 00031 (…/driver.go:48) LEAQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.DoDefaultAvgRSS.func1.1(SB), CX
        0x0026 00038 (…/driver.go:48) MOVQ     CX, (AX)
        0x0029 00041 (…/driver.go:48) PCDATA   $0, $-2
        0x0029 00041 (…/driver.go:48) CMPL     runtime.writeBarrier(SB), $0
        0x0030 00048 (…/driver.go:48) JNE      57
        0x0032 00050 (…/driver.go:48) MOVQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP), CX
        0x0037 00055 (…/driver.go:48) JMP      70
        0x0039 00057 (…/driver.go:48) CALL     runtime.gcWriteBarrier1(SB)
        0x003e 00062 (…/driver.go:48) MOVQ     golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b+32(SP), CX
        0x0043 00067 (…/driver.go:48) MOVQ     CX, (R11)
        0x0046 00070 (…/driver.go:48) MOVQ     CX, 8(AX)
        0x004a 00074 (…/driver.go:48) PCDATA   $0, $-1
        0x004a 00074 (…/driver.go:48) TESTB    AL, (CX)
        0x004c 00076 (…/driver.go:48) PCDATA   $0, $-2
        0x004c 00076 (…/driver.go:48) CMPL     runtime.writeBarrier(SB), $0
        0x0053 00083 (…/driver.go:48) JEQ      101
        0x0055 00085 (…/driver.go:48) CALL     runtime.gcWriteBarrier2(SB)
        0x005a 00090 (…/driver.go:48) MOVQ     AX, (R11)
        0x005d 00093 (…/driver.go:48) MOVQ     88(CX), DX
        0x0061 00097 (…/driver.go:48) MOVQ     DX, 8(R11)
        0x0065 00101 (…/driver.go:48) MOVQ     AX, 88(CX)
        0x0069 00105 (…/driver.go:51) ADDQ     $16, SP
        0x006d 00109 (…/driver.go:51) POPQ     BP
        0x006e 00110 (…/driver.go:51) RET
        0x006f 00111 (…/driver.go:51) NOP
        0x006f 00111 (…/driver.go:47) PCDATA $1, $-1
        0x006f 00111 (…/driver.go:47) PCDATA $0, $-2
        0x006f 00111 (…/driver.go:47) MOVQ   AX, 8(SP)
        0x0074 00116 (…/driver.go:47) CALL   runtime.morestack_noctxt(SB)
        0x0079 00121 (…/driver.go:47) MOVQ   8(SP), AX
        0x007e 00126 (…/driver.go:47) PCDATA $0, $-1
        0x007e 00126 (…/driver.go:47) NOP
        0x0080 00128 (…/driver.go:47) JMP 0
        0x0000 49 3b 66 10 76 69 55 48 89 e5 48 83 ec 10 48 89  I;f.viUH..H...H.
        0x0010 44 24 20 48 8d 05 00 00 00 00 e8 00 00 00 00 48  D$ H...........H
        0x0020 8d 0d 00 00 00 00 48 89 08 83 3d 00 00 00 00 00  ......H...=.....
        0x0030 75 07 48 8b 4c 24 20 eb 0d e8 00 00 00 00 48 8b  u.H.L$ .......H.
        0x0040 4c 24 20 49 89 0b 48 89 48 08 84 01 83 3d 00 00  L$ I..H.H....=..
        0x0050 00 00 00 74 10 e8 00 00 00 00 49 89 03 48 8b 51  ...t......I..H.Q
        0x0060 58 49 89 53 08 48 89 41 58 48 83 c4 10 5d c3 48  XI.S.H.AXH...].H
        0x0070 89 44 24 08 e8 00 00 00 00 48 8b 44 24 08 66 90  .D$......H.D$.f.
        0x0080 e9 7b ff ff ff                                   .{...
        rel 22+4 t=14 type:noalg.struct { F uintptr; golang.org/x/benchmarks/sweet/benchmarks/internal/driver.b *golang.org/x/benchmarks/sweet/benchmarks/internal/driver.B }+0
        rel 27+4 t=7 runtime.newobject+0
        rel 34+4 t=14 golang.org/x/benchmarks/sweet/benchmarks/internal/driver.DoDefaultAvgRSS.func1.1+0
        rel 43+4 t=14 runtime.writeBarrier+-1
        rel 58+4 t=7 runtime.gcWriteBarrier1+0
        rel 78+4 t=14 runtime.writeBarrier+-1
        rel 86+4 t=7 runtime.gcWriteBarrier2+0
        rel 117+4 t=7 runtime.morestack_noctxt+0

Here we can see that we copied b directly into the closure (not &b), at +0x003e and +0x0046.

If we pull in the revert (ce10e9d), DoDefaultAvgRSS is no longer being inlined into init at all, so we call the non-inlined copy and everything is fine.

@aclements
Copy link
Member

cc @mdempsky and @cherrymui since this involves escape analysis.

@thanm
Copy link
Contributor

thanm commented May 3, 2023

Here is a single-file reproducer:

https://go.dev/play/p/6IkaUaWvYwD

My CL stack that rolls forward the closure inlining change:

http://go.dev/cl/c/go/+/492017

With this test case and the CL stack above, the crashing stack trace in GDB is:

#0  0x0000000000464eb5 in main.init.func1.1 (~r0=<optimized out>, ~r1=...) at issue59680/repro.go:36
#1  0x0000000000464b78 in main.(*B).startit.func1 () at issue59680/repro.go:53
#2  0x0000000000456581 in runtime.goexit () at runtime/asm_amd64.s:1650
#3  0x0000000000000000 in ?? ()

I did a build of the testcase with "gcflags=-m=3 -W=3 -S". What is surprising about the output there is that the offending function definitely turns up in walk/order and then the assembly output:

main.init.func1.1 STEXT size=46 args=0x0 locals=0x10 funcid=0x0 align=0x0
	0x0000 00000 (issue59680/repro.go:35)	TEXT	main.init.func1.1(SB), NEEDCTXT|ABIInternal, $16-0
...

however it never seems to be touched by escape analysis. I added some debugging code here and from what I can tell it never actually gets visited, which might explain the problem.

@thanm
Copy link
Contributor

thanm commented May 3, 2023

OK, I think I see the issue here. It's a pkginit problem. Before we run pkginit.Task(), the main init function looks like

.   DCLFUNC main.init Closgen:1 Label:1 ABI:ABIInternal InlinabilityChecked IsPackageInit FUNC-func() tc(1) # repro.go:64:5
.   DCLFUNC-Dcl
.   .   NAME-main.~R0 Class:PAUTO Offset:0 InlFormal OnStack Used main.RO tc(1) # repro.go:33:12,repro.go:30:5
.   DCLFUNC-body
.   .   AS tc(1) # repro.go:64:5
.   .   .   NAME-main.S Class:PEXTERN Offset:0 Used int tc(1) # repro.go:64:5
.   .   AS tc(1) # repro.go:29:5
.   .   .   NAME-main.ROSL Class:PEXTERN Offset:0 Used SLICE-[]RO tc(1) # repro.go:29:5
.   .   .   SLICELIT Len:1 SLICE-[]RO tc(1) # repro.go:29:16
.   .   .   SLICELIT-List
.   .   .   .   INLCALL-init
.   .   .   .   .   AS2 Def tc(1) # repro.go:30:5
.   .   .   .   .   INLMARK Index:2 # +repro.go:30:5
.   .   .   .   INLCALL main.RO tc(1) # repro.go:30:5
.   .   .   .   INLCALL-Body
.   .   .   .   .   BLOCK tc(1) # repro.go:30:5
.   .   .   .   .   BLOCK-List
.   .   .   .   .   .   DCL tc(1) # repro.go:30:5
.   .   .   .   .   .   .   NAME-main.~R0 Class:PAUTO Offset:0 InlFormal OnStack Used main.RO tc(1) # repro.go:33:12,repro.go:30:5
.   .   .   .   .   .   AS2 tc(1) # repro.go:30:5
.   .   .   .   .   .   AS2-Lhs
.   .   .   .   .   .   .   NAME-main.~R0 Class:PAUTO Offset:0 InlFormal OnStack Used main.RO tc(1) # repro.go:33:12,repro.go:30:5
.   .   .   .   .   .   AS2-Rhs
.   .   .   .   .   .   .   CONVNOP Implicit main.RO tc(1) # repro.go:34:9,repro.go:30:5
.   .   .   .   .   .   .   .   CLOSURE fnName(main.init.func1) FUNC-func(*B) tc(1) # repro.go:34:9,repro.go:30:5
.   .   .   .   .   .   .   .   CLOSURE-Func
.   .   .   .   .   .   .   .   .   DCLFUNC main.init.func1 Closgen:1 ABI:ABIInternal IsHiddenClosure FUNC-func(*B) tc(1) # repro.go:34:9
...

but when pkginit.Task is done, the init function is basically empty since the slice can be initialized entirely statically.

I think what's needed here is something in pkginit that will detect this case and mark the closures as no longer hidden. I'll work on a CL for that.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/492135 mentions this issue: cmd/compile: un-hide closure func if parent expr moved to staticinit

@thanm thanm closed this as completed May 5, 2023
gopherbot pushed a commit that referenced this issue May 5, 2023
If the function referenced by a closure expression is incorporated
into a static init, be sure to mark it as non-hidden, since otherwise
it will be live but no longer reachable from the init func, hence it
will be skipped during escape analysis, which can lead to
miscompilations.

Fixes #59680.

Change-Id: Ib858aee296efcc0b7655d25c23ab8a6a8dbdc5f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/492135
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 5, 2023
@golang golang locked and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants