[COFF] Aliases resolve directly to defined external targets

[epastor]

Summary

Avoid introducing unnecessary indirection for weak-external references.

We only need to introduce ".weak.<SYMBOL>.default" when referencing a
symbol that is defined, but not external.

[mstorsjo]

This does seem to produce the right results for all the cases that I can come to think of right now, so I guess it's good, but I'd like to think about it for another day or two, if that's ok.

[mstorsjo]

I tested this with an input file like this here:

        .globl uniquename
uniquename:
        ret

        .globl weakundef

        .globl func
func:
        call weakundef
        ret

        .weak aliasname
aliasname = func

        .weak secondalias
secondalias = aliasname

        .weak weakundef

        .weak weakfunc 
        .globl weakfunc
weakfunc:
        ret

        .weak undefalias
undefalias = undefsym

Maybe it'd be good to add more testcases out of these, in addition to the one added here (with verbose comments in the testcase about what's tested and what the expected outcome is), if some of the cases aren't exercised by the current tests. At least the second-level alias and alias against an undefined symbol (undefalias) are untested I think, and I don't see a case that really matches the weakfunc case either - so it'd be good to have those covered as well.

[mstorsjo]

llvm/lib/MC/WinCOFFObjectWriter.cpp

359–360

Can you come up with a scenario where you hit this else/return nullptr? Because in all of my testcases, I either hit the "return GetOr...", or return early at the first check (!Symbol.isVariable()).

I.e. it's hard to get a feel for whether this is the right condition here, as long as one could just as well remove the if statement altogether.

[epastor]

Add test demonstrating hitting the else/return case

[epastor]

Add test for a weak function definition

[epastor]

In D88305#2295996, @mstorsjo wrote:

I tested this with an input file like this here:
...

Maybe it'd be good to add more testcases out of these, in addition to the one added here (with verbose comments in the testcase about what's tested and what the expected outcome is), if some of the cases aren't exercised by the current tests. At least the second-level alias and alias against an undefined symbol (undefalias) are untested I think, and I don't see a case that really matches the weakfunc case either - so it'd be good to have those covered as well.

I've added a new test file covering (I think) all of your proposed cases except the weakfunc case. That one, I added to the weak.s test file instead.

llvm/lib/MC/WinCOFFObjectWriter.cpp

359–360

This scenario comes up when making a weak reference to a local label.

I've just added a new test file for most of the alias cases I could think of; the definition of t2 there (aliasee: proc2) triggers this scenario.

[mstorsjo]

LGTM

Awesome, thanks for finding all the relevant cases here. I've tested it with my own testsuite for GNU style weak symbols, and it seems to still be working.

llvm/test/MC/COFF/weak-alias-labels.s

88

Out of curiosity - how does the t4 case differ from t1? Only in the fact that bar is defined further ahead in the file?

[epastor]

llvm/test/MC/COFF/weak-alias-labels.s

88

That was the idea, yes - making sure that (effective) forward declarations still worked properly.

llvm/lib/MC/WinCOFFObjectWriter.cpp

if (!Symbol.isVariable())
     return nullptr;
 
   const MCSymbolRefExpr *SymRef =
       dyn_cast<MCSymbolRefExpr>(Symbol.getVariableValue());
   if (!SymRef)
     return nullptr;
 
   const MCSymbol &Aliasee = SymRef->getSymbol();
-  if (!Aliasee.isUndefined())
-    return nullptr;
+  if (Aliasee.isUndefined() || Aliasee.isExternal())
     return GetOrCreateCOFFSymbol(&Aliasee);
