[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D120159#3348927 <https://reviews.llvm.org/D120159#3348927>, @jyknight wrote:

> 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.

Ah, okay, that's a good point. I was thinking this would show up in the AST in places like:

  template <typename Ty>
  auto func() -> Ty;
  
  int main() {
    func<decltype(__builtin_source_location())>();
  }

when we go to print the `Ty` type from the template. Does that not happen?



================
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
----------------
jyknight wrote:
> 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.
Okay, I'm fine adding that support later; I definitely agree about the compatibility concerns.


================
Comment at: clang/include/clang/AST/DeclCXX.h:4221
+  /// Print this in a human-readable format.
+  void printName(llvm::raw_ostream &OS) const override;
+
----------------
jyknight wrote:
> 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.
How the heck did I miss that? Sorry for the noise.


================
Comment at: clang/include/clang/AST/Expr.h:4709
 
-  bool isStringType() const {
+  bool isIntType() const {
     switch (getIdentKind()) {
----------------
jyknight wrote:
> 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.
SGTM


================
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).
----------------
jyknight wrote:
> 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...)
I'm fine adding the hack, but the restrictions I was thinking of was forcing it to only work in a system header.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3405
+  case Decl::UnnamedGlobalConstant:
+    valueKind = VK_LValue;
+    break;
----------------
jyknight wrote:
> 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.
I was thinking the same thing, but I had thought string literals were a prvalue the same as integer literals, oops. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159