[ELF] Update st_size when merging a common symbol with a shared symbol

[MaskRay]

Summary

When a common symbol is merged with a shared symbol, increase st_size if
the shared symbol has a larger st_size. At runtime, the executable's
symbol overrides the shared symbol. The shared symbol may be created
from common symbols in a previous link. This rule makes sure we pick
the largest size among all common symbols.

This behavior matches GNU ld. See
https://sourceware.org/bugzilla/show_bug.cgi?id=25236 for discussions.

A shared symbol does not hold alignment constraints. Ignore the
alignment update.

[peter.smith]

Reading through the PR I think that this is the right thing to. I think that there is a possibility of narrowing down the assumption that a Shared definition is common, by checking its address against the loadable segment containing BSS. I don't think it is worth doing that.

[MaskRay]

🤡

[peter.smith]

Looks good to me. Will be worth seeing if there are any comments in the US time zone.

[ruiu]

This behavior seems to make sense but is very subtle. Can you add a comment to explain what this is doing?

[MaskRay]

Add comments

lld/ELF/Symbols.cpp

 }
 
 void Symbol::resolveCommon(const CommonSymbol &other) {
   int cmp = compare(&other);
   if (cmp < 0)
     return;
 
   if (cmp > 0) {
+    if (auto *s = dyn_cast<SharedSymbol>(this)) {
+      // Increase st_size if the shared symbol has a larger st_size. The shared
+      // symbol may be created from common symbols. The fact that some object
+      // files were linked into a shared object first should not change the
+      // regular rule that picks the largest st_size.
+      uint64_t size = s->size;
       replace(other);
+      if (size > cast<CommonSymbol>(this)->size)
+        cast<CommonSymbol>(this)->size = size;
+    } else {
+      replace(other);
+    }
     return;
   }
 
   CommonSymbol *oldSym = cast<CommonSymbol>(this);
 
   oldSym->alignment = std::max(oldSym->alignment, other.alignment);
   if (oldSym->size < other.size) {
     oldSym->file = other.file;
if (isWeak()) {
     binding = STB_WEAK;
     return;
   }
 
   other.fetch();
 }
 
 void Symbol::resolveShared(const SharedSymbol &other) {
+  if (isCommon()) {
+    // See the comment in resolveCommon() above.
+    if (other.size > cast<CommonSymbol>(this)->size)
+      cast<CommonSymbol>(this)->size = other.size;
+    return;
+  }
   if (visibility == STV_DEFAULT && (isUndefined() || isLazy())) {
     // An undefined symbol with non default visibility must be satisfied
     // in the same DSO.
     uint8_t bind = binding;
     replace(other);
     binding = bind;
     referenced = true;
   }
 }
 
 } // namespace elf
 } // namespace lld

lld/test/ELF/common-shared.s

+# REQUIRES: x86
+## When a common symbol is merged with a shared symbol, pick the larger st_size.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo '.globl com; .comm com, 16' | llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
+# RUN: ld.lld -shared %t1.o -o %t1.so
+
+# RUN: ld.lld %t.o %t1.so -o %t
+# RUN: llvm-readelf -s %t | FileCheck %s
+# RUN: ld.lld %t1.so %t.o -o %t
+# RUN: llvm-readelf -s %t | FileCheck %s
+
+# CHECK: 16 OBJECT GLOBAL DEFAULT 7 com
+
+.globl com
+.comm com,1