Some compatibility improvements for linker option processing #20384
Conversation
src/main.zig
Outdated
| // | ||
| // Note that this ambiguity only exists for the linker option; the Clang option is explicitly | ||
| // macOS-specific. | ||
| try force_undefined_symbols.put(arena, lookup_type, {}); |
There was a problem hiding this comment.
The only other option I can think of here is to make the behavior dependent on -target. But that only works if -target comes first. I think it's also unprecedented for the option processing to be conditioned on target knowledge.
There was a problem hiding this comment.
In the face of ambiguity, refuse to proceed.
There was a problem hiding this comment.
What does that mean concretely here? If I follow that to the letter, I would have to rip out the dynamic_lookup/error handling that was already here (which I'm sure was added because someone needed it).
There was a problem hiding this comment.
@andrewrk ping just in case you missed the question above.
There was a problem hiding this comment.
I do think it should be followed to the letter, including ripping out the existing handling. However, I think it's possible to support unambiguously by lowering it as a placeholder that is resolved later when the target is known. Note this is for zig cc compatibility only; the regular zig CLI doesn't allow setting linker flags directly.
There was a problem hiding this comment.
Ok, I'll see if I can make that strategy work first, and remove it otherwise.
alexrp
force-pushed
the
master
branch
3 times, most recently
from
2b39651 to
5faf556
Compare
There was a problem hiding this comment.
Thanks for working on this. I think we can merge your manual work for now, however, wouldn't it be nice to parse those .td files that you linked instead of maintaining this by hand? This is already implemented for compiler options with tools/update_clang_options.zig.
src/main.zig
Outdated
| // | ||
| // Note that this ambiguity only exists for the linker option; the Clang option is explicitly | ||
| // macOS-specific. | ||
| try force_undefined_symbols.put(arena, lookup_type, {}); |
There was a problem hiding this comment.
In the face of ambiguity, refuse to proceed.
As I got about 25% through this, that was my thought as well. 🙂 But my plate is full enough as it is, so I can't commit to that right now. Figured I may as well finish what I started, and then hopefully we can transition to something better in the future. |
|
@andrewrk I've rebased this without the commit pertaining to the |
alexrp
force-pushed
the
master
branch
2 times, most recently
from
acf8268 to
9cd1e64
Compare
Per @alexrp's suggestion ziglang/zig#20384 (comment). I've tested it with bfd and lld. It should work with existing zld as well (which is currently handling `-Wl,-e,0x0`). Fixes #106029
|
@andrewrk ping in case you didn't see it: #20384 (comment) |
These options are used by Libtool when targeting MinGW triples. They are ignored by LLD; probably safe for us to do the same. Closes ziglang#19613.
This is ignored by LLD; probably safe for us to do the same.
See ziglang#15438 for a case where it appears.
…s for consistency.
…er))` linker options.
… for consistency.
Unblocks scenarios like the one in ziglang#18750, but does not actually fix it.
There was a problem hiding this comment.
I really don't like accumulating all these different ways of specifying the same arguments. It would be better if people standardized on one option name for everything.
So, I think this change should focus on solving the actual bug (#19613), which it doesn't seem like it tackles at all since it just eats the arg and does nothing. Neither the PR description nor the code comments include a justification for this behavior.
If you want to make it handle all of LLD's variants without a specific motivating bug report, then this code needs to use the options td files like is already done for compiler flags.
| } else if (mem.eql(u8, arg, "--enable-auto-image-base") or | ||
| mem.eql(u8, arg, "-enable-auto-image-base") or | ||
| mem.eql(u8, arg, "--disable-auto-image-base") or | ||
| mem.eql(u8, arg, "-disable-auto-image-base")) | ||
| { | ||
| // These are ignored by LLD, but are used by Libtool for MinGW triples. |
There was a problem hiding this comment.
I don't think zig should recognize options and then do nothing.
What LLD does is not really relevant here. The relevant question is, does the zig project support this functionality or not?
If the user's request cannot be satisfied, it must be an error. If the user's request is satisfied regardless of this option, it should be a warning that the option is superfluous. If the user's request is ambiguous, the flag should not be supported.
There was a problem hiding this comment.
--enable-auto-image-base is a flag that binutils added 24 years ago for MinGW. It does a hash of the file and uses that as part of the image base value. I assume the idea was to avoid DLLs needing to be relocated when loaded. This is practically irrelevant today as all PEs produced since Windows Vista have ASLR enabled by default anyway, and Windows 10+ has Mandatory ASLR on by default, so it doesn't even care what the PE file wants.
We could implement the actual functionality behind the flag, but I'm fairly certain nobody actually cares about that anymore. The only reason the flag is remotely relevant is because it's hardcoded in Libtool, so if we don't at least accept and ignore it, it's not possible to cross-compile an Autotools project for MinGW with Zig.
There was a problem hiding this comment.
@andrewrk given this explanation, would you consider it acceptable to recognize and ignore just --(enable,disable)-auto-image-base as a special case?
I don't really care as much about the rest of the stuff I added in this PR, and agree that it would be nice to use LLD's TableGen files like we do for Clang. But usage of --enable-auto-image-base in particular is hardcoded in Libtool when targeting MinGW, and that makes zig cc unusable for that use case. Were it any other project, I would just ask them to stop using that flag (since it's pointless linker churn nowadays due to ASLR), but given the kinds of archaic systems they insist on maintaining support for in Autotools, I don't think there's any chance whatsoever of that happening. Just ignoring the flag unblocks a real use case, and LLD having ignored it for ~7 years would indicate that no one building modern software actually cares about the effect it has in GNU ld.
|
Closing in favor of #21603. |
Went through and made sure that most options have all their variants handled, and added handling for a few options that were missing.
This ended up being more commits than I initially expected. I can squash them if desired.
Closes #19613.
Alleviates #18750.
References: