[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:734
+    // TODO: Make it possible to specify this in source.
+    BoolArgument<"LiteralLabel">
+  ];
----------------
`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.


================
Comment at: clang/include/clang/Basic/Attr.td:740
+[{
+bool equivalent(AsmLabelAttr *Other) const {
+  return getLabel() == Other->getLabel() && getLiteralLabel() == Other->getLiteralLabel();
----------------
`isEquivalent`, please.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2579
+  }];
+}
+
----------------
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.


================
Comment at: clang/lib/AST/Mangle.cpp:130
+      return;
+    }
+
----------------
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.


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

https://reviews.llvm.org/D67774