[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;
----------------
efriedma wrote:
> efriedma wrote:
> > 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.
> Actually, thinking about it a bit more, I'm tempted to just declare that combining .set with .weak  is defined to do whatever horribly broken thing we do right now, and introduce new syntax to  represent the actual raw semantics of COFF weak externals.  Because I can't find any documentation or tests, and apparently whatever I do is going to break something...
For reference, the rules without this patch are roughly:

If a name A is .set to another name B, all reference to A instead refer to B.  If B is itself .set, this happens recursively.
If a name A is .set to another name B, and the name B is defined, define A to refer to the same address as B.
If a name A is .set to another name B, the name B is undefined, and A is marked .weak, emit a weak external for A referring to B.

With this patch, the rules for .weak symbols .set to another symbol change:
If a name A is .set to another name B, and A is marked .weak, do not resolve references to the symbol A; just emit a weak external for A referring to B.  If another alias refers to A, treat A as if it's undefined.

-----

Given that, the possible things we can do here:

- Revert the rules for ".weak" to the original rules; introduce a new ".weak_search_alias" or something like that to get the new semantics.
- Come up with some sort of compromise rule, so a non-weak alias referring to a weak alias produces some sort of definition.
- Declare that the current behavior is correct, and fix whole program devirtualization so it doesn't produce the construct in question.  (Which is maybe worth doing anyway, so the resulting IR doesn't depend on confusing edge cases of alias semantics.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145208