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: no instruction marked with is_stmt in debug_line for statement calling generic function, inside a generic function #49628

Closed
aarzilli opened this issue Nov 17, 2021 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@aarzilli
Copy link
Contributor

go version devel go1.18-1d004fa201 Wed Nov 17 04:55:12 2021 +0000 linux/amd64
package main

import "fmt"

func F[T any](n T) {
	fmt.Printf("called\n")
}

func G[T any](n T) {
	F(n)
	fmt.Printf("after\n")
}

func main() {
	G(3)
}

The instantiation of main.G has no instruction marked is_stmt for the call to main.F.

@aarzilli
Copy link
Contributor Author

cc @dr2chase @danscales

@dr2chase dr2chase self-assigned this Nov 17, 2021
@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 17, 2021
@danscales
Copy link
Contributor

@dr2chase Thanks for taking a look. I don't know much about the is_stmt markers, and it looks likes WithIsStmt() is mostly used in SSA and inlining, so I'm unsure what I might need to fix with the stenciling/instantiation code.

Let me know if you want to talk about this - maybe I can take over once if you figure out there is something I should be doing in the instantiation code or the code that initially creates the nodes for generic functions (but that is the same code that creates nodes for non-generic functions).

@dr2chase
Copy link
Contributor

I used go build -gcflags=-d=ssa/all/dump='G[go.shape.int_0]' aa.go to get a better description of what was going on (it is like GOSSAFUNC output, but phase-by-phase into files, with the inlining locations fully expanded so we can see what's going on). Any line marked with a "+" has IsStmt set. I don't immediately see the problem (I see an instruction with line "+10") but I could be overlooking something.

# /Users/drchase/work/tmp/b49628/aa.go:9
       	00000 (9) 	TEXT	"".G[go.shape.int_0](SB), DUPOK|ABIInternal
       	00001 (9) 	FUNCDATA	$0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
       	00002 (9) 	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
       	00003 (9) 	FUNCDATA	$5, "".G[go.shape.int_0].arginfo1(SB)
       	00004 (9) 	FUNCDATA	$6, "".G[go.shape.int_0].argliveinfo(SB)
 b4    	00005 (9) 	PCDATA	$3, $1
 v10   	00006 (+10) 	TESTB	AX, (AX)
 v15   	00007 (?) 	NOP
# /Users/drchase/work/tmp/b49628/aa.go:10
# /Users/drchase/work/tmp/b49628/aa.go:6
 v20   	00008 (+6) 	XCHGL	AX, AX
# /Users/drchase/work/tmp/b49628/aa.go:10
# /Users/drchase/work/tmp/b49628/aa.go:6
# $GOROOT/src/fmt/print.go:213
 v23   	00009 (+213) 	MOVQ	os.Stdout(SB), BX
 v33   	00010 (-213) 	LEAQ	go.itab.*os.File,io.Writer(SB), AX
 v22   	00011 (-213) 	LEAQ	go.string."called\n"(SB), CX
 v34   	00012 (-213) 	MOVL	$7, DI
 v36   	00013 (-213) 	XORL	SI, SI
 v24   	00014 (-213) 	XORL	R8, R8
 v30   	00015 (-213) 	MOVQ	R8, R9
 v25   	00016 (-213) 	PCDATA	$1, $0
 v25   	00017 (-213) 	CALL	fmt.Fprintf(SB)
 v32   	00018 (?) 	NOP
# /Users/drchase/work/tmp/b49628/aa.go:11
# $GOROOT/src/fmt/print.go:213
 v35   	00019 (-213) 	MOVQ	os.Stdout(SB), BX
 v17   	00020 (-213) 	LEAQ	go.itab.*os.File,io.Writer(SB), AX
 v16   	00021 (-213) 	LEAQ	go.string."after\n"(SB), CX
 v28   	00022 (-213) 	MOVL	$6, DI
 v41   	00023 (-213) 	XORL	SI, SI
 v19   	00024 (-213) 	XORL	R8, R8
 v18   	00025 (-213) 	MOVQ	R8, R9
 v37   	00026 (-213) 	CALL	fmt.Fprintf(SB)
# /Users/drchase/work/tmp/b49628/aa.go:11
 b4    	00027 (+11) 	RET
       	00028 (?) 	END

@danscales
Copy link
Contributor

danscales commented Nov 19, 2021

Thanks, David. Note that the output @dr2chase is showing is from the G[go.shape.int_0]_52__genssa.dump file written to the file system.

Here's same output, but for aa2.go in which we convert the generic functions to non-generic functions where T is int:

package main

import "fmt"

func F(n int) {
        fmt.Printf("called\n")
}

func G(n int) {
        F(n)
        fmt.Printf("after\n")
}

func main() {
        G(3)
}

The output is now:

# aa2.go:9
       	00000 (9) 	TEXT	"".G(SB), ABIInternal
       	00001 (9) 	FUNCDATA	$0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
       	00002 (9) 	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
       	00003 (9) 	FUNCDATA	$5, "".G.arginfo1(SB)
       	00004 (9) 	FUNCDATA	$6, "".G.argliveinfo(SB)
 b4    	00005 (9) 	PCDATA	$3, $1
 v6    	00006 (?) 	NOP
# aa2.go:10
# aa2.go:6
 v11   	00007 (+6) 	XCHGL	AX, AX
# aa2.go:10
 v14   	00008 (+10) 	MOVQ	os.Stdout(SB), BX
# aa2.go:10
# aa2.go:6
# $GOROOT/src/fmt/print.go:213
 v24   	00009 (+213) 	LEAQ	go.itab.*os.File,io.Writer(SB), AX
 v13   	00010 (-213) 	LEAQ	go.string."called\n"(SB), CX
 v25   	00011 (-213) 	MOVL	$7, DI
 v27   	00012 (-213) 	XORL	SI, SI
 v21   	00013 (-213) 	XORL	R8, R8
 v15   	00014 (-213) 	MOVQ	R8, R9
 v16   	00015 (-213) 	PCDATA	$1, $0
 v16   	00016 (-213) 	CALL	fmt.Fprintf(SB)
# aa2.go:11
 v23   	00017 (+11) 	XCHGL	AX, AX
# aa2.go:11
# $GOROOT/src/fmt/print.go:213
 v26   	00018 (-213) 	MOVQ	os.Stdout(SB), BX
 v8    	00019 (-213) 	LEAQ	go.itab.*os.File,io.Writer(SB), AX
 v7    	00020 (-213) 	LEAQ	go.string."after\n"(SB), CX
 v19   	00021 (-213) 	MOVL	$6, DI
 v32   	00022 (-213) 	XORL	SI, SI
 v10   	00023 (-213) 	XORL	R8, R8
 v9    	00024 (-213) 	MOVQ	R8, R9
 v28   	00025 (-213) 	CALL	fmt.Fprintf(SB)
# aa2.go:12
 b4    	00026 (12) 	RET
       	00027 (?) 	END

Both have the same assembly statements marked with '+' (though aa.go has an extra assembly statement 'TESTB AX, (AX)' that is marked +10). The main difference I see is that the 'MOVQ os.Stdout(SB), BX' statement is marked (+10) rather than (+213).

@aarzilli Can you point out more of what the problem is that you are seeing? Is it something related to the difference for the 'MOVQ os.Stdout(SB), BX' statement (even though in both cases it is marked as IsStmt)?

@aarzilli
Copy link
Contributor Author

Interesting. It looks like it only happens with optimizations disabled.

go build -x -gcflags='-N -l -d=ssa/all/dump=G[go.shape.int_0]' test.go
       	00000 (9) 	TEXT	"".G[go.shape.int_0](SB), DUPOK|ABIInternal
       	00001 (9) 	FUNCDATA	$0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
       	00002 (9) 	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
       	00003 (9) 	FUNCDATA	$5, "".G[go.shape.int_0].arginfo1(SB)
 v32   	00004 (+9) 	PCDATA	$0, $-2
 v32   	00005 (+9) 	MOVQ	AX, ""..dict(SP)
 v22   	00006 (-9) 	MOVQ	BX, "".n+8(SP)
 v13   	00007 (-10) 	PCDATA	$0, $-1
 v13   	00008 (-10) 	TESTB	AX, (AX)
 v17   	00009 (-10) 	MOVQ	16(AX), AX
 v21   	00010 (-10) 	MOVQ	AX, ""..autotmp_2-8(SP)
 v23   	00011 (-10) 	MOVQ	"".n+8(SP), BX
 v24   	00012 (-10) 	PCDATA	$1, $0
 v24   	00013 (-10) 	CALL	"".F[go.shape.int_0](SB)
 v19   	00014 (+11) 	LEAQ	go.string."after\n"(SB), AX
 v16   	00015 (-11) 	MOVL	$6, BX
 v5    	00016 (-11) 	XORL	CX, CX
 v4    	00017 (-11) 	XORL	DI, DI
 v27   	00018 (-11) 	MOVQ	DI, SI
 v28   	00019 (+11) 	CALL	fmt.Printf(SB)
 b1    	00020 (-11) 	RET
       	00021 (?) 	END

(the step before the last one does have an instruction marked with +)

@danscales
Copy link
Contributor

OK, so I think that you are pointing out that line 10 does not have any associated assembly instruction that is marked with "+10".

If I compare the dump output for test.go and the aa2.go about that I showed above where we substitute in 'int' for T everywhere, I see that up to the last SSA pass, both test.go and aa2.go have values that are marked with "+10". But in the test.go (generic case), the value that has the +10 is the conversion to unsafe pointer:

(+10) v11 = Convert <unsafe.Pointer> v33 v22 : AX

This is the conversion of the dictionary argument to unsafe pointer before then converting to a *[3]uintptr that we can then index into. When we convert to assembly, this unsafe pointer conversion is just dropped, and the +10 is not transferred to any other statement (it appears).

I'm not sure what the most consistent fix is. The most obvious fix is to find where the unsafe pointer conversion is essentially dropped (as a no-op), and make sure the isStmt attributed is passed to the next assembly statement.

On the other hand, not sure if we should somehow be attributing all the dictionary loading to the previous statement (line 9) so that the beginning of line 10 is more directly already focused on the call. This seems less likely to me, but the load of the other argument is already attributed to line 9 (the load of n into the BX register).

@randall77 @dr2chase

@dr2chase
Copy link
Contributor

Pretty sure there's a bad interaction with the LoweredNilCheck -- we don't like to float statement markers past those, because setting breakpoints AFTER the nil check gives bad user experience in the case that user is trying to debug a nil panic.

@dr2chase
Copy link
Contributor

I have what looks like a fix, curious what it does to other line number placement.
Ought to be strictly better.

@gopherbot
Copy link

Change https://golang.org/cl/366694 mentions this issue: cmd/compile: try to preserve IsStmt marks from OpConvert

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants