fix deadcode disabling in Go #30

tonistiigi · 2024-07-09T01:16:03Z

Avoid embedding reflect.Type and its Method() method because exposing it disables deadcode removal in linker.

https://tip.golang.org/src/cmd/link/internal/ld/deadcode.go#L395

Avoid embedding reflect.Type and its Method() method because exposing it disables deadcode removal in linker. Signed-off-by: Tonis Tiigi <[email protected]>

thaJeztah · 2025-03-21T19:06:57Z

@taowen PTAL 🙏

ikonst · 2025-03-22T23:51:15Z

@tonistiigi Are you sure it's meaningful? Can you suggest a simple sample main.go importing this package such that go build -ldflags=-dumpdep main.go |& whydeadcode produces output?

tonistiigi · 2025-03-23T02:20:36Z

@ikonst

» cat go.mod| grep reflect2
	github.com/modern-go/reflect2 v1.0.2 // indirect
» go build -ldflags=-dumpdep ./cmd/buildx 2>&1 | ~/go/bin/whydeadcode                                                                                                                                
github.com/modern-go/reflect2.UnsafeSliceType.Method reachable from:
	 type:github.com/modern-go/reflect2.UnsafeSliceType
	 github.com/modern-go/reflect2.newUnsafeSliceType
	 github.com/modern-go/reflect2.(*frozenConfig).wrapType
	 github.com/modern-go/reflect2.(*frozenConfig).Type2
	 github.com/modern-go/reflect2.(*frozenConfig).TypeOf
	 github.com/json-iterator/go.init
	 github.com/json-iterator/go..inittask
	 go:main.inittasks
	 _

alternatively sometimes

github.com/modern-go/reflect2.(*UnsafeSliceType).MethodByName reachable from:
	 type:*github.com/modern-go/reflect2.UnsafeSliceType
	 type:github.com/json-iterator/go.sliceEncoder
	 github.com/json-iterator/go.encoderOfSlice
	 github.com/json-iterator/go._createEncoderOfType
	 github.com/json-iterator/go.createEncoderOfType
	 github.com/json-iterator/go.encoderOfType
	 github.com/json-iterator/go.describeStruct
	 github.com/json-iterator/go.decoderOfStruct
	 github.com/json-iterator/go._createDecoderOfType
	 github.com/json-iterator/go.createDecoderOfType
	 github.com/json-iterator/go.decoderOfType
	 github.com/json-iterator/go.(*frozenConfig).DecoderOf
	 github.com/json-iterator/go.(*Iterator).ReadVal
	 github.com/json-iterator/go.(*Iterator).Read.func2
	 github.com/json-iterator/go.(*Iterator).Read
	 github.com/json-iterator/go.(*frozenConfig).validateJsonRawMessage.func1
	 github.com/json-iterator/go.(*frozenConfig).validateJsonRawMessage
	 github.com/json-iterator/go.Config.Froze
	 github.com/json-iterator/go.init
	 github.com/json-iterator/go..inittask
	 go:main.inittasks
	 _

» cat go.mod| grep reflect2
require github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect

» go build -ldflags=-dumpdep ./cmd/buildx 2>&1 | ~/go/bin/whydeadcode
(no traces via reflect2)

ikonst · 2025-03-23T07:34:34Z

@tonistiigi yes, but can you provide a minimal main.go?

From whydeadcode README:

