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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to