[CodeGen] Fix wrapping personality symbol on ARM

[smeenai]

Summary

The ARM backend was explicitly setting global binding on the personality
symbol. This was added without any comment in a7ec2dcefd954, which
introduced EHABI support (back in 2011). None of the other backends do
anything equivalent, as far as I can tell.

This causes problems when attempting to wrap the personality symbol.
Wrapped symbols are marked as weak inside LTO to inhibit IPO (see
https://reviews.llvm.org/D33621). When we wrap the personality symbol,
it initially gets weak binding, and then the ARM backend attempts to
change the binding to global, which causes an error in MC because of
attempting to change the binding of a symbol from non-global to global
(the error was added in https://reviews.llvm.org/D90108).

Simply drop the ARM backend's explicit global binding setting to fix
this. This matches all the other backends, and a large internal
application successfully linked and ran with this change, so it
shouldn't cause any problems. Test via LLD, since wrapping is required
to exhibit the issue.

[MaskRay]

lld/test/ELF/arm-lto-wrap-personality.ll

1 ↗

(On Diff #375463)

Move the test to ELF/lto/ and remove lto- from the filename

10 ↗

(On Diff #375463)

%t.bc

11 ↗

(On Diff #375463)

%t.bc -o %t.so

Then add llvm-readelf -s %t.so | FileCheck %s and check the binding of ther personality symbol.

We should make better use of a test, not just testing that something doesn't crash.

[smeenai]

Address comments

[smeenai]

Thanks for the review.

In following your suggestion to check the binding, I noticed that the original symbol (the target of the wrapping) is retaining its weak binding, which seems counter to the intent of D33621. That isn't related to this patch (all wrapped symbols will exhibit this behavior, as will the personality symbol on all other backends), and I imagine it won't affect most uses of --wrap because everyone will reference the wrapped symbol and not the original.

[smeenai]

In D110609#3027778, @smeenai wrote:

Thanks for the review.

In following your suggestion to check the binding, I noticed that the original symbol (the target of the wrapping) is retaining its weak binding, which seems counter to the intent of D33621. That isn't related to this patch (all wrapped symbols will exhibit this behavior, as will the personality symbol on all other backends), and I imagine it won't affect most uses of --wrap because everyone will reference the wrapped symbol and not the original.

I dug into this more, and it dates all the way back to rGb45c164fc25200afcf48fd5df8912a52319be3e5. Filed https://bugs.llvm.org/show_bug.cgi?id=52004 for this.

[smeenai]

Add comment to test about unrelated wrapping LTO bug

[MaskRay]

and then the ARM backend attempts to change the binding to global, which causes an error.

Elaborate the error.

lld/test/ELF/lto/arm-wrap-personality.ll

2

We repeat the comment marker (e.g. ;; ) for non-RUN non-CHECK lines in lld/test/ELF and some binary utilities

16

;;

[smeenai]

Change to ;;

[smeenai]

In D110609#3028479, @MaskRay wrote:

and then the ARM backend attempts to change the binding to global, which causes an error.

Elaborate the error.

Thanks for the review! Does the updated summary look sufficient or would you like more details added?

[MaskRay]

LGTM.

lld/test/ELF/lto/arm-wrap-personality.ll

16

[smeenai]

Capitalize

lld/test/ELF/lto/arm-wrap-personality.ll

+;; Wrapped symbols are marked as weak for LTO to inhibit IPO. The ARM backend
+;; was previously explicitly marking the personality function as global, so if
+;; we attempted to wrap the personality function, its initial binding would be
+;; weak and then the ARM backend would attempt to change it to global, causing
+;; an error. Verify that the ARM backend no longer does this and we can
+;; successfully wrap the personality symbol.
+
+; REQUIRES: arm
+
+; RUN: llvm-as -o %t.bc %s
+; RUN: ld.lld -shared --wrap __gxx_personality_v0 %t.bc -o %t.so
+; RUN: llvm-readelf --dyn-syms %t.so | FileCheck %s
+
+; CHECK: GLOBAL {{.*}} __wrap___gxx_personality_v0
+;; This should be GLOBAL when PR52004 is fixed.
+; CHECK: WEAK {{.*}} __gxx_personality_v0
+
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv7-none-linux-android"
+
+define i32 @__gxx_personality_v0(...) {
+  ret i32 0
+}
+
+define void @dummy() optnone noinline personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+  invoke void @dummy() to label %cont unwind label %lpad
+
+cont:
+  ret void
+
+lpad:
+  %lp = landingpad { i8*, i32 } cleanup
+  resume { i8*, i32 } %lp
+}

llvm/lib/CodeGen/AsmPrinter/ARMException.cpp

bool shouldEmitPersonality = forceEmitPersonality ||
     !MF->getLandingPads().empty();
   if (!Asm->MF->getFunction().needsUnwindTableEntry() &&
       !shouldEmitPersonality)
     ATS.emitCantUnwind();
   else if (shouldEmitPersonality) {
     // Emit references to personality.
     if (Per) {
       MCSymbol *PerSym = Asm->getSymbol(Per);
-      Asm->OutStreamer->emitSymbolAttribute(PerSym, MCSA_Global);
       ATS.emitPersonality(PerSym);
     }
 
     // Emit .handlerdata directive.
     ATS.emitHandlerData();
 
     // Emit actual exception table
     emitExceptionTable();