Skip to content

MC: Support quoted symbol names #138817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 8, 2025

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented May 7, 2025

gas has supported " quoted symbols since 2015:
https://sourceware.org/pipermail/binutils/2015-August/090003.html

We don't handle \ or " , leading to clang -c --save-temps vs clang -c
difference for the following C code:

int x asm("a\"\\b");

Fix #138390

MC/COFF/safeseh.h looks incorrect. \01 in .safeseh "\01foo" is not a
correct escape sequence. Change it to \

Created using spr 1.3.5-bogner
@MaskRay MaskRay requested a review from brad0 May 7, 2025 07:55
@llvmbot llvmbot added the mc Machine (object) code label May 7, 2025
@llvmbot
Copy link
Member

llvmbot commented May 7, 2025

@llvm/pr-subscribers-mc

Author: Fangrui Song (MaskRay)

Changes

gas has supported " quoted symbols since 2015:
https://sourceware.org/pipermail/binutils/2015-August/090003.html

Close #138390


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

2 Files Affected:

  • (modified) llvm/lib/MC/MCContext.cpp (+21)
  • (modified) llvm/test/MC/ELF/symbol-names.s (+16-3)
diff --git a/llvm/lib/MC/MCContext.cpp b/llvm/lib/MC/MCContext.cpp
index f70087e14f702..41caf20b331b2 100644
--- a/llvm/lib/MC/MCContext.cpp
+++ b/llvm/lib/MC/MCContext.cpp
@@ -212,6 +212,27 @@ MCDataFragment *MCContext::allocInitialFragment(MCSection &Sec) {
 MCSymbol *MCContext::getOrCreateSymbol(const Twine &Name) {
   SmallString<128> NameSV;
   StringRef NameRef = Name.toStringRef(NameSV);
+  if (NameRef.contains('\\')) {
+    NameSV = NameRef;
+    size_t S = 0;
+    // Support escaped \\ and \" as in GNU Assembler. GAS issues a warning for
+    // other characters following \\, which we do not implement due to code
+    // structure.
+    for (size_t I = 0, E = NameSV.size(); I < E; ++I) {
+      char C = NameSV[I];
+      if (C == '\\') {
+        switch (NameSV[I + 1]) {
+        case '"':
+        case '\\':
+          C = NameSV[++I];
+          break;
+        }
+      }
+      NameSV[S++] = C;
+    }
+    NameSV.resize(S);
+    NameRef = NameSV;
+  }
 
   assert(!NameRef.empty() && "Normal symbols cannot be unnamed!");
 
diff --git a/llvm/test/MC/ELF/symbol-names.s b/llvm/test/MC/ELF/symbol-names.s
index f605c723d4d4d..8c891052ebd9a 100644
--- a/llvm/test/MC/ELF/symbol-names.s
+++ b/llvm/test/MC/ELF/symbol-names.s
@@ -1,12 +1,25 @@
-// RUN: llvm-mc -triple i686-pc-linux -filetype=obj %s -o - | llvm-readobj --symbols - | FileCheck %s
+// RUN: llvm-mc -triple=x86_64 -filetype=obj %s | llvm-objdump -tdr - | FileCheck %s
 
 // MC allows ?'s in symbol names as an extension.
 
+// CHECK-LABEL:SYMBOL TABLE:
+// CHECK-NEXT: 0000000000000001 l     F .text  0000000000000000 a"b\{{$}}
+// CHECK-NEXT: 0000000000000000 g     F .text  0000000000000000 foo?bar
+// CHECK-NEXT: 0000000000000000 *UND*          0000000000000000 a"b\q{{$}}
+// CHECK-EMPTY:
+
 .text
 .globl foo?bar
 .type foo?bar, @function
 foo?bar:
 ret
 
-// CHECK: Symbol
-// CHECK: Name: foo?bar
+// CHECK-LABEL:<a"b\>:
+// CHECK-NEXT:   callq  {{.*}} <a"b\>
+// CHECK-NEXT:   callq  {{.*}}
+// CHECK-NEXT:     R_X86_64_PLT32 a"b\q-0x4
+.type "a\"b\\", @function
+"a\"b\\":
+  call "a\"b\\"
+/// GAS emits a warning for \q
+  call "a\"b\q"

MaskRay added 2 commits May 7, 2025 21:20
Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit 87db094 into main May 8, 2025
6 of 9 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/mc-support-quoted-symbol-names branch May 8, 2025 06:08
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 8, 2025
gas has supported " quoted symbols since 2015:
https://sourceware.org/pipermail/binutils/2015-August/090003.html

We don't handle \\ or \" , leading to clang -c --save-temps vs clang -c
difference for the following C code:

```
int x asm("a\"\\b");
```

Fix #138390

MC/COFF/safeseh.h looks incorrect. \01 in `.safeseh "\01foo"` is not a
correct escape sequence. Change it to \\

Pull Request: llvm/llvm-project#138817
petrhosek pushed a commit to petrhosek/llvm-project that referenced this pull request May 8, 2025
gas has supported " quoted symbols since 2015:
https://sourceware.org/pipermail/binutils/2015-August/090003.html

We don't handle \\ or \" , leading to clang -c --save-temps vs clang -c
difference for the following C code:

```
int x asm("a\"\\b");
```

Fix llvm#138390

MC/COFF/safeseh.h looks incorrect. \01 in `.safeseh "\01foo"` is not a
correct escape sequence. Change it to \\

Pull Request: llvm#138817
@joker-eph
Copy link
Collaborator

This broke the bots it seems: https://lab.llvm.org/buildbot/#/builders/153/builds/31071

joker-eph added a commit that referenced this pull request May 9, 2025
Reverts #138817

The BOLT testing is failing after this change.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 9, 2025
MaskRay added a commit that referenced this pull request May 10, 2025
gas has supported " quoted symbols since 2015.
Both \ and " need to be escaped.
https://sourceware.org/pipermail/binutils/2015-August/090003.html

We don't unescape \\ or \" in assembly strings, leading to clang -c
--save-temps vs clang -c difference for the following C code:

```
int x asm("a\"\\b");
```

Fix #138390

MC/COFF/safeseh.h looks incorrect. \01 in `.safeseh "\01foo"` is not a
correct escape sequence. Change it to \\

Pull Request: #138817
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 10, 2025
gas has supported " quoted symbols since 2015.
Both \ and " need to be escaped.
https://sourceware.org/pipermail/binutils/2015-August/090003.html

We don't unescape \\ or \" in assembly strings, leading to clang -c
--save-temps vs clang -c difference for the following C code:

```
int x asm("a\"\\b");
```

Fix #138390

MC/COFF/safeseh.h looks incorrect. \01 in `.safeseh "\01foo"` is not a
correct escape sequence. Change it to \\

Pull Request: llvm/llvm-project#138817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

llvm assembler does not emit the correct symbol names when escaped characters are present
4 participants