[Mangle] Add flag to asm labels to disable '\01' prefixing

[vsk]

Summary

LLDB synthesizes decls using asm labels. These decls cannot have a mangle
different than the one specified in the label name. I.e., the '\01' prefix
should not be added.

Fixes an expression evaluation failure in lldb's TestVirtual.py on iOS.

rdar://45827323

[rjmccall]

clang/lib/AST/Mangle.cpp

137 ↗

(On Diff #220899)

This is one of those bugs where looking at the code really reveals a lot of problematic behavior.

AsmLabelAttr is generally assumed to carry a literal label, i.e. it should not have the global prefix added to it. We should not be changing that assumption globally just based on whether an ExternalASTSource has been set up; that's going to break in any sort of non-uniform situation, e.g. if an LLDB user writes a declaration with an asm label, or if we import a Clang module where a declaration has an asm label. Either ExternalASTSources should be putting the real (prefixed) symbol name in the AsmLabelAttr, or they should be making AsmLabelAttrs that are different somehow, e.g. by giving AsmLabelAttr a flag (which doesn't need to be spellable in the source language) saying whether the label is literal or subject to global prefixing.

Separately, it seems to me that if the AsmLabelAttr is literal but the label happens to include the global prefix, we should strip the prefix from the label and not add a \01 so that we get a consistent symbol name in LLVM.

[teemperor]

I wonder what's the motivation for making this a setting in the ExternalASTSource? Opposed to e.g. storing a bit in AsmLabelAttr, which we could squeeze in by maybe stealing a bit from labelLength? Having this as a global setting in the ExternalASTSource seems like it could end up in a lot of confusion when we refactor our ExternalASTSources and this unexpectedly breaks. Especially since multiplexing ExternalASTSources is a thing people do (including LLDB itself), so if one source wants this feature on and the other doesn't, then we are in a dead end.

Also I wonder what happens with our real declarations we get from Clang modules (ObjC for now and C++ in the future). They now also share this setting even though they could use asm labels for legitimate reasons (even though I'm not sure I ever saw one being used in practice).

One minor bug I found: You need to turn this on in SemaSourceWithPriorities and maybe also ExternalASTSourceWrapper (see ASTUtils.h) otherwise this seems to regress when using C++ modules and mangling stuff for an expression (I couldn't use ClangExternalASTSourceCommon there as we needed a clang::SemaSource).

[vsk]

Thanks for your feedback. I've added a flag to asm labels to disable global prefixing. I've tried to minimize the behavior change in this patch -- it seems to me that additional cleanups can happen in follow-ups.

[rjmccall]

clang/include/clang/Basic/Attr.td

725 ↗

(On Diff #221066)

Does this actually change how it can be written in source? I think it might be a good idea to do that in the long run, but let's not in this patch.

Also something really needs to document what this is actually for.

clang/lib/AST/Mangle.cpp

137 ↗

(On Diff #220899)

Please check for whether the label is literal first before pulling out the global prefix.

clang/lib/Sema/SemaDecl.cpp

2770 ↗

(On Diff #221066)

This is definitely not a safe assumption; for one, we might eventually allow this to be spelled in source, and for another, you can redeclare something from the global context in LLDB and therefore get this kind of mismatch. Anyway, I don't think you need the assumption here; just add a method to AsmLabelAttr that checks for equality in a way that compensates for the inclusion/suppression of the global prefix.

[vsk]

• Address latest review feedback.

[rjmccall]

clang/include/clang/Basic/Attr.td

734 ↗

(On Diff #221391)

LiteralLabel is an unfortunate name for this property because getLiteralLabel sounds like it ought to return a string. How about IsLiteralLabel?

Also there's a "fake" flag to make it clear that this argument isn't (currently) written in source, so this should be BoolArgument<"IsLiteralLabel", /*optional*/ 0, /*fake*/ 1>.

Comment suggestion:

IsLiteralLabel specifies whether the label is literal (i.e. suppresses the global C symbol prefix) or not.

740 ↗

(On Diff #221391)

isEquivalent, please.

clang/include/clang/Basic/AttrDocs.td

2579 ↗

(On Diff #221391)

Oh, thank you for adding documentation. I think the prefixing thing probably ought to be explained here; here's a suggestion for some wording:

This attribute can be used on a function or variable to specify its symbol name.

On some targets, all C symbols are prefixed by default with a single character, typically ``_``.  This was done historically to distinguish them from symbols used by other languages.  (This prefix is also added to the standard Itanium C++ ABI prefix on "mangled" symbol names, so that e.g. on such targets the true symbol name for a C++ variable declared as ``int cppvar;`` would be ``__Z6cppvar``; note the two underscores.)  This prefix is *not* added to the symbol names specified by the ``asm`` attribute; programmers wishing to match a C symbol name must compensate for this.

For example, consider the following C code:

<example>

Clang's implementation of this attribute is compatible with GCC's, `documented here <https://gcc.gnu.org/onlinedocs/gcc/Asm-Labels.html>`_.

While it is possible to use this attribute to name a special symbol used internally by the compiler, such as an LLVM intrinsic, this is neither recommended nor supported and may cause the compiler to crash or miscompile.  Users who wish to gain access to intrinsic behavior are strongly encouraged to request new builtin functions.

clang/lib/AST/Mangle.cpp

130 ↗

(On Diff #221391)

This is actually backwards, right? A literal label is one that doesn't get the global prefix and therefore potentially needs the \01 prefix to suppress it.

[vsk]

clang/lib/AST/Mangle.cpp

130 ↗

(On Diff #221391)

Thank you for the careful and helpful review! Before updating this patch, I'd like to make sure we're on the same page on this point.

What I'm searching for is a way to prevent mangleName from adding either a \01 prefix or the global prefix to the label contained within an AsmLabelAttr. As the result from mangleName is passed to a hash function, omitting both prefixes is necessary to fix the lldb expression evaluation failure mentioned in the summary. Based on (probably a bad reading of) earlier feedback, I thought that marking an attr as "Literal" would be the preferred method to suppress all prefixes.

However, it sounds like you believe:

1. An attr marked as "Literal" should have a \01 prefix applied, but no global prefix.
2. An attr *not* marked as "Literal" should have a global prefix applied (if one is available).

If I've characterized your view correctly, then we really need something else to denote "do not add any kind of prefix". One option is to use BoolArgument<"IsUnprefixed", /*optional=*/0, /*fake=*/1>, and set the literal/non-literal distinction aside.

OTOH, if what you really meant was that mangleName should omit all prefixes for non-"Literal" labels (and that "Literal" is the right name/distinction for this idea), I can simply invert the condition I've been using.

[rjmccall]

clang/lib/AST/Mangle.cpp

130 ↗

(On Diff #221391)

I'm just asking you to invert the condition. At the user level, a "literal" label is one that should be taken "literally", as in, it shouldn't have the global prefix. The \01 prefix is not part of the user-level semantics of asm labels; it's just part of the representation of symbols in LLVM IR, and mangleName should add it or not depending on what gets the right behavior from LLVM. Ultimately, that will include canonicalizing labels so that an asm label that starts with the global prefix (e.g. asm("_foo")) will just be translated appropriately (to @foo rather than @\01_foo), but you've reasonably asked not to take that on in this patch, so all you need to do is make sure that we're only adding \01 when the label is literal and there actually is a global prefix.

[vsk]

• Address latest round of comments.

[rjmccall]

Thanks. One last and minor request: please mention in the comment on IsLiteralLabel that non-literal labels are used by some external AST sources like LLDB.

[teemperor]

This LGTM modulo rjmccall's comment.

[vsk]

• Add a comment describing where non-literal labels are used.

[rjmccall]

Thanks, LGTM.

[aaron.ballman]

cfe/trunk/lib/AST/Mangle.cpp

127

Sorry to dredge up an old review, but I recently ran into a bug in this area and am not certain of how to fix it. What should happen if the asm label is a literal which is empty? e.g.,

void foo(void) __asm__("");
void foo(void) {}

Currently, that seems to get through this code path and stream an empty string into the output stream. However, writeFuncOrVarName() doesn't check that it actually wrote anything into the stream when calling mangleName() and returns success to the caller. This causes writeName() to eventually call llvm::Mangler::getNameWithPrefix(), which asserts that the passed StringRef is not an empty string and we crash. The typical LLVM code path seems to treat unnamed symbols as being mangled with __unnamed_N (where N is replaced), but this relies on having a GlobalValue* available to get the next mangling ID.

I'm not certain of where to fix the bug. Should the LLVM side be able to accept an empty string and try to gin up a GlobalValue* to mangle it properly? Should the FE side not generate an empty name here? Should empty asm labels be rejected at the source level?

Thanks for any insights you can provide!

cfe/trunk/include/clang/Basic/Attr.td

 def AVRSignal : InheritableAttr, TargetSpecificAttr<TargetAVR> {
   let Spellings = [GCC<"signal">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [AVRSignalDocs];
 }
 
 def AsmLabel : InheritableAttr {
   let Spellings = [Keyword<"asm">, Keyword<"__asm__">];
-  let Args = [StringArgument<"Label">];
+  let Args = [
+    // Label specifies the mangled name for the decl.
+    StringArgument<"Label">,
+
+    // IsLiteralLabel specifies whether the label is literal (i.e. suppresses
+    // the global C symbol prefix) or not. If not, the mangle-suppression prefix
+    // ('\01') is omitted from the decl name at the LLVM IR level.
+    //
+    // Non-literal labels are used by some external AST sources like LLDB.
+    BoolArgument<"IsLiteralLabel", /*optional=*/0, /*fake=*/1>
+  ];
   let SemaHandler = 0;
-  let Documentation = [Undocumented];
+  let Documentation = [AsmLabelDocs];
+  let AdditionalMembers =
+[{
+bool isEquivalent(AsmLabelAttr *Other) const {
+  return getLabel() == Other->getLabel() && getIsLiteralLabel() == Other->getIsLiteralLabel();
+}
+}];
 }
 
 def Availability : InheritableAttr {
   let Spellings = [Clang<"availability">];
   let Args = [IdentifierArgument<"platform">, VersionArgument<"introduced">,
               VersionArgument<"deprecated">, VersionArgument<"obsoleted">,
               BoolArgument<"unavailable">, StringArgument<"message">,
               BoolArgument<"strict">, StringArgument<"replacement">,

cfe/trunk/include/clang/Basic/AttrDocs.td

def FlagEnumDocs : Documentation {
   let Content = [{
 This attribute can be added to an enumerator to signal to the compiler that it
 is intended to be used as a flag type. This will cause the compiler to assume
 that the range of the type includes all of the values that you can get by
 manipulating bits of the enumerator when issuing warnings.
   }];
 }
 
+def AsmLabelDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+This attribute can be used on a function or variable to specify its symbol name.
+
+On some targets, all C symbols are prefixed by default with a single character, typically ``_``.  This was done historically to distinguish them from symbols used by other languages.  (This prefix is also added to the standard Itanium C++ ABI prefix on "mangled" symbol names, so that e.g. on such targets the true symbol name for a C++ variable declared as ``int cppvar;`` would be ``__Z6cppvar``; note the two underscores.)  This prefix is *not* added to the symbol names specified by the ``asm`` attribute; programmers wishing to match a C symbol name must compensate for this.
+
+For example, consider the following C code:
+
+.. code-block:: c
+
+  int var1 asm("altvar") = 1;  // "altvar" in symbol table.
+  int var2 = 1; // "_var2" in symbol table.
+
+  void func1(void) asm("altfunc");
+  void func1(void) {} // "altfunc" in symbol table.
+  void func2(void) {} // "_func2" in symbol table.
+
+Clang's implementation of this attribute is compatible with GCC's, `documented here <https://gcc.gnu.org/onlinedocs/gcc/Asm-Labels.html>`_.
+
+While it is possible to use this attribute to name a special symbol used internally by the compiler, such as an LLVM intrinsic, this is neither recommended nor supported and may cause the compiler to crash or miscompile.  Users who wish to gain access to intrinsic behavior are strongly encouraged to request new builtin functions.
+  }];
+}
+
 def EnumExtensibilityDocs : Documentation {
   let Category = DocCatDecl;
   let Content = [{
 Attribute ``enum_extensibility`` is used to distinguish between enum definitions
 that are extensible and those that are not. The attribute can take either
 ``closed`` or ``open`` as an argument. ``closed`` indicates a variable of the
 enum type takes a value that corresponds to one of the enumerators listed in the
 enum definition or, when the enum is annotated with ``flag_enum``, a value that

cfe/trunk/lib/AST/Mangle.cpp

 }
 
 void MangleContext::mangleName(const NamedDecl *D, raw_ostream &Out) {
   // Any decl can be declared with __asm("foo") on it, and this takes precedence
   // over all other naming in the .o file.
   if (const AsmLabelAttr *ALA = D->getAttr<AsmLabelAttr>()) {
     // If we have an asm name, then we use it as the mangling.
 
+    // If the label isn't literal, or if this is an alias for an LLVM intrinsic,
+    // do not add a "\01" prefix.
+    if (!ALA->getIsLiteralLabel() || ALA->getLabel().startswith("llvm.")) {
+      Out << ALA->getLabel();
+      return;
+    }
+
     // Adding the prefix can cause problems when one file has a "foo" and
     // another has a "\01foo". That is known to happen on ELF with the
     // tricks normally used for producing aliases (PR9177). Fortunately the
     // llvm mangler on ELF is a nop, so we can just avoid adding the \01
-    // marker.  We also avoid adding the marker if this is an alias for an
-    // LLVM intrinsic.
+    // marker.
     char GlobalPrefix =
         getASTContext().getTargetInfo().getDataLayout().getGlobalPrefix();
-    if (GlobalPrefix && !ALA->getLabel().startswith("llvm."))
+    if (GlobalPrefix)
       Out << '\01'; // LLVM IR Marker for __asm("foo")
 
     Out << ALA->getLabel();
     return;
   }
 
   const ASTContext &ASTContext = getASTContext();
   CCMangling CC = getCallingConvMangling(ASTContext, D);

cfe/trunk/lib/Sema/SemaDecl.cpp

if (bool(OldConstInit) != bool(NewConstInit)) {
     }
   }
 
   // Attributes declared post-definition are currently ignored.
   checkNewAttributesAfterDef(*this, New, Old);
 
   if (AsmLabelAttr *NewA = New->getAttr<AsmLabelAttr>()) {
     if (AsmLabelAttr *OldA = Old->getAttr<AsmLabelAttr>()) {
-      if (OldA->getLabel() != NewA->getLabel()) {
+      if (!OldA->isEquivalent(NewA)) {
         // This redeclaration changes __asm__ label.
         Diag(New->getLocation(), diag::err_different_asm_label);
         Diag(OldA->getLocation(), diag::note_previous_declaration);
       }
     } else if (Old->isUsed()) {
       // This redeclaration adds an __asm__ label to a declaration that has
       // already been ODR-used.
       Diag(New->getLocation(), diag::err_late_asm_label_name)
if (S->getFnParent() != nullptr) {
       }
 
       if (!R->isIntegralType(Context) && !R->isPointerType()) {
         Diag(D.getBeginLoc(), diag::err_asm_bad_register_type);
         NewVD->setInvalidDecl(true);
       }
     }
 
-    NewVD->addAttr(::new (Context)
-                       AsmLabelAttr(Context, SE->getStrTokenLoc(0), Label));
+    NewVD->addAttr(::new (Context) AsmLabelAttr(
+        Context, SE->getStrTokenLoc(0), Label, /*IsLiteralLabel=*/true));
   } else if (!ExtnameUndeclaredIdentifiers.empty()) {
     llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
       ExtnameUndeclaredIdentifiers.find(NewVD->getIdentifier());
     if (I != ExtnameUndeclaredIdentifiers.end()) {
       if (isDeclExternC(NewVD)) {
         NewVD->addAttr(I->second);
         ExtnameUndeclaredIdentifiers.erase(I);
       } else
FilterLookupForScope(Previous, OriginalDC, S, shouldConsiderLinkage(NewFD),
                        D.getCXXScopeSpec().isNotEmpty() ||
                        isMemberSpecialization ||
                        isFunctionTemplateSpecialization);
 
   // Handle GNU asm-label extension (encoded as an attribute).
   if (Expr *E = (Expr*) D.getAsmLabel()) {
     // The parser guarantees this is a string.
     StringLiteral *SE = cast<StringLiteral>(E);
-    NewFD->addAttr(::new (Context) AsmLabelAttr(Context, SE->getStrTokenLoc(0),
-                                                SE->getString()));
+    NewFD->addAttr(::new (Context)
+                       AsmLabelAttr(Context, SE->getStrTokenLoc(0),
+                                    SE->getString(), /*IsLiteralLabel=*/true));
   } else if (!ExtnameUndeclaredIdentifiers.empty()) {
     llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
       ExtnameUndeclaredIdentifiers.find(NewFD->getIdentifier());
     if (I != ExtnameUndeclaredIdentifiers.end()) {
       if (isDeclExternC(NewFD)) {
         NewFD->addAttr(I->second);
         ExtnameUndeclaredIdentifiers.erase(I);
       } else
void Sema::ActOnPragmaRedefineExtname(IdentifierInfo* Name,
                                       IdentifierInfo* AliasName,
                                       SourceLocation PragmaLoc,
                                       SourceLocation NameLoc,
                                       SourceLocation AliasNameLoc) {
   NamedDecl *PrevDecl = LookupSingleName(TUScope, Name, NameLoc,
                                          LookupOrdinaryName);
   AttributeCommonInfo Info(AliasName, SourceRange(AliasNameLoc),
                            AttributeCommonInfo::AS_Pragma);
-  AsmLabelAttr *Attr =
-      AsmLabelAttr::CreateImplicit(Context, AliasName->getName(), Info);
+  AsmLabelAttr *Attr = AsmLabelAttr::CreateImplicit(
+      Context, AliasName->getName(), /*LiteralLabel=*/true, Info);
 
   // If a declaration that:
   // 1) declares a function or a variable
   // 2) has external linkage
   // already exists, add a label attribute to it.
   if (PrevDecl && (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl))) {
     if (isDeclExternC(PrevDecl))
       PrevDecl->addAttr(Attr);

cfe/trunk/unittests/AST/DeclTest.cpp

 //===- unittests/AST/DeclTest.cpp --- Declaration tests -------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Unit tests for Decl nodes in the AST.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Mangle.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tooling;
+using namespace clang;
 
 TEST(Decl, CleansUpAPValues) {
   MatchFinder Finder;
   std::unique_ptr<FrontendActionFactory> Factory(
       newFrontendActionFactory(&Finder));
 
   // This is a regression test for a memory leak in APValues for structs that
   // allocate memory. This test only fails if run under valgrind with full leak
TEST(Decl, CleansUpAPValues) {
 
   // FIXME: Once this test starts breaking we can test APValue::needsCleanup
   // for ComplexInt.
   ASSERT_FALSE(runToolOnCodeWithArgs(
       Factory->create(),
       "constexpr _Complex __uint128_t c = 0xffffffffffffffff;",
       Args));
 }
+
+TEST(Decl, AsmLabelAttr) {
+  // Create two method decls: `f` and `g`.
+  StringRef Code = R"(
+    struct S {
+      void f() {}
+      void g() {}
+    };
+  )";
+  auto AST =
+      tooling::buildASTFromCodeWithArgs(Code, {"-target", "i386-apple-darwin"});
+  ASTContext &Ctx = AST->getASTContext();
+  assert(Ctx.getTargetInfo().getDataLayout().getGlobalPrefix() &&
+         "Expected target to have a global prefix");
+  DiagnosticsEngine &Diags = AST->getDiagnostics();
+  SourceManager &SM = AST->getSourceManager();
+  FileID MainFileID = SM.getMainFileID();
+
+  // Find the method decls within the AST.
+  SmallVector<Decl *, 1> Decls;
+  AST->findFileRegionDecls(MainFileID, Code.find('{'), 0, Decls);
+  ASSERT_TRUE(Decls.size() == 1);
+  CXXRecordDecl *DeclS = cast<CXXRecordDecl>(Decls[0]);
+  NamedDecl *DeclF = *DeclS->method_begin();
+  NamedDecl *DeclG = *(++DeclS->method_begin());
+
+  // Attach asm labels to the decls: one literal, and one not.
+  DeclF->addAttr(::new (Ctx) AsmLabelAttr(Ctx, SourceLocation(), "foo",
+                                          /*LiteralLabel=*/true));
+  DeclG->addAttr(::new (Ctx) AsmLabelAttr(Ctx, SourceLocation(), "goo",
+                                          /*LiteralLabel=*/false));
+
+  // Mangle the decl names.
+  std::string MangleF, MangleG;
+  MangleContext *MC = ItaniumMangleContext::create(Ctx, Diags);
+  {
+    llvm::raw_string_ostream OS_F(MangleF);
+    llvm::raw_string_ostream OS_G(MangleG);
+    MC->mangleName(DeclF, OS_F);
+    MC->mangleName(DeclG, OS_G);
+  }
+
+  ASSERT_TRUE(0 == MangleF.compare("\x01" "foo"));
+  ASSERT_TRUE(0 == MangleG.compare("goo"));
+}

lldb/trunk/source/Symbol/ClangASTContext.cpp

clang::CXXMethodDecl *ClangASTContext::AddMethodToCXXRecordType(
 
   cxx_method_decl->setAccess(access_specifier);
   cxx_method_decl->setVirtualAsWritten(is_virtual);
 
   if (is_attr_used)
     cxx_method_decl->addAttr(clang::UsedAttr::CreateImplicit(*getASTContext()));
 
   if (mangled_name != nullptr) {
-    cxx_method_decl->addAttr(
-        clang::AsmLabelAttr::CreateImplicit(*getASTContext(), mangled_name));
+    cxx_method_decl->addAttr(clang::AsmLabelAttr::CreateImplicit(
+        *getASTContext(), mangled_name, /*literal=*/false));
   }
 
   // Populate the method decl with parameter decls
 
   llvm::SmallVector<clang::ParmVarDecl *, 12> params;
 
   for (unsigned param_index = 0; param_index < num_params; ++param_index) {
     params.push_back(clang::ParmVarDecl::Create(