[PATCH] D120159: [Clang] Implement __builtin_source_location.

Mon Feb 28 07:35:17 PST 2022

jyknight added a comment.

In D120159#3341224 <https://reviews.llvm.org/D120159#3341224>, @aaron.ballman wrote:

> Btw, I think there may be some functionality missing for AST dumping, so I'd like to see some additional tests for that.

I'm not sure what test would actually show a problem (or lack thereof) in the ast dumping -- AFAICT, UnnamedGlobalConstantDecl can't actually show up in an AST dump at the moment. The AST generated for a `__builtin_source_location` call doesn't contain one, since it's only when evaluating the value that you receive an LValue pointing to an UnnamedGlobalConstantDecl. But https://github.com/llvm/llvm-project/blob/fdfe26ddbeb1a5eb6106a6a27d6f8240deca543f/clang/lib/AST/TextNodeDumper.cpp#L535 doesn't print those usefully, anyhow.

================
Comment at: clang/docs/LanguageExtensions.rst:3343-3344
+defined, and must contain exactly four fields: ``const char *_M_file_name``,
+``const char *_M_function_name``, ``<any-integral-type> _M_line``, and
+``<any-integral-type> _M_column``. The fields will be populated in the same
+manner as the above four builtins, except that ``_M_function_name`` is populated
----------------
aaron.ballman wrote:
> Doesn't the type for `<any-integral-type>` matter though, due to layout differences based on the size of the integer type picked?
> 
> Also, a downside of requiring this type to be defined before the builtin can be used is that this builtin would otherwise be suitable in C, but there's no way to name that particular type. Is there anything we can do to keep this builtin generic enough to be used in C as well?
The type of the integer (as well as ordering of the fields) doesn't matter, because the code populates fields in the actual struct layout as defined, it doesn't assume a particular layout.

As for making it usable from C code, possibly we could have it attempt to look-up a `__source_location_impl` type in C mode (or maybe always, as a fallback if std::source_location::__impl doesn't exist). 

But, that's something we could add later if needed -- and before going that route, I think we should discuss with GCC/libstdc++ to try to remain compatible.

================
Comment at: clang/include/clang/AST/DeclCXX.h:4192
+/// An artificial decl, representing am global anonymous constant value which is
+/// uniquified by value within a compilation.
+///
----------------
aaron.ballman wrote:
> I presume this is what was meant, right?
Yep.

================
Comment at: clang/include/clang/AST/DeclCXX.h:4221
+  /// Print this in a human-readable format.
+  void printName(llvm::raw_ostream &OS) const override;
+
----------------
aaron.ballman wrote:
> `printName()` in a class named `UnnamedGlobalConstantDecl` (which isn't a `NamedDecl`) seems a bit confusing as to what this actually prints.
> 
> Also, what is this overriding? `NamedDecl` has a virtual `printName()` function, but `ValueDecl` does not. I have the sneaking suspicion that this can be removed.
`class ValueDecl : public NamedDecl` -- so, UnnamedGlobalConstantDecl _is_ a "NamedDecl". I suspect it's not strictly needed, but it does affect what's printed in debug output for this type.

================
Comment at: clang/include/clang/AST/Expr.h:4709

-  bool isStringType() const {
+  bool isIntType() const {
     switch (getIdentKind()) {
----------------
aaron.ballman wrote:
> Should this also be marked `LLVM_READONLY` as in the original interface?
I don't think it'd actually make a difference, and we don't typically go around annotating everything with that attribute. It was weird that isStringType wasn't annotated but isIntType was, in the first place.

================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44
 /// AST files at this time.
 const unsigned VERSION_MAJOR = 15;

----------------
aaron.ballman wrote:
> Do we need to bump this value now that we have a new kind of AST node?
Probably so. It looks like we don't generally bump this number, but that's probably accidental.

================
Comment at: clang/lib/AST/Expr.cpp:2270
+        // __builtin_FUNCTION() above returns!
+        const Decl *CurDecl = dyn_cast_or_null<Decl>(Context);
+        Value.getStructField(F->getFieldIndex()) = MakeStringLiteral(
----------------
aaron.ballman wrote:
> Under what conditions can `Context` be null? (should this be a `dyn_cast` instead?)
That came from the similar code above for __builtin_FUNCTION. I don't think it can actually be null (in either place). Changed in both.

================
Comment at: clang/lib/AST/ExprConstant.cpp:8830-8832
+      // (GCC apparently doesn't diagnose casts from void* to T* in constant
+      // evaluation, regardless of the types of the object or pointer; a
+      // subsequent access through the wrong type is diagnosed, however).
----------------
aaron.ballman wrote:
> Do we want to be even more restrictive here and tie it to libstdc++ (e.g., like we did in https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L12352)
I don't see anything in the code you linked that makes it libstdc++-tied? But, in any case, I think restricting to std::source_location::current is sufficiently narrow that it doesn't much matter.

We also have the option of not doing this hack, since it's going to be fixed in libstdc++. However, given that a similar hack is already present for std::allocator, extending it to source_location doesn't seem that terrible. 

(And IMO, it would make sense for the C++ standard to actually permit this in constant evaluation, anyways...)

================
Comment at: clang/lib/Sema/SemaExpr.cpp:3405
+  case Decl::UnnamedGlobalConstant:
+    valueKind = VK_LValue;
+    break;
----------------
aaron.ballman wrote:
> Perhaps stupid question, but do we want this to be an lvalue as opposed to a prvalue?
I think so? I was thinking of it as acting effectively like a string literal.

================
Comment at: clang/test/SemaCXX/source_location.cpp:383-385
+  //  static_assert(is_equal(Default.info.function(), "TestCtor<>::TestCtor()"));
+  static_assert(is_equal(Default.ctor_info.function(), ""));
+  //  static_assert(is_equal(Template.info.function(), "TestCtor<>::TestCtor(int, U) [U = std::source_location]"));
----------------
aaron.ballman wrote:
> Why are these commented out?
Oops -- intended to update those to the proper strings. Fixed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159