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: var _ = f prevents f from being dead-code eliminated #60464

Open
neild opened this issue May 26, 2023 · 6 comments
Open

cmd/compile: var _ = f prevents f from being dead-code eliminated #60464

neild opened this issue May 26, 2023 · 6 comments
Assignees
Labels
binary-size compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@neild
Copy link
Contributor

neild commented May 26, 2023

Context: https://go.dev/cl/498595

This statement prevents Config.EncryptTicket from being dead-code eliminated:

var _ = &Config{WrapSession: (&Config{}).EncryptTicket}

Seems like the compiler (linker?) could notice that the reference is dead and eliminate it.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 26, 2023
@dmitshur dmitshur added this to the Backlog milestone May 26, 2023
@thediveo
Copy link

thediveo commented May 26, 2023

Erm, no, no, no! This would, for instance, break the Ginkgo TDD framework, and in consequence Kubernetes. Probably lots of other projects too. Or am I misunderstanding the OP?

import . "github.com/onsi/ginkgo"

var _ = Describe("sometimes good caps simply go bad", func() {
    ...
})

@dmitshur
Copy link
Contributor

dmitshur commented May 26, 2023

@thediveo I think what you're pointing to is an example of a function call whose return value is being assigned to the blank identifier. That shouldn't and wouldn't change behavior. The original issue is about a variable (involving no function calls) being assigned to the blank identifier, where no side-effects are possible.

@prattmic
Copy link
Member

Specifically, Describe has side effects, so it can’t be eliminated. I think a function with no side effects would be safe to eliminate.

@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels May 30, 2023
@mknyszek
Copy link
Contributor

CC @golang/compiler

@mvdan
Copy link
Member

mvdan commented May 30, 2023

Does this also affect the var _ I = N{} or var _ I = (*N)(nil) pattern to ensure that the type N implements T?

@thanm
Copy link
Contributor

thanm commented May 30, 2023

This seems related to the same problems around deadcoding of unused global map variables, e.g. issues #2559 + #36021. The problem is that when you write

var <Whatever> = <expression>

at the global scope, the compiler turns this into a fragment of the form

   t := <expression>
   store t into <whatever>

within the package "init" function. Since "init" is live if anything in the package is live, the variable itself is live, as well as anything referenced in the expression being assigned.

Interestingly, if you do something like this:

package main

import "crypto/tls"

func main() {
	var _ = &tls.Config{WrapSession: (&tls.Config{}).EncryptTicket}
	println("whee!")
}

The final assembly for "main" looks like:

# /usr/local/google/home/thanm/usecrypto.go
00000 (7) TEXT main.main(SB), ABIInternal
00001 (7) FUNCDATA $0, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
00002 (7) FUNCDATA $1, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
00003 (+8) LEAQ type:crypto/tls.Config(SB), AX
00004 (8) PCDATA $1, $0
00005 (8) CALL runtime.newobject(SB)
00006 (+9) CALL runtime.printlock(SB)
00007 (9) LEAQ go:string."whee!\n"(SB), AX
00008 (9) MOVL $6, BX
00009 (9) CALL runtime.printstring(SB)
00010 (9) CALL runtime.printunlock(SB)
00011 (10) RET
00012 (?) END

e.g. the compiler has been able to optimize away the reference to Config.EncryptTicket. On the other hand if you move the "var _ = &tls.Config ..." statement up out in the global scope, the init function does indeed still have a reference to Config.EncryptTicket.

I think the difference seems to be that in the init version, the final store is done to a real static temp, whereas in the local version the assign is done to a "_" pseudo var. AST for the local-to-main version:

 AS tc(1) # usecrypto.go:6:6
. . NAME-main._ Offset:0 blank tc(1)
. . NAME-main..autotmp_1 esc(N) Class:PAUTO Offset:0 AutoTemp OnStack Used PTR-*tls.Config tc(1) # usecrypto.go:6:10
. BLOCK tc(1) # usecrypto.go:7:9

AST for the global scope version:

 AS tc(1) # usecrypto.go:5:49
. . LINKSYMOFFSET-main..stmp_0256 Offset:256 FUNC-func(tls.ConnectionState, *tls.SessionState) ([]byte, error) tc(1)
. . CONVNOP FUNC-func(tls.ConnectionState, *tls.SessionState) ([]byte, error) tc(1) # usecrypto.go:5:49
. . . NAME-main..autotmp_0 esc(N) Class:PAUTO Offset:0 AutoTemp OnStack Used PTR-*struct { F uintptr; R *tls.Config } tc(1) # usecrypto.go:5:49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binary-size compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

10 participants