rules_go
rules_go copied to clipboard
bazelbuild
fix: properly expand absolute path placeholder for link calls
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
PR #4009 added a mechanism that uses a sentinel value as a placeholder for paths which need to be expanded to absolutes for compile operations. These paths end up embedded in .a files and come back to the linker during link operations. The previous PR replaces the CC tool with a pointer to builder cc which in turn substitutes the placeholder value for actual paths. Unfortunately this wasn't done for the linker, so the linker is receiving parameters like --sysroot=__GO_BAZEL_CC_PLACEHOLDER__external/_main~_repo_rules~ubuntu_20_04_sysroot/ which is not valid. When a sysroot is configured this means that internally the linker won't be able to find the sysroot in some cases.
In almost comically bad luck, this only affects the go tool link calls to linkerFlagSupported because for some reason linkerFlagSupported puts the -extldflags at the beginning of the linker command, while the main host link command has them at the end. So this means the linker detects all flags as unsupported but then proceeds to run the linker "correctly" which results in really confusing linker errors. In my case this resulted in the linker trying to run with -rdynamic incorrectly.
Which issues(s) does this PR fix?
Fixes #3886
It could be reasonable to open a bug against go tool link describing the difference between the host link and the flag checks. I suspect it's not really intended behavior and was just sort of happenstance that things ended up that way.
I also really have no idea how to write a test for this. I'm willing to give it a shot but would really appreciate a suggestion as to where/how.
Clearly this naive approach won't work on windows, I guess we could detect the OS write an OS specific script? This all feels pretty messy so i'm hoping someone out there has ideas for a better solution.
One idea for a different approach would be to use a symlink named something like builder-cc and have the builder inspect os.Argv[0] to set the command.
I've changed this to use a symlink approach instead of a script which seems to be more portable.
Could you add a test for this under tests/core/cgo? It doesn't have to be a full end-to-end test, but perhaps you could set some --copts that are turned into absolute paths and assert on the build output with -s? That way you don't have to set up an actual sysroot in the test.
Sure, I will do both.
I honestly think even if an upstream issue is fixed there is still technically a bug here, we just won't hit the bug for sysroots because the last one will take precedence but it is conceivable that other parameters with paths wouldn't work the same way and we would end up with some new subtle bug.
It looks like someone else already filed an issue relating to this:
https://github.com/golang/go/issues/69350
@fmeum I've added a test. I think the test most likely only works on platforms with gcc-ish compilers (I have no idea if this bug really even exists on windows and have no way to test unfortunately) so I added a build constraint for !windows.
actually it seems the test passes without the build constraint and fails with it so I'll leave it alone for now. I'm assuming the windows test must still use gcc
I think the build failure is in an unrelated test, maybe a fluke? Either way I'm unable to trigger a retry without pushing an update.
@fmeum I believe this may be ready. I addressed one comment slightly differently than you asked so I let it unresolved but I think the issue is fixed. Let me know if you need me to do anything else on this one.