Because of how -dumpdep works only the first result output by whydeadcode is real. Because of how -dumpdep works anything beyond the first result can be a false positive (i.e. things that look like they will affect deadcode elimination but won't if the first result is taken care of) and it can also have false negatives (i.e. things that will continue to keep deadcode elimination disabled if the first result is taken care of).

I saw a similar "problem" in another package but when I tried to reproduce it in isolation, it wasn't a problem anymore.

tonistiigi · 2025-03-24T01:01:12Z

yes, but can you provide a minimal main.go?

I don't have time to work on this. What I can show you is that this does have a significant effect on the build above.

» ls -l buildx                                                                                                                                                                                       
-rwxr-xr-x  1 tonistiigi  wheel  95985362 Mar 23 00:56 buildx
» ls -l buildx                                                                                                                                                                                      
-rwxr-xr-x  1 tonistiigi  wheel  65406946 Mar 23 00:55 buildx

The difference of these builds is only the patch in this PR (you can't repro this as more fixes are needed, most notably awful text/template but other patches were already part of the first build).

Because of how -dumpdep works anything beyond the first result can be a false positive (i.e. things that look like they will affect deadcode elimination but won't if the first result is taken care of) and it can also have false negatives (i.e. things that will continue to keep deadcode elimination disabled if the first result is taken care of).

This is true, but the current case is not a false positive. It is entirely possible that there are other cases that mess up deadcode in this package that this PR does not fix. I only followed the call path in buildx. The output of whydeadcode is also more complex than defined here. It can show call paths where MethodByName() is called with the constant argument as the first item but these are really not that problematic. In debugging you would need to look at the first codepath that doesn't use a constant argument.

pgimalac · 2025-03-24T09:41:42Z

Definitely not a minimal example, but I can confirm updating to the merged commit fixes it for the https://github.com/DataDog/datadog-agent binaries, see DataDog/datadog-agent#32527 (comment) for the size diff (note that the PR is a hacky one where I just comment out the uses of template 😄)

Without the update, the output of whydeadcode is

Details

github.com/modern-go/reflect2.(*safeType).MethodByName reachable from:
	 type:*github.com/modern-go/reflect2.safeType
	 type:github.com/modern-go/reflect2.safeType
	 github.com/modern-go/reflect2.(*frozenConfig).wrapType
	 github.com/modern-go/reflect2.(*frozenConfig).Type2
	 github.com/modern-go/reflect2.(*frozenConfig).TypeOf
	 github.com/json-iterator/go.(*frozenConfig).validateJsonRawMessage
	 github.com/json-iterator/go.Config.Froze
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.newPodUnmarshaller
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.NewKubeUtil
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.GetKubeUtilWithRetrier
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/kubelet.GetKubeUtil
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/hostinfo.NewNodeInfo
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/clustername.getClusterName
	 github.com/DataDog/datadog-agent/pkg/util/kubernetes/clustername.GetClusterNameTagValue
	 github.com/DataDog/datadog-agent/comp/metadata/host/hostimpl/hosttags.Get
	 github.com/DataDog/datadog-agent/pkg/aggregator.newHostTagProviderWithClock
	 github.com/DataDog/datadog-agent/pkg/aggregator.initAgentDemultiplexer
	 github.com/DataDog/datadog-agent/pkg/aggregator.InitAndStartAgentDemultiplexer
	 github.com/DataDog/datadog-agent/comp/aggregator/demultiplexer/demultiplexerimpl.newDemultiplexer
	 github.com/DataDog/datadog-agent/comp/aggregator/demultiplexer/demultiplexerimpl.newDemultiplexer·f
	 github.com/DataDog/datadog-agent/comp/aggregator/demultiplexer/demultiplexerimpl.Module
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/subcommands/start.RunDogstatsdFct
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/subcommands/start.MakeCommand.func1
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/subcommands/start.MakeCommand
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/command.makeCommands
	 github.com/DataDog/datadog-agent/cmd/dogstatsd/command.MakeRootCommand
	 main.main
	 runtime.main_main·f
	 runtime.main
	 runtime.mainPC
	 runtime.rt0_go
	 main
	 _

The output of whydeadcode is also more complex than defined here. It can show call paths where MethodByName() is called with the constant argument as the first item but these are really not that problematic.

Actually this is something I fixed in aarzilli/whydeadcode#3, if you use the latest it should be correct.

ikonst · 2025-03-24T10:09:35Z

@pgimalac To complement that, can you comment out the use of text/template but undo this change and see if the binary size remains large? I want to make sure this has an actual effect.

I've made a similar PR in jszwec/csvutil#73 but then I couldn't reproduce the effect.

ikonst · 2025-03-24T10:25:27Z

For example, given this program:

package main

import (
	"fmt"
	"reflect"
)

type embedding struct {
	reflect.Type
	foo string
}

type s struct {
}

func (s) Foo() {}

func main() {
	t := reflect.TypeOf(s{})
	a := embedding{Type: t, foo: "bar"}
	fmt.Println(a)
	//m := a.Method(0)
	//fmt.Println(m)
}

I'm only whydeadcode output if I uncomment the last 2 lines.

Am I missing something?

pgimalac · 2025-03-24T11:06:51Z

That's actually what I did above, if I just comment out the update of reflect2 then whydeadcode gives the output from my previous comment

I tried to make a minimal example a couple months ago but wasn't able to, I think it has to be complex enough (multi module at least) so that the linker can't figure out which methods are used...

fix deadcode disabling in Go

4a6a3c9

Avoid embedding reflect.Type and its Method() method because exposing it disables deadcode removal in linker. Signed-off-by: Tonis Tiigi <[email protected]>

ikonst approved these changes Mar 15, 2025

View reviewed changes

taowen merged commit 35a7c28 into modern-go:master Mar 22, 2025

pgimalac mentioned this pull request Apr 14, 2025

[AGENTRUN-217] Update reflect2 to avoid disabling dead code elimination DataDog/datadog-agent#36092

Merged

pgimalac mentioned this pull request Jun 8, 2025

Remove use of html/template in apiserver to avoid disabling dead code elimination kubernetes/kubernetes#132177

Merged

liggitt mentioned this pull request Jun 10, 2025

WIP: fork jsoniter kubernetes-sigs/structured-merge-diff#289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix deadcode disabling in Go #30

fix deadcode disabling in Go #30

Uh oh!

tonistiigi commented Jul 9, 2024

Uh oh!

thaJeztah commented Mar 21, 2025

Uh oh!

ikonst commented Mar 22, 2025

Uh oh!

tonistiigi commented Mar 23, 2025

Uh oh!

ikonst commented Mar 23, 2025 •

edited

Loading

Uh oh!

tonistiigi commented Mar 24, 2025 •

edited

Loading

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

ikonst commented Mar 24, 2025 •

edited

Loading

Uh oh!

ikonst commented Mar 24, 2025

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fix deadcode disabling in Go #30

fix deadcode disabling in Go #30

Uh oh!

Conversation

tonistiigi commented Jul 9, 2024

Uh oh!

thaJeztah commented Mar 21, 2025

Uh oh!

ikonst commented Mar 22, 2025

Uh oh!

tonistiigi commented Mar 23, 2025

Uh oh!

ikonst commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

ikonst commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikonst commented Mar 24, 2025

Uh oh!

pgimalac commented Mar 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ikonst commented Mar 23, 2025 •

edited

Loading

tonistiigi commented Mar 24, 2025 •

edited

Loading

ikonst commented Mar 24, 2025 •

edited

Loading