[llvm] [MC][ARM] Don't set funcs to Thumb as a side effect of .hidden (PR #181156)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-arm

Author: Simon Tatham (statham-arm)

<details>
<summary>Changes</summary>

When assembling a source file which switches between Arm and Thumb state using `.arm` and `.thumb`, if you defined a function in Arm state and mark it as hidden at dynamic link time using `.hidden`, but don't actually issue the `.hidden` directive until you have switched back to Thumb state, then the function would be accidentally marked as Thumb as a side effect of making it hidden.

This happened in `ARMELFStreamer::emitSymbolAttribute`, and the comment suggests that it was semi-deliberate: it was intended to happen as a side effect of `.type foo,%function`, because the function label might have already been defined without a type, and shouldn't be marked as Thumb until it's known that it's a function. But I think it was an accident that the same behavior also applies to any other addition of a symbol attribute, such as `.hidden`: the call to `setIsThumbFunc` was conditioned on whether the symbol has function type after setting the attribute, not whether function type was the attribute _actually being set_. So if you set the symbol to function type and _then_ use `.hidden`, the condition would match again.

Fixes #<!-- -->180358, in which this came up in real-world code: rustc's IR output included a top-level function in the form of inline asm, defined in Arm state, and marked it as hidden via the LLVM IR `declare`, so that LLVM didn't emit the `.hidden` directive until after it had switched back to Thumb state.

The new test case includes a check for the _intended_ behavior of the code in `emitSymbolAttribute`, to confirm that that still works.

---
Full diff: https://github.com/llvm/llvm-project/pull/181156.diff


2 Files Affected:

- (modified) llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp (+23-13) 
- (added) llvm/test/MC/ARM/hidden-thumb-function.s (+33) 


``````````diff
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
index 94b511a09e400..42b0569c2882b 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
@@ -600,22 +600,32 @@ class ARMELFStreamer : public MCELFStreamer {
     MCELFStreamer::emitValueImpl(Value, Size, Loc);
   }
 
-  /// If a label is defined before the .type directive sets the label's type
-  /// then the label can't be recorded as thumb function when the label is
-  /// defined. We override emitSymbolAttribute() which is called as part of the
-  /// parsing of .type so that if the symbol has already been defined we can
-  /// record the label as Thumb. FIXME: there is a corner case where the state
-  /// is changed in between the label definition and the .type directive, this
-  /// is not expected to occur in practice and handling it would require the
-  /// backend to track IsThumb for every label.
+  /// Called to set any attribute on a symbol.
+  ///
+  /// If this function is called for the .type directive that marks the symbol
+  /// as a function, and the label has been defined already without being typed
+  /// as a function, then this is the first opportunity we have to mark the
+  /// label as Thumb rather than Arm (if we're in Thumb mode).
+  ///
+  /// FIXME: there is a corner case where the state is changed in between the
+  /// label definition and the .type directive. This is not expected to occur
+  /// in practice, and handling it would require the backend to track IsThumb
+  /// for every label.
+  ///
+  /// We do not mark the symbol as Thumb due to any attributes other than
+  /// setting its type to 'function', because there _are_ cases in practice
+  /// where an attribute directive such as .hidden can be widely separated from
+  /// the symbol definition. (For example, bug #180358: rustc targeting mostly
+  /// Thumb generates a top-level Arm function entirely in inline assembly, and
+  /// then uses an LLVM IR `declare` statement to mark it as hidden symbol
+  /// visibility, which causes LLVM to emit a `.hidden` directive after having
+  /// switched back to Thumb mode.)
   bool emitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override {
     bool Val = MCELFStreamer::emitSymbolAttribute(Symbol, Attribute);
 
-    if (!IsThumb)
-      return Val;
-
-    unsigned Type = static_cast<MCSymbolELF *>(Symbol)->getType();
-    if ((Type == ELF::STT_FUNC || Type == ELF::STT_GNU_IFUNC) &&
+    if (IsThumb &&
+        (Attribute == MCSA_ELF_TypeFunction ||
+         Attribute == MCSA_ELF_TypeIndFunction) &&
         Symbol->isDefined())
       getAssembler().setIsThumbFunc(Symbol);
 
diff --git a/llvm/test/MC/ARM/hidden-thumb-function.s b/llvm/test/MC/ARM/hidden-thumb-function.s
new file mode 100644
index 0000000000000..ab43583d91059
--- /dev/null
+++ b/llvm/test/MC/ARM/hidden-thumb-function.s
@@ -0,0 +1,33 @@
+// RUN: llvm-mc --triple=thumbv7-none-eabi -filetype=obj %s | llvm-readelf -s - | FileCheck %s
+
+// Switch to Arm state to define a function, then switch back to Thumb
+// state before marking it as .hidden. We expect that the function is
+// still listed as Arm in the symbol table (low bit clear).
+
+        .arm
+        .type   hidden_arm_func, %function
+hidden_arm_func: bx lr
+
+        .thumb
+        .hidden hidden_arm_func
+
+// CHECK: 00000000 0 FUNC LOCAL HIDDEN {{[0-9]+}} hidden_arm_func
+
+// Define two function symbols in Thumb state, with the .type
+// directive before the label in one case and after it in the other.
+// We expect that both are marked as Thumb. (This was the _intended_
+// use of the 'mark as Thumb' behavior that was accidentally applying
+// to .hidden as well.)
+
+        .balign 4
+thumb_symbol_before_type:
+        .type thumb_symbol_before_type, %function
+        bx lr
+
+        .balign 4
+        .type thumb_symbol_after_type, %function
+thumb_symbol_after_type:
+        bx lr
+
+// CHECK: 00000005 0 FUNC LOCAL DEFAULT {{[0-9]+}} thumb_symbol_before_type
+// CHECK: 00000009 0 FUNC LOCAL DEFAULT {{[0-9]+}} thumb_symbol_after_type

``````````

</details>


https://github.com/llvm/llvm-project/pull/181156