[lld] [LLD][COFF] Detect weak reference cycles. (PR #104463)

[Jacek Caban via llvm-commits]

================
@@ -126,9 +127,14 @@ DefinedImportThunk::DefinedImportThunk(COFFLinkerContext &ctx, StringRef name,
 
 Defined *Undefined::getWeakAlias() {
   // A weak alias may be a weak alias to another symbol, so check recursively.
-  for (Symbol *a = weakAlias; a; a = cast<Undefined>(a)->weakAlias)
+  SmallSet<Symbol *, 4> weakChain;
+  for (Symbol *a = weakAlias; a; a = cast<Undefined>(a)->weakAlias) {
+    if (weakChain.contains(a))
+      break; // We have a cycle.
----------------
cjacek wrote:

I tried to implement that, but there were a some problems with it. I pushed a draft here: https://github.com/cjacek/llvm-project/commit/55a76b765ff307c88a224201e088e50bb0aac89b

`getWeakAlias` callers don't expect errors from the call, so I had to use `fatal()` instead of `error()` to make it work like that. I guess it's what @dpaoliello meant by ICE, so maybe that's fine, I'm not sure.

We also don't track an input file associated with `Undefined` symbols, so we can't easily emit referencing files. We could potentially use something like `getSymbolLocations` (like we do in `reportUndefinedSymbol`), but we don't have access to the symbol table here without further refactoring. It seems to me that if we want something like that, it may be better to keep `getWeakAlias` returning null, detect in `reportUndefinedSymbol` that the reason for the symbol being undefined is a cycle and improve its reporting there. It would also avoid using `fatal()`.

FWIW, both MSVC linker and GNU ld report such symbols as undefined. link.exe does it pretty much like I did in this PR. GNU ld reports it only if the symbol is actually referenced, so I had to add something like `.rva testsym` to trigger it (that's not specific to cycles, other undefined weak externals are treated like that as well).

https://github.com/llvm/llvm-project/pull/104463