[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/lib/AST/Mangle.cpp:130
+      return;
+    }
+
----------------
vsk wrote:
> rjmccall wrote:
> > 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.
> 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.
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.


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

https://reviews.llvm.org/D67774