[PATCH] D145208: [COFF] Add MC support for emitting IMAGE_WEAK_EXTERN_ANTI_DEPENDENCY symbols

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:764
 static bool canExpand(const MCSymbol &Sym, bool InSet) {
+  if (Sym.isWeakExternal())
+    return false;
----------------
aeubanks wrote:
> efriedma wrote:
> > bcl5980 wrote:
> > > Do we need to limit it to AntiDep only? This code looks means any weak symbol can form a cycle.
> > isWeakExternal should be true for any symbol where we call setIsWeakExternal, which should be all weak symbols on Windows.
> > 
> > The whole recursion this is just much less likely to be relevant for non-AntiDep weak symbols; I've never seen this sort of usage of aliases elsewhere.
> in https://crbug.com/1435480 we're getting undefined symbol errors with symbols introduced via whole program devirtualization. removing this change resolves the issue. any thoughts?
> 
> the undefined symbol is a hidden alias to a weak_odr alias to a private ptr array constant
So you're talking about something like the following, I guess?

```
baz:
        .word   1
        .weak   bar
.set bar, baz
        .globl  foo
.set foo, bar
```

Without the patch, we resolve "foo" to point directly at "baz". That seems dubious; we're basically ignoring the linkage of the alias "bar".

With the patch, "foo" is undefined, which is pretty clearly wrong.  I'll need to take another look; I guess the code that handles aliases isn't expecting a non-weak alias to point at a weak alias?  You can revert in the meantime if you need to...

What I'd expect is that we actually just emit the alias into the object file.

Do you know what code is emitting this construct?  It seems like whatever the original IR was trying to do, it wasn't actually getting the effect it wanted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145208/new/

https://reviews.llvm.org/D145208