+  else
+    return nullptr;
 }
 
 /// This function takes a symbol data object from the assembler
 /// and creates the associated COFF symbol staging object.
 void WinCOFFObjectWriter::DefineSymbol(const MCSymbol &MCSym,
                                        MCAssembler &Assembler,
                                        const MCAsmLayout &Layout) {
   COFFSymbol *Sym = GetOrCreateCOFFSymbol(&MCSym);
   const MCSymbol *Base = Layout.getBaseSymbol(MCSym);

llvm/test/MC/COFF/weak-alias-labels.s

+// RUN: llvm-mc -filetype=obj -triple i686-pc-win32 %s | llvm-readobj --symbols - | FileCheck %s
+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-win32 %s | llvm-readobj --symbols - | FileCheck %s
+
+// When making weak references to labels/procedures, we reference them directly
+// if they have global symbols; otherwise, we need to create a global symbol for
+// the reference to resolve to.
+
+    .text
+
+    .globl proc1
+proc1:
+    ret
+
+proc2:
+    ret
+// CHECK:      Symbol {
+// CHECK:        Name: proc2
+// CHECK-NEXT:   Value: [[PROC2_VALUE:[0-9]+]]
+// CHECK-NEXT:   Section: [[PROC2_SECTION:.*]]
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: Static
+// CHECK-NEXT:   AuxSymbolCount: 0
+// CHECK-NEXT: }
+
+    .weak t1
+t1 = proc1
+
+// CHECK:      Symbol {
+// CHECK:        Name: t1
+// CHECK-NEXT:   Value: 0
+// CHECK-NEXT:   Section: IMAGE_SYM_UNDEFINED
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: WeakExternal
+// CHECK-NEXT:   AuxSymbolCount: 1
+// CHECK-NEXT:   AuxWeakExternal {
+// CHECK-NEXT:     Linked: proc1
+// CHECK-NEXT:     Search: Alias
+// CHECK-NEXT:   }
+// CHECK-NEXT: }
+
+    .weak t2
+t2 = proc2
+
+// CHECK:      Symbol {
+// CHECK:        Name: t2
+// CHECK-NEXT:   Value: 0
+// CHECK-NEXT:   Section: IMAGE_SYM_UNDEFINED
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: WeakExternal
+// CHECK-NEXT:   AuxSymbolCount: 1
+// CHECK-NEXT:   AuxWeakExternal {
+// CHECK-NEXT:     Linked: .weak.t2.default
+// CHECK-NEXT:     Search: Alias
+// CHECK-NEXT:   }
+// CHECK-NEXT: }
+// CHECK:      Symbol {
+// CHECK:        Name: .weak.t2.default
+// CHECK-NEXT:   Value: [[PROC2_VALUE]]
+// CHECK-NEXT:   Section: [[PROC2_SECTION]]
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: External
+// CHECK-NEXT:   AuxSymbolCount: 0
+// CHECK-NEXT: }
+
+    .weak t3
+t3 = foo
+
+// CHECK:      Symbol {
+// CHECK:        Name: t3
+// CHECK-NEXT:   Value: 0
+// CHECK-NEXT:   Section: IMAGE_SYM_UNDEFINED
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: WeakExternal
+// CHECK-NEXT:   AuxSymbolCount: 1
+// CHECK-NEXT:   AuxWeakExternal {
+// CHECK-NEXT:     Linked: foo
+// CHECK-NEXT:     Search: Alias
+// CHECK-NEXT:   }
+// CHECK-NEXT: }
+
+    .weak t4
+t4 = bar
+
+    .globl bar
+bar:
+    ret
+
+// CHECK:      Symbol {
+// CHECK:        Name: t4
+// CHECK-NEXT:   Value: 0
+// CHECK-NEXT:   Section: IMAGE_SYM_UNDEFINED
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: WeakExternal
+// CHECK-NEXT:   AuxSymbolCount: 1
+// CHECK-NEXT:   AuxWeakExternal {
+// CHECK-NEXT:     Linked: bar
+// CHECK-NEXT:     Search: Alias
+// CHECK-NEXT:   }
+// CHECK-NEXT: }
+
+    .weak t5
+t5 = t2
+
+// CHECK:      Symbol {
+// CHECK:        Name: t5
+// CHECK-NEXT:   Value: 0
+// CHECK-NEXT:   Section: IMAGE_SYM_UNDEFINED
+// CHECK-NEXT:   BaseType: Null
+// CHECK-NEXT:   ComplexType: Null
+// CHECK-NEXT:   StorageClass: WeakExternal
+// CHECK-NEXT:   AuxSymbolCount: 1
+// CHECK-NEXT:   AuxWeakExternal {
+// CHECK-NEXT:     Linked: t2
+// CHECK-NEXT:     Search: Alias
+// CHECK-NEXT:   }
+// CHECK-NEXT: }

llvm/test/MC/COFF/weak.s

 // CHECK:        Name:           _test_weak_alias
 // CHECK-NEXT:   Value:          0
 // CHECK-NEXT:   Section:        IMAGE_SYM_UNDEFINED (0)
 // CHECK-NEXT:   BaseType:       Null
 // CHECK-NEXT:   ComplexType:    Null
 // CHECK-NEXT:   StorageClass:   WeakExternal
 // CHECK-NEXT:   AuxSymbolCount: 1
 // CHECK-NEXT:   AuxWeakExternal {
-// CHECK-NEXT:     Linked: .weak._test_weak_alias.default
+// CHECK-NEXT:     Linked: _main
 // CHECK-NEXT:      Search: Alias
 // CHECK-NEXT:   }
 // CHECK-NEXT: }
-
-// CHECK:      Symbol {
-// CHECK:        Name: .weak._test_weak_alias.default._main
-// CHECK-NEXT:   Value: 0
-// CHECK-NEXT:   Section: .text
-// CHECK-NEXT:   BaseType: Null
-// CHECK-NEXT:   ComplexType: Null
-// CHECK-NEXT:   StorageClass: External
-// CHECK-NEXT:   AuxSymbolCount: 0
-// CHECK-NEXT: }