[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

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

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

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

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

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 ↗

(On Diff #221805)

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!

clang/include/clang/AST/ExternalASTSource.h

public:
   /// building a new declaration.
   ///
   /// This method only needs to be implemented if the AST source ever
   /// passes back decl sets as VisibleDeclaration objects.
   ///
   /// The default implementation of this method is a no-op.
   virtual Decl *GetExternalDecl(uint32_t ID);
 
+  /// Allow use of a global prefix (as defined in DataLayout) in the mangle
+  /// for asm labels. Defaults to true.
+  virtual bool UseGlobalPrefixInAsmLabelMangle() { return true; }
+
   /// Resolve a selector ID into a selector.
   ///
   /// This operation only needs to be implemented if the AST source
   /// returns non-zero for GetNumKnownSelectors().
   ///
   /// The default implementation of this method is a no-op.
   virtual Selector GetExternalSelector(uint32_t ID);
 

clang/lib/AST/Mangle.cpp

void MangleContext::mangleName(const NamedDecl *D, raw_ostream &Out) {
   if (const AsmLabelAttr *ALA = D->getAttr<AsmLabelAttr>()) {
     // If we have an asm name, then we use it as the mangling.
 
     // 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.
+    // LLVM intrinsic, or if the external AST source which provided the decl
+    // opts out of this behavior.
     char GlobalPrefix =
         getASTContext().getTargetInfo().getDataLayout().getGlobalPrefix();
-    if (GlobalPrefix && !ALA->getLabel().startswith("llvm."))
+    ExternalASTSource *Source = getASTContext().getExternalSource();
+    if (GlobalPrefix && !ALA->getLabel().startswith("llvm.") &&
+        (!Source || Source->UseGlobalPrefixInAsmLabelMangle()))
       Out << '\01'; // LLVM IR Marker for __asm("foo")
 
     Out << ALA->getLabel();
     return;
   }
 
   const ASTContext &ASTContext = getASTContext();
   CCMangling CC = getCallingConvMangling(ASTContext, D);
   bool MCXX = shouldMangleCXXName(D);

clang/test/Import/asm-label-mangle/Inputs/asm-label.cpp

+struct S1 {
+  void func1() __asm("someotherfunc") {
+  }
+};

clang/test/Import/asm-label-mangle/test.cpp

+// RUN: clang-import-test -dump-ir -import %S/Inputs/asm-label.cpp -expression %s | FileCheck %s
+
+// The asm label mangle should not include a mangle-suppression prefix.
+
+// CHECK: define void @someotherfunc
+
+void expr() {
+  S1().func1();
+}

clang/tools/clang-import-test/clang-import-test.cpp

std::unique_ptr<CodeGenerator> BuildCodeGen(CompilerInstance &CI,
   return std::unique_ptr<CodeGenerator>(CreateLLVMCodeGen(
       CI.getDiagnostics(), ModuleName, CI.getHeaderSearchOpts(),
       CI.getPreprocessorOpts(), CI.getCodeGenOpts(), LLVMCtx));
 }
 } // namespace init_convenience
 
 namespace {
 
+// A mock ExternalASTMerger, used for testing purposes.
+class MockExternalASTMerger : public clang::ExternalASTMerger {
+public:
+  MockExternalASTMerger(const ImporterTarget &Target,
+                        llvm::ArrayRef<ImporterSource> Sources)
+      : clang::ExternalASTMerger(Target, Sources) {}
+
+  // Disallow use of a global prefix in the mangle for asm labels. This is
+  // required by lldb.
+  bool UseGlobalPrefixInAsmLabelMangle() override { return false; }
+};
+
 /// A container for a CompilerInstance (possibly with an ExternalASTMerger
 /// attached to its ASTContext).
 ///
 /// Provides an accessor for the DeclContext origins associated with the
 /// ExternalASTMerger (or an empty list of origins if no ExternalASTMerger is
 /// attached).
 ///
 /// This is the main unit of parsed source code maintained by clang-import-test.

 void AddExternalSource(CIAndOrigins &CI,
                        llvm::MutableArrayRef<CIAndOrigins> Imports) {
   ExternalASTMerger::ImporterTarget Target(
       {CI.getASTContext(), CI.getFileManager()});
   llvm::SmallVector<ExternalASTMerger::ImporterSource, 3> Sources;
   for (CIAndOrigins &Import : Imports)
     Sources.push_back({Import.getASTContext(), Import.getFileManager(),
                        Import.getOriginMap()});
-  auto ES = std::make_unique<ExternalASTMerger>(Target, Sources);
+  auto ES = std::make_unique<MockExternalASTMerger>(Target, Sources);
   CI.getASTContext().setExternalSource(ES.release());
   CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
 }
 
 CIAndOrigins BuildIndirect(CIAndOrigins &CI) {
   CIAndOrigins IndirectCI{init_convenience::BuildCompilerInstance()};
   auto ST = std::make_unique<SelectorTable>();
   auto BC = std::make_unique<Builtin::Context>();

lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h

public:
   ~ClangExternalASTSourceCommon() override;
 
   ClangASTMetadata *GetMetadata(const void *object);
   void SetMetadata(const void *object, ClangASTMetadata &metadata);
   bool HasMetadata(const void *object);
 
   static ClangExternalASTSourceCommon *Lookup(clang::ExternalASTSource *source);
 
+  // Request that no "hidden" prefix is added to the mangle for asm labels.
+  // LLDB synthesizes many decls as simple asm labels: these synthesized decls
+  // cannot have a mangle different than the one specified in the label name.
+  bool UseGlobalPrefixInAsmLabelMangle() override { return false; }
+
 private:
   typedef llvm::DenseMap<const void *, ClangASTMetadata> MetadataMap;
 
   MetadataMap m_metadata;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ClangExternalASTSourceCommon_h