[Lldb-commits] [PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

[John McCall via Phabricator via lldb-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:725
   let Spellings = [Keyword<"asm">, Keyword<"__asm__">];
-  let Args = [StringArgument<"Label">];
+  let Args = [StringArgument<"Label">, BoolArgument<"LiteralLabel">];
   let SemaHandler = 0;
----------------
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.


================
Comment at: clang/lib/AST/Mangle.cpp:137
+        (!Source || Source->UseGlobalPrefixInAsmLabelMangle()))
       Out << '\01'; // LLVM IR Marker for __asm("foo")
 
----------------
rjmccall wrote:
> 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 `ExternalASTSource`s should be putting the real (prefixed) symbol name in the `AsmLabelAttr`, or they should be making `AsmLabelAttr`s 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.
Please check for whether the label is literal first before pulling out the global prefix.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2770
+      assert(OldA->getLiteralLabel() == NewA->getLiteralLabel() &&
+             "Redeclaration of asm label changes label kind");
       if (OldA->getLabel() != NewA->getLabel()) {
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67774/new/

https://reviews.llvm.org/D67774