[PATCH] D90108: [MC] Error for .weak/.globl/.local which change the symbol binding

[Fangrui Song via Phabricator via llvm-commits]

MaskRay updated this revision to Diff 301425.
MaskRay edited the summary of this revision.
MaskRay added a comment.

Use a warning if the binding is changed to STB_WEAK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90108

Files:
  llvm/lib/MC/MCELFStreamer.cpp
  llvm/test/MC/ELF/symbol-binding-changed.s


Index: llvm/test/MC/ELF/symbol-binding-changed.s
===================================================================
--- /dev/null
+++ llvm/test/MC/ELF/symbol-binding-changed.s
@@ -0,0 +1,28 @@
+# RUN: not llvm-mc -filetype=obj -triple=x86_64 %s -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
+
+# CHECK: error: local changed binding to STB_GLOBAL
+local:
+.local local
+.globl local
+
+## `.globl x; .weak x` matches the GNU as behavior. We issue a warning for now.
+# CHECK: warning: global changed binding to STB_WEAK
+global:
+.global global
+.weak global
+
+# CHECK: error: weak changed binding to STB_LOCAL
+weak:
+.weak weak
+.local weak
+
+# CHECK-NOT: error:
+multi_local:
+.local multi_local
+.local multi_local
+multi_global:
+.global multi_global
+.global multi_global
+multi_weak:
+.weak multi_weak
+.weak multi_weak
Index: llvm/lib/MC/MCELFStreamer.cpp
===================================================================
--- llvm/lib/MC/MCELFStreamer.cpp
+++ llvm/lib/MC/MCELFStreamer.cpp
@@ -225,17 +225,31 @@
     break;
 
   case MCSA_Global:
+    // For `.weak x; .global x`, GNU as sets the binding to STB_WEAK while we
+    // traditionally set the binding to STB_GLOBAL. This is error-prone, so we
+    // error on such cases. Note, we also disallow changed binding from .local .
+    if (Symbol->isBindingSet() && Symbol->getBinding() != ELF::STB_GLOBAL)
+      getContext().reportError(SMLoc(), Symbol->getName() +
+                                            " changed binding to STB_GLOBAL");
     Symbol->setBinding(ELF::STB_GLOBAL);
     Symbol->setExternal(true);
     break;
 
   case MCSA_WeakReference:
   case MCSA_Weak:
+    // For `.globl x; .weak x`, both MC and GNU as set the binding to STB_WEAK.
+    // We emit a warning for now but may switch to an error in the future.
+    if (Symbol->isBindingSet() && Symbol->getBinding() != ELF::STB_WEAK)
+      getContext().reportWarning(SMLoc(), Symbol->getName() +
+                                              " changed binding to STB_WEAK");
     Symbol->setBinding(ELF::STB_WEAK);
     Symbol->setExternal(true);
     break;
 
   case MCSA_Local:
+    if (Symbol->isBindingSet() && Symbol->getBinding() != ELF::STB_LOCAL)
+      getContext().reportError(SMLoc(), Symbol->getName() +
+                                            " changed binding to STB_LOCAL");
     Symbol->setBinding(ELF::STB_LOCAL);
     Symbol->setExternal(false);
     break;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90108.301425.patch
Type: text/x-patch
Size: 2448 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201028/ddd18c10/attachment.bin>