[kolyshkin]

Go linker dead code elimination (DCE) of exported methods is disabled for the whole program if reflect.Value.Method[ByName] with a non-constant argument is used anywhere in the binary. One such use is in text/template (see golang/go#72895).

This PR (together with a few prior ones) eliminates all such uses:

1. build(deps): bump github.com/urfave/cli/v3 from 3.13.0 to 3.14.0 #5508 (to include Add urfave_cli_no_template build tag urfave/cli#2441)
2. libct: don't use reflect to log CRIU options #5486
3. Makefile: always use urfave_cli_no_template build tag (runc does not use custom help templates).
4. Add make verify-dce (checks there are no <ReflectMethod> symbols in -ldflags=-dumpdep output), and run it in CI, so this does not regress.

As a result, runc binary is about 15% smaller on all architectures.

NOTE runc binary size is important because when running as rootless, runc has to copy itself.

Supersedes: #5486.

[kolyshkin]

Here are the sizes of released binaries (for amd64, static/stripped) over time:

This PR is in the lower right corner. We are so back to 2022!

[AkihiroSuda]

Should be a separate PR

[kolyshkin]

We can wait a week for dependabot to create a PR to update urfave/cli (because of cooldown setting), and then I will rebase this PR.

[kolyshkin]

Marked as draft; opened a bump PR: #5508

[kolyshkin]

@AkihiroSuda @thaJeztah @rata this one is no longer a draft. We can merge #5486 first and I'll do another rebase, or just merge this one (and close #5486) -- I'm fine either way.

[kolyshkin]

We can also add a validate job that checks the binary does not grow too big (podman has something like this I believe), but this can be done as a followup.

[thaJeztah]

This feels a bit brittle; we'd almost need to have a reverse check to verify the check itself (e.g. if the template build-arg is removed)

[kolyshkin]

Well, we can try building runc without the urfave_cli_no_template tag and check that <ReflectMethod> is indeed there:

.PHONY: verify-dce
verify-dce:
	# Sanity check: without urfave_cli_no_template, DCE must be disabled,
	# otherwise the check below is not working.
	$(GO) build -tags "$(BUILDTAGS)" -o /dev/null -ldflags=-dumpdep . 2>&1 \
		| grep -q '<ReflectMethod>'
	set -o pipefail; $(GO_BUILD) -o /dev/null -ldflags=-dumpdep . 2>&1 \
		| { ! grep '<ReflectMethod>'; }

but if we ever switch to urfave/cli/v4 and they will stop using text/template (they plan to), or if text/template itself is somehow fixed in a future Go version, the check will fail again.

Or, we can quickly build a binary which calls reflect.Value.MethodByName with a non-constant string and check that <ReflectMethod> is there:

1. tests/dce-canary/main.go:

// dce-canary is a positive control for "make verify-dce". It calls
// reflect.Value.MethodByName with a non-constant argument, which makes
// the linker disable dead code elimination of exported methods, so
// "verify-dce" must detect it. If it does not, the check itself is broken.
package main

import (
      "os"
      "reflect"
)

type t struct{}

func (t) Foo() {}

func main() {
      reflect.ValueOf(t{}).MethodByName(os.Args[0])
}

2. Makefile:

verify-dce:
+	# Make sure the check works, using a program which disables DCE.
+	$(GO) build -o /dev/null -ldflags=-dumpdep ./tests/dce-canary 2>&1 \
+		| grep -q '<ReflectMethod>'
	set -o pipefail; $(GO_BUILD) -o /dev/null -ldflags=-dumpdep . 2>&1 \
		| { ! grep '<ReflectMethod>'; }

@thaJeztah do you want something like this, or perhaps we can just drop this entirely and add a binary size diff check?

[kolyshkin]

We could also add a forbidigo linter rule (to not use text/template or call reflect.Value.Method, but

• this check is only for runc itself, i.e. packages we use are not checked (but it would find something like what was fixed in libct: don't use reflect to log CRIU options #5486);
• reflect.Value.Method[ByName] is not breaking DCE but forbidigo can't distinguish const vs non-const.

[kolyshkin]

So it's not a linter job at all...

Maybe use whydeadcode? Nah, it doesn't have exit code, it's just a tool to check why DCE is disabled.

[kolyshkin]

OK I think the best way is to use a canary to check the check (and a hint to use whydeadcode). Last commit amended; WDYT @thaJeztah ?

[thaJeztah]

It probably shouldn't be a blocker for this PR (which is great to have), but at least we should give it some thought, because we don't want to silently regress.

What option do you prefer?

[thaJeztah]

Oh! You updated; let me check the latest version (just stepped away from my keyboard 😅)

[kolyshkin]

we could also add a positive check by doing something like ! make RUNC_BUILDTAGS=-urfave_cli_no_template verify-dce here but again this will break once we switch to urfave/cli/v4

[thaJeztah]

LGTM

left one suggestion / comment

[thaJeztah]

I was slightly wondering if we needed the make var at all, as there's no reason to override it, but making it a var somewhat implies it can be, and it currently doesn't make things less repetitive (we still need to include $(BUILDTAGS_ALWAYS) in both places.

If we do keep the var, we could consider naming it BUILDTAGS_COMMON, aligning with LDFLAGS_COMMON (shared between static and non-static)

[kolyshkin]

Makes sense; build tag inlined (hope we won't have any more places 😓 to add it).

[thaJeztah]

still LGTM