[IR] Avoid the need to prefix MS C++ symbols with '\01'

[rnk]

Summary

Now the Windows mangling modes ('w' and 'x') do not do any mangling for
symbols starting with '?'. This means that clang can stop adding the
hideous '\01' leading escape. This means LLVM debug logs are less likely
to contain ASCII escape characters and it will be easier to copy and
paste MS symbol names from IR.

Finally.

For non-Windows platforms, names starting with '?' still get IR
mangling, so once clang stops escaping MS C++ names, we will get extra
'_' prefixing on MachO. That's fine, since it is currently impossible to
construct a triple that uses the MS C++ ABI in clang and emits macho
object files.

[rnk]

• Remove the check for stdcall/fastcall mangling

[majnemer]

LGTM but I'd like @rafael to look at this as well.

[• rafael]

This would change the result of

@"?foo" = private global i32 42

no? In particular it would miss the .L on ELF.

lib/IR/Mangler.cpp

118 ↗

(On Diff #20357)

You can move this before " Prefix = ..";

[rnk]

In D7775#127101, @rafael wrote:

This would change the result of

@"?foo" = private global i32 42

no? In particular it would miss the .L on ELF.

Yes. The lack of .L is concerning, but why have we been able to get by without the L prefix on COFF for so long with these names? Is there a bug there too? Maybe we should do the private symbol prefixing even in the presence of '?' or '\01'?

[• rafael]

no? In particular it would miss the .L on ELF.

Yes. The lack of .L is concerning, but why have we been able to get by without the L prefix on COFF for so long with these names? Is there a bug there too? Maybe we should do the private symbol prefixing even in the presence of '?' or '\01'?

I think the verifier should reject globals with a \0 name that are
internal or private. Globals with internal and private linkage can be
renamed. It makes no sense to force a particular name. I think I have
a wip patch for this on git. I will try to find it after lunch.

The objc implementation in clang used to break this, but I think it is
now fixed.

Not adding a L prefix for a genuine private global on COFF does look like a bug.

Cheers,
Rafael

[• rafael]

The WIP patch is attached. Unfortunately clang still produces bogus
GVs for objc. That would have to be fixed first.

• t.patch1 KBDownload

[rnk]

• Ensure symbols starting with '?' get private prefixes

[rnk]

So, I don't really want to step into the Obj-C vs. LLVM IR mangler mess. I fixed this patch so that symbols starting with '?' can get a private prefix.

This patch has some functionality change on MachO, where symbols that started with '?' will now lose their prefix, but I think that's acceptable. OK to commit?

[rnk]

Monday ping

[• rafael]

This needs at least three extra tests:

• Showing that ELF still have a .L prefix when given something like '@"?foo" private global i32 42'
• Similar for MachO, it should still get a L prefix
• A test showing the difference that now a MachO GV starting with ? will *not* get a _ prefix.

It might also be nice to have a langref/release notes update.

And last but not least, Grosbach is probably the one to approve the MachO change.

[rnk]

• Add langref, release notes, macho tests, and elf tests

[• rafael]

test/CodeGen/X86/mangle-question-mark.ll

23 ↗

(On Diff #21153)

You need to CHECK the non-stub too. Probably the easiest is to just check with global variables instead of functions.

[rnk]

test/CodeGen/X86/mangle-question-mark.ll

23 ↗

(On Diff #21153)

I'd rather just check the .indirect_symbol directive.

[rnk]

• Check the stubs

[rnk]

Thoughts, Jim?

[grosbach]

Conceptually, it's a bit odd to me that something for windows changes macho output all, but I can see why it happens here.

That aside, this is probably OK. I suggest also asking John McCall, as he knows far more than I about what sorts of cleverness front ends may get up to with symbol naming.

[rnk]

• rebase
• rewrite
• Leave MachO alone

[rnk]

PTAL, let's try to do this again. I'm tired of having to edit out the '\01' escape when reading IR names.

[smeenai]

<3

[rnk]

• Rename DataLayout helper
• Fix bug in non-GlobalValue mangling path found in clang indexer

[• espindola]

While having a symbol table resolved some of the issues, I still think we should aim for the IR having real symbol names one day. It would make it impossible for two GVs to have the same symbol and much easier to reason about which GVs are used from inline assembly.

So while this is an improvement, wouldn't it be better to try to switch the produces to using MM_None?

[rnk]

I guess the only consideration is, are we ready for clang to emit "_malloc", "_main", and "_printf". I'm not sure I have the consensus to make that change.

Clang already does the __fastcall etc mangling in the frontend. It would be nice to not have to add escapes there as well. As long as we have these escapes, there will be bugs.

I doubt there are any other frontends that care about the fastcall mangling, and we can provide this API if they want to reuse our mangling logic.

I would say this is at least an incremental step towards that. After this, I will update all the Clang IRGen test cases to not expect the '\01' escape for MS ABI code. Then there will be fewer cases to update when we eventually get consensus to go to MM_None. Reasonable?

[• espindola]

In D7775#1039687, @rnk wrote:

I guess the only consideration is, are we ready for clang to emit "_malloc", "_main", and "_printf". I'm not sure I have the consensus to make that change.

If I recall correctly, last time the only objection was from the MachO camp. Since the mangling mode is part of the datalayout now, we could leave MachO alone which would already at least simplify the implementation.

I would say this is at least an incremental step towards that. After this, I will update all the Clang IRGen test cases to not expect the '\01' escape for MS ABI code. Then there will be fewer cases to update when we eventually get consensus to go to MM_None. Reasonable?

Yes, I am sold. I am taking a look at the implementation.

[• espindola]

LGTM with just a nit.

llvm/lib/IR/Mangler.cpp

146 ↗

(On Diff #138653)

You don't need to set Prefix in here, just MSFunc. With that you also don't need the {}.

[smeenai]

llvm/docs/LangRef.rst

1957 ↗

(On Diff #138653)

To clarify, I assume x is used on x86 and w is used everywhere else. stdcall and fastcall are only a thing on x86, in which case w shouldn't be mangling them? (This is more confusing with the old version of the comment, but it's still ambiguous here.)

[rnk]

I hadn't considered switching just Windows to MM_None, that's is an interesting idea. Usually Windows has the most bugs involving '\01' because of how prevalent it is on Windows, but if we do MM_None that will all go away, and only Darwin will suffer them. =p

llvm/lib/IR/Mangler.cpp

146 ↗

(On Diff #138653)

Yep, will do.

[rnk]

llvm/docs/LangRef.rst

1957 ↗

(On Diff #138653)

__vectorcall gets @@N on x64, so the way I think of it is, all these conventions get manglings on Windows, but __fastcall and __stdcall are ignored on x64. It just so happens that we need to do some of this ignoring in the IR mangler, but we could just as easily reject these CCs in the verifier since they should be meaningless for the target.

llvm/trunk/docs/LangRef.rst

``f<size>:<abi>:<pref>``
     This specifies the alignment for a floating point type of a given bit
     ``<size>``. Only values of ``<size>`` that are supported by the target
     will work. 32 (float) and 64 (double) are supported on all targets; 80
     or 128 (different flavors of long double) are also supported on some
     targets.
 ``a:<abi>:<pref>``
     This specifies the alignment for an object of aggregate type.
 ``m:<mangling>``
-    If present, specifies that llvm names are mangled in the output. The
+    If present, specifies that llvm names are mangled in the output. Symbols
+    prefixed with the mangling escape character ``\01`` are passed through
+    directly to the assembler without the escape character. The mangling style
     options are
 
     * ``e``: ELF mangling: Private symbols get a ``.L`` prefix.
     * ``m``: Mips mangling: Private symbols get a ``$`` prefix.
     * ``o``: Mach-O mangling: Private symbols get ``L`` prefix. Other
       symbols get a ``_`` prefix.
-    * ``w``: Windows COFF prefix:  Similar to Mach-O, but stdcall and fastcall
-      functions also get a suffix based on the frame size.
-    * ``x``: Windows x86 COFF prefix:  Similar to Windows COFF, but use a ``_``
-      prefix for ``__cdecl`` functions.
+    * ``x``: Windows x86 COFF mangling: Private symbols get the usual prefix.
+      Regular C symbols get a ``_`` prefix. Functions with ``__stdcall``,
+      ``__fastcall``, and ``__vectorcall`` have custom mangling that appends
+      ``@N`` where N is the number of bytes used to pass parameters. C++ symbols
+      starting with ``?`` are not mangled in any way.
+    * ``w``: Windows COFF mangling: Similar to ``x``, except that normal C
+      symbols do not receive a ``_`` prefix.
 ``n<size1>:<size2>:<size3>...``
     This specifies a set of native integer widths for the target CPU in
     bits. For example, it might contain ``n32`` for 32-bit PowerPC,
     ``n32:64`` for PowerPC 64, or ``n8:16:32:64`` for X86-64. Elements of
     this set are considered to support most general arithmetic operations
     efficiently.
 ``ni:<address space0>:<address space1>:<address space2>...``
     This specifies pointer types with the specified address spaces

llvm/trunk/docs/ReleaseNotes.rst

.. NOTE
    For small 1-3 sentence descriptions, just add an entry at the end of
    this list. If your description won't fit comfortably in one bullet
    point (e.g. maybe you would like to give an example of the
    functionality, or simply have a lot to talk about), see the `NOTE` below
    for adding a new subsection.
 
 * The LoopInstSimplify pass (-loop-instsimplify) has been removed.
 
+* Symbols starting with ``?`` are no longer mangled by LLVM when using the
+  Windows ``x`` or ``w`` IR mangling schemes.
+
 * Note..
 
 .. NOTE
    If you would like to document a larger change, then you can add a
    subsection about it right here. You can copy the following boilerplate
    and un-indent it (the indentation causes it to be inside this comment).
 
    Special New Feature

llvm/trunk/include/llvm/IR/DataLayout.h

public:
   unsigned getAllocaAddrSpace() const { return AllocaAddrSpace; }
 
   unsigned getProgramAddressSpace() const { return ProgramAddrSpace; }
 
   bool hasMicrosoftFastStdCallMangling() const {
     return ManglingMode == MM_WinCOFFX86;
   }
 
+  /// Returns true if symbols with leading question marks should not receive IR
+  /// mangling. True for Windows mangling modes.
+  bool doNotMangleLeadingQuestionMark() const {
+    return ManglingMode == MM_WinCOFF || ManglingMode == MM_WinCOFFX86;
+  }
+
   bool hasLinkerPrivateGlobalPrefix() const { return ManglingMode == MM_MachO; }
 
   StringRef getLinkerPrivateGlobalPrefix() const {
     if (ManglingMode == MM_MachO)
       return "l";
     return "";
   }
 

llvm/trunk/lib/IR/Mangler.cpp

static void getNameWithPrefixImpl(raw_ostream &OS, const Twine &GVName,
 
   // No need to do anything special if the global has the special "do not
   // mangle" flag in the name.
   if (Name[0] == '\1') {
     OS << Name.substr(1);
     return;
   }
 
+  if (DL.doNotMangleLeadingQuestionMark() && Name[0] == '?')
+    Prefix = '\0';
+
   if (PrefixTy == Private)
     OS << DL.getPrivateGlobalPrefix();
   else if (PrefixTy == LinkerPrivate)
     OS << DL.getLinkerPrivateGlobalPrefix();
 
   if (Prefix != '\0')
     OS << Prefix;
 
void Mangler::getNameWithPrefix(raw_ostream &OS, const GlobalValue *GV,
   }
 
   StringRef Name = GV->getName();
   char Prefix = DL.getGlobalPrefix();
 
   // Mangle functions with Microsoft calling conventions specially.  Only do
   // this mangling for x86_64 vectorcall and 32-bit x86.
   const Function *MSFunc = dyn_cast<Function>(GV);
-  if (Name.startswith("\01"))
-    MSFunc = nullptr; // Don't mangle when \01 is present.
+
+  // Don't add byte count suffixes when '\01' or '?' are in the first
+  // character.
+  if (Name.startswith("\01") ||
+      (DL.doNotMangleLeadingQuestionMark() && Name.startswith("?")))
+    MSFunc = nullptr;
+
   CallingConv::ID CC =
       MSFunc ? MSFunc->getCallingConv() : (unsigned)CallingConv::C;
   if (!DL.hasMicrosoftFastStdCallMangling() &&
       CC != CallingConv::X86_VectorCall)
     MSFunc = nullptr;
   if (MSFunc) {
     if (CC == CallingConv::X86_FastCall)
       Prefix = '@'; // fastcall functions have an @ prefix instead of _.

llvm/trunk/test/CodeGen/X86/mangle-question-mark.ll

+; Test that symbols starting with '?' are not affected by IR mangling.
+
+; RUN: llc -mtriple i686-pc-win32 < %s | FileCheck %s --check-prefix=COFF
+; RUN: llc -mtriple x86_64-pc-win32 < %s | FileCheck %s --check-prefix=COFF64
+; RUN: llc -mtriple i686-linux-gnu < %s | FileCheck %s --check-prefix=ELF
+; RUN: llc -mtriple i686-apple-darwin < %s | FileCheck %s --check-prefix=MACHO
+
+; Currently all object files allow escaping private symbols, but eventually we
+; might want to reject that.
+
+; COFF: calll "?withescape@A@@QBEXXZ"
+; COFF: calll "?withquestion@A@@QBEXXZ"
+; COFF: calll "L?privatequestion@A@@QBEXXZ"
+; COFF: calll "L?privatequestionfast@A@@QBEXXZ"
+; COFF: calll "?escapedprivate@A@@QBEXXZ"
+
+; COFF64: callq "?withescape@A@@QBEXXZ"
+; COFF64: callq "?withquestion@A@@QBEXXZ"
+; COFF64: callq ".L?privatequestion@A@@QBEXXZ"
+; COFF64: callq ".L?privatequestionfast@A@@QBEXXZ"
+; COFF64: callq "?escapedprivate@A@@QBEXXZ"
+
+; ELF: calll "?withescape@A@@QBEXXZ"
+; ELF: calll "?withquestion@A@@QBEXXZ"
+; ELF: calll ".L?privatequestion@A@@QBEXXZ"
+; ELF: calll ".L?privatequestionfast@A@@QBEXXZ"
+; ELF: calll "?escapedprivate@A@@QBEXXZ"
+
+; MACHO: calll "?withescape@A@@QBEXXZ"
+; MACHO: calll "_?withquestion@A@@QBEXXZ"
+; MACHO: calll "l_?privatequestion@A@@QBEXXZ"
+; MACHO: calll "l_?privatequestionfast@A@@QBEXXZ"
+; MACHO: calll "?escapedprivate@A@@QBEXXZ"
+
+%struct.A = type {}
+
+define i32 @main() {
+entry:
+  tail call void @"\01?withescape@A@@QBEXXZ"(%struct.A* null)
+  tail call void @"?withquestion@A@@QBEXXZ"(%struct.A* null)
+  tail call void @"?privatequestion@A@@QBEXXZ"(%struct.A* null)
+  tail call x86_fastcallcc void @"?privatequestionfast@A@@QBEXXZ"(%struct.A* null)
+  tail call void @"\01?escapedprivate@A@@QBEXXZ"(%struct.A* null)
+  ret i32 0
+}
+
+declare void @"\01?withescape@A@@QBEXXZ"(%struct.A*)
+declare void @"?withquestion@A@@QBEXXZ"(%struct.A*)
+
+define private void @"?privatequestion@A@@QBEXXZ"(%struct.A*) {
+  ret void
+}
+
+define private x86_fastcallcc void @"?privatequestionfast@A@@QBEXXZ"(%struct.A*) {
+  ret void
+}
+
+define private void @"\01?escapedprivate@A@@QBEXXZ"(%struct.A*) {
+  ret void
+}

llvm/trunk/test/MC/COFF/symbol-mangling.ll

-; The purpose of this test is to see if the MC layer properly handles symbol
-; names needing quoting on MS/Windows. This code is generated by clang when
-; using -cxx-abi microsoft.
-
-; RUN: llc -filetype=asm -mtriple i686-pc-win32 %s -o - | FileCheck %s
-
-; CHECK: ?sayhi@A@@QBEXXZ
-
-%struct.A = type {}
-
-define i32 @main() {
-entry:
-  tail call void @"\01?sayhi@A@@QBEXXZ"(%struct.A* null)
-  ret i32 0
-}
-
-declare void @"\01?sayhi@A@@QBEXXZ"(%struct.A*)

llvm/trunk/unittests/IR/CMakeLists.txt

set(IRSources
   DominatorTreeBatchUpdatesTest.cpp
   FunctionTest.cpp
   PassBuilderCallbacksTest.cpp
   IRBuilderTest.cpp
   InstructionsTest.cpp
   IntrinsicsTest.cpp
   LegacyPassManagerTest.cpp
   MDBuilderTest.cpp
+  ManglerTest.cpp
   MetadataTest.cpp
   ModuleTest.cpp
   PassManagerTest.cpp
   PatternMatch.cpp
   TypeBuilderTest.cpp
   TypesTest.cpp
   UseTest.cpp
   UserTest.cpp

llvm/trunk/unittests/IR/ManglerTest.cpp

+//===- llvm/unittest/IR/ManglerTest.cpp - Mangler unit tests --------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/Mangler.h"
+#include "llvm/IR/CallingConv.h"
+#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/GlobalValue.h"
+#include "llvm/IR/Module.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+static std::string mangleStr(StringRef IRName, Mangler &Mang,
+                             const DataLayout &DL) {
+  std::string Mangled;
+  raw_string_ostream SS(Mangled);
+  Mang.getNameWithPrefix(SS, IRName, DL);
+  SS.flush();
+  return Mangled;
+}
+
+static std::string mangleFunc(StringRef IRName,
+                              GlobalValue::LinkageTypes Linkage,
+                              llvm::CallingConv::ID CC, Module &Mod,
+                              Mangler &Mang) {
+  Type *VoidTy = Type::getVoidTy(Mod.getContext());
+  Type *I32Ty = Type::getInt32Ty(Mod.getContext());
+  FunctionType *FTy =
+      FunctionType::get(VoidTy, {I32Ty, I32Ty, I32Ty}, /*isVarArg=*/false);
+  Function *F = Function::Create(FTy, Linkage, IRName, &Mod);
+  F->setCallingConv(CC);
+  std::string Mangled;
+  raw_string_ostream SS(Mangled);
+  Mang.getNameWithPrefix(SS, F, false);
+  SS.flush();
+  F->eraseFromParent();
+  return Mangled;
+}
+
+namespace {
+
+TEST(ManglerTest, MachO) {
+  LLVMContext Ctx;
+  DataLayout DL("m:o"); // macho
+  Module Mod("test", Ctx);
+  Mod.setDataLayout(DL);
+  Mangler Mang;
+  EXPECT_EQ(mangleStr("foo", Mang, DL), "_foo");
+  EXPECT_EQ(mangleStr("\01foo", Mang, DL), "foo");
+  EXPECT_EQ(mangleStr("?foo", Mang, DL), "_?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "_foo");
+  EXPECT_EQ(mangleFunc("?foo", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "_?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::PrivateLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "L_foo");
+}
+
+TEST(ManglerTest, WindowsX86) {
+  LLVMContext Ctx;
+  DataLayout DL("m:x-p:32:32"); // 32-bit windows
+  Module Mod("test", Ctx);
+  Mod.setDataLayout(DL);
+  Mangler Mang;
+  EXPECT_EQ(mangleStr("foo", Mang, DL), "_foo");
+  EXPECT_EQ(mangleStr("\01foo", Mang, DL), "foo");
+  EXPECT_EQ(mangleStr("?foo", Mang, DL), "?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "_foo");
+  EXPECT_EQ(mangleFunc("?foo", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::PrivateLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "L_foo");
+
+  // Test calling conv mangling.
+  EXPECT_EQ(mangleFunc("stdcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_StdCall, Mod, Mang),
+            "_stdcall@12");
+  EXPECT_EQ(mangleFunc("fastcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_FastCall, Mod, Mang),
+            "@fastcall@12");
+  EXPECT_EQ(mangleFunc("vectorcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_VectorCall, Mod, Mang),
+            "vectorcall@@12");
+
+  // Adding a '?' prefix blocks calling convention mangling.
+  EXPECT_EQ(mangleFunc("?fastcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_FastCall, Mod, Mang),
+            "?fastcall");
+}
+
+TEST(ManglerTest, WindowsX64) {
+  LLVMContext Ctx;
+  DataLayout DL("m:w-p:64:64"); // windows
+  Module Mod("test", Ctx);
+  Mod.setDataLayout(DL);
+  Mangler Mang;
+  EXPECT_EQ(mangleStr("foo", Mang, DL), "foo");
+  EXPECT_EQ(mangleStr("\01foo", Mang, DL), "foo");
+  EXPECT_EQ(mangleStr("?foo", Mang, DL), "?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "foo");
+  EXPECT_EQ(mangleFunc("?foo", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            "?foo");
+  EXPECT_EQ(mangleFunc("foo", llvm::GlobalValue::PrivateLinkage,
+                       llvm::CallingConv::C, Mod, Mang),
+            ".Lfoo");
+
+  // Test calling conv mangling.
+  EXPECT_EQ(mangleFunc("stdcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_StdCall, Mod, Mang),
+            "stdcall");
+  EXPECT_EQ(mangleFunc("fastcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_FastCall, Mod, Mang),
+            "fastcall");
+  EXPECT_EQ(mangleFunc("vectorcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_VectorCall, Mod, Mang),
+            "vectorcall@@24");
+
+  // Adding a '?' prefix blocks calling convention mangling.
+  EXPECT_EQ(mangleFunc("?vectorcall", llvm::GlobalValue::ExternalLinkage,
+                       llvm::CallingConv::X86_VectorCall, Mod, Mang),
+            "?vectorcall");
+}
+
+} // end anonymous namespace