builder,interp: fix -ldflags -X not overriding variables with default values#5345
builder,interp: fix -ldflags -X not overriding variables with default values#5345abgoyal wants to merge 1 commit intotinygo-org:devfrom
Conversation
… values
Currently, -ldflags "-X pkg.Var=value" fails to override string variables
that have a default value in source code:
var Version = "dev" // -X override is ignored, stays "dev"
var Version string // -X override works correctly
In standard Go, -X works in both cases. This patch brings TinyGo's
behavior in line with Go.
The root cause is the timing of when -X values are applied relative to
the interp pass. The previous approach:
1. Erase global, make it undefined
2. Run interp (executes `Version = "dev"` from init code)
3. Link in -X value via makeGlobalsModule
By step 2, interp had already stored the default value, and the linking
step couldn't reliably override it.
This patch changes the approach:
1. Set -X value as initializer before interp runs
2. Run interp, but skip stores to -X globals
3. Global retains the -X value
This also means dependent compile-time computations work correctly:
var Version = "dev"
var VersionLen = len(Version) // now evaluates with -X value
With this fix, makeGlobalsModule becomes redundant - we no longer need
a separate module to work around type renaming issues since we set the
value directly on the global using its existing type during per-package
compilation.
I've removed makeGlobalsModule as part of this change. If there are edge
cases I haven't considered where keeping it would be safer, happy to
restore it as a fallback.
| // Erase all globals that are part of the undefinedGlobals list. | ||
| // This list comes from the -ldflags="-X pkg.foo=val" option. | ||
| // Instead of setting the value directly in the AST (which would | ||
| // mean the value, which may be a secret, is stored in the build |
There was a problem hiding this comment.
I think this is a common use case for -ldflags. It is a major implication of this change, have to think about it. I am looking to see if we can use some of this proposed change and still prevent this from being cached...
|
I had noticed the build cache comment while implementing the patch. also saw #5346 and makes sense to me as a less invasive change. I will close my PR in favor of #5346 if thats more in line with the existing code flow. I do have a question, separately: I am not sure I understand the secrets-in-build-cache issue. Won't that same secret be in the binary as well? And regular go also seems to not mind leaving them in the build cache. Could you please help me understand? |
It is a very common pattern for device provisioning to generate a unique binary per device with whatever config data embedded right into the binary. Is embedding unprotected secrets into a binary a good idea? Probably not a good idea. But perhaps even worse is having those secrets or any other provisioning data left on the machine used to perform the build, if for no other reason than it being unexpected; you would think to protect the build artifacts themselves. Scanning caches looking for secrets is possible if you have that data in the cache. Anyhow, hopefully that explains some of the current thinking. |
|
Suggestion for an alternative approach: remove the value from the AST before the SSA step runs. That way it is left unset and can be overridden. |
3 participants
Currently, -ldflags "-X pkg.Var=value" fails to override string variables that have a default value in source code:
In standard Go, -X works in both cases. This patch brings TinyGo's behavior in line with Go.
The root cause is the timing of when -X values are applied relative to the interp pass. The previous approach:
Version = "dev"from init code)By step 2, interp had already stored the default value, and the linking step couldn't reliably override it.
This patch changes the approach:
This also means dependent compile-time computations work correctly:
With this fix, makeGlobalsModule becomes redundant - we no longer need a separate module to work around type renaming issues since we set the value directly on the global using its existing type during per-package compilation.
I've removed makeGlobalsModule as part of this change. If there are edge cases I haven't considered where keeping it would be safer, happy to restore it as a fallback.