[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Kevin Athey via Phabricator via cfe-commits]

kda added a comment.

Fix looks good.  Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D120159#3415112 , @kda wrote:

> This appears to have broken buildbot: sanitizer-x86_64-linux-fast
>
> Validating if reversion clears it up.
>
> Then will revert.

Fixed by 8f66f1371981bda1af1ca43d505e1bc5836b3e36 
 I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Kevin Athey via Phabricator via cfe-commits]

kda added a comment.

This appears to have broken buildbot: sanitizer-x86_64-linux-fast

Validating if reversion clears it up.

Then will revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd61487490022: [Clang] Implement __builtin_source_location. 
(authored by jyknight).

Changed prior to commit:
  https://reviews.llvm.org/D120159?vs=412087=418718#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGenCXX/builtin-source-location.cpp
  clang/test/SemaCXX/source_location.cpp
  clang/test/SemaCXX/source_location_err.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -6478,6 +6478,7 @@
   case Decl::Binding:
   case Decl::MSProperty:
   case Decl::MSGuid:
+  case Decl::UnnamedGlobalConstant:
   case Decl::TemplateParamObject:
   case Decl::IndirectField:
   case Decl::ObjCIvar:
Index: clang/test/SemaCXX/source_location_err.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/source_location_err.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=1 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=2 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=3 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=4 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=5 %s
+
+#if TEST == 1
+auto test1a = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location;
+}
+}
+
+auto test1b = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location {
+struct __impl;
+  };
+}
+}
+auto test1c = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+#elif TEST == 2
+auto test2a = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+struct source_location {
+  struct __impl { int x; };
+};
+}
+}
+auto test2b = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+#elif TEST == 3
+namespace std {
+struct source_location {
+  struct __impl {
+int other_member;
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+  };
+};
+}
+auto test3 = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+#elif TEST == 4
+namespace std {
+struct source_location {
+  struct parent {};
+  struct __impl : public parent {
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+  };
+};
+}
+auto test4 = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+
+#elif TEST == 5
+namespace std {
+struct source_location {
+  struct __impl {
+signed char _M_line; // odd integral type to choose, but ok!
+const char *_M_file_name;
+signed char _M_column;
+const 

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

no comments, looks fine to me.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping; any more comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 412087.
jyknight marked an inline comment as done.
jyknight added a comment.

Fix and test __impl lookup within the definition of std::source_location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGenCXX/builtin-source-location.cpp
  clang/test/SemaCXX/source_location.cpp
  clang/test/SemaCXX/source_location_err.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -6466,6 +6466,7 @@
   case Decl::Binding:
   case Decl::MSProperty:
   case Decl::MSGuid:
+  case Decl::UnnamedGlobalConstant:
   case Decl::TemplateParamObject:
   case Decl::IndirectField:
   case Decl::ObjCIvar:
Index: clang/test/SemaCXX/source_location_err.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/source_location_err.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=1 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=2 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=3 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=4 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=5 %s
+
+#if TEST == 1
+auto test1a = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location;
+}
+}
+
+auto test1b = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location {
+struct __impl;
+  };
+}
+}
+auto test1c = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+#elif TEST == 2
+auto test2a = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+struct source_location {
+  struct __impl { int x; };
+};
+}
+}
+auto test2b = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+#elif TEST == 3
+namespace std {
+struct source_location {
+  struct __impl {
+int other_member;
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+  };
+};
+}
+auto test3 = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+#elif TEST == 4
+namespace std {
+struct source_location {
+  struct parent {};
+  struct __impl : public parent {
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+  };
+};
+}
+auto test4 = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+
+#elif TEST == 5
+namespace std {
+struct source_location {
+  struct __impl {
+signed char _M_line; // odd integral type to choose, but ok!
+const char *_M_file_name;
+signed char _M_column;
+const char *_M_function_name;
+static int other_member; // static members are OK
+  };
+  using BuiltinT = 

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

In D120159#3349271 , @jyknight wrote:

> In D120159#3349069 , @aaron.ballman 
> wrote:
>
>> Ah, okay, that's a good point. I was thinking this would show up in the AST 
>> in places like:
>>
>>   template 
>>   auto func() -> Ty;
>>   
>>   int main() {
>> func();
>>   }
>>
>> when we go to print the `Ty` type from the template. Does that not happen?
>
>
>
>   |-FunctionTemplateDecl 0x10ef3288  col:6 func
>   | |-TemplateTypeParmDecl 0x10ef3068  col:20 referenced 
> typename depth 0 index 0 Ty
>   | |-FunctionDecl 0x10ef31e8  col:6 func 'auto () -> Ty'
>   | `-FunctionDecl 0x10ef36d8  col:6 used func 'auto () -> 
> const std::source_location::__impl *'
>   |   `-TemplateArgument type 'const std::source_location::__impl *'
>   | `-PointerType 0x10ef34c0 'const std::source_location::__impl *'
>   |   `-QualType 0x10ef2d81 'const std::source_location::__impl' const
>   | `-RecordType 0x10ef2d80 'std::source_location::__impl'
>   |   `-CXXRecord 0x10ef2ce8 '__impl'
>   `-FunctionDecl 0x10ef33f0  line:14:5 main 'int ()'
> `-CompoundStmt 0x10ef38c0 
>   `-CallExpr 0x10ef38a0  'const 
> std::source_location::__impl *':'const std::source_location::__impl *'
> `-ImplicitCastExpr 0x10ef3888  'auto (*)() -> const 
> std::source_location::__impl *' 
>   `-DeclRefExpr 0x10ef37d0  'auto () -> const 
> std::source_location::__impl *' lvalue Function 0x10ef36d8 'func' 'auto () -> 
> const std::source_location::__impl *' (FunctionTemplate 0x10ef3288 'func')
>
> The type is simply `const std::source_location::__impl*`. Only the evaluated 
> value refers to an UnnamedGlobalConstantDecl.
>
> If it was possible to use in a non-type template argument, I think you could 
> get it to show up in the AST for the instantiation, but that's not supported. 
> E.g.
>
>   template 
>   class X {};
>   
>   X<> x;
>
> Results in:
>
>   test.cc:12:20: error: non-type template argument does not refer to any 
> declaration
>   template 
>  ^~~
>   test.cc:15:3: note: while checking a default template argument used here
>   X<> x;
>   ~~^

Excellent, thank you for double-checking! This does raise an interesting 
question about the AST import functionality -- if `UnnamedGlobalConstantDecl` 
can't show up in the AST, do we need to handle AST reading and writing for it, 
or can we assert that it should never show up through PCH/Modules?

In general, this is looking pretty good to me. Adding Erich in case he spots 
something I missed.




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:
> > 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.
> Oh, I see. That doesn't seem important here, since non-system code should not 
> be defining a std::source_location::current() function anyhow.
> 
> (But if we keep needing to add exceptions here, maybe eventually we just 
> decide to just turn this error into a default-on warningerror, and not 
> diagnose it in system headers.)
okay, that's fair.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight marked 8 inline comments as done.
jyknight added a comment.

In D120159#3349069 , @aaron.ballman 
wrote:

> Ah, okay, that's a good point. I was thinking this would show up in the AST 
> in places like:
>
>   template 
>   auto func() -> Ty;
>   
>   int main() {
> func();
>   }
>
> when we go to print the `Ty` type from the template. Does that not happen?



  |-FunctionTemplateDecl 0x10ef3288  col:6 func
  | |-TemplateTypeParmDecl 0x10ef3068  col:20 referenced 
typename depth 0 index 0 Ty
  | |-FunctionDecl 0x10ef31e8  col:6 func 'auto () -> Ty'
  | `-FunctionDecl 0x10ef36d8  col:6 used func 'auto () -> const 
std::source_location::__impl *'
  |   `-TemplateArgument type 'const std::source_location::__impl *'
  | `-PointerType 0x10ef34c0 'const std::source_location::__impl *'
  |   `-QualType 0x10ef2d81 'const std::source_location::__impl' const
  | `-RecordType 0x10ef2d80 'std::source_location::__impl'
  |   `-CXXRecord 0x10ef2ce8 '__impl'
  `-FunctionDecl 0x10ef33f0  line:14:5 main 'int ()'
`-CompoundStmt 0x10ef38c0 
  `-CallExpr 0x10ef38a0  'const 
std::source_location::__impl *':'const std::source_location::__impl *'
`-ImplicitCastExpr 0x10ef3888  'auto (*)() -> const 
std::source_location::__impl *' 
  `-DeclRefExpr 0x10ef37d0  'auto () -> const 
std::source_location::__impl *' lvalue Function 0x10ef36d8 'func' 'auto () -> 
const std::source_location::__impl *' (FunctionTemplate 0x10ef3288 'func')

The type is simply `const std::source_location::__impl*`. Only the evaluated 
value refers to an UnnamedGlobalConstantDecl.

If it was possible to use in a non-type template argument, I think you could 
get it to show up in the AST for the instantiation, but that's not supported. 
E.g.

  template 
  class X {};
  
  X<> x;

Results in:

  test.cc:12:20: error: non-type template argument does not refer to any 
declaration
  template 
 ^~~
  test.cc:15:3: note: while checking a default template argument used here
  X<> x;
  ~~^




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:
> 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.
Oh, I see. That doesn't seem important here, since non-system code should not 
be defining a std::source_location::current() function anyhow.

(But if we keep needing to add exceptions here, maybe eventually we just decide 
to just turn this error into a default-on warningerror, and not diagnose it in 
system headers.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D120159#3348927 , @jyknight wrote:

> In 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 
  auto func() -> Ty;
  
  int main() {
func();
  }

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``, `` _M_line``, and
+`` _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 `` 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 ) 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 

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In 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``, `` _M_line``, and
+`` _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 `` 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 ) 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(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 
> 

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 411811.
jyknight marked 15 inline comments as done.
jyknight added a comment.

Update per review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGenCXX/builtin-source-location.cpp
  clang/test/SemaCXX/source_location.cpp
  clang/test/SemaCXX/source_location_err.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -6466,6 +6466,7 @@
   case Decl::Binding:
   case Decl::MSProperty:
   case Decl::MSGuid:
+  case Decl::UnnamedGlobalConstant:
   case Decl::TemplateParamObject:
   case Decl::IndirectField:
   case Decl::ObjCIvar:
Index: clang/test/SemaCXX/source_location_err.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/source_location_err.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=1 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=2 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=3 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=4 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=5 %s
+
+#if TEST == 1
+auto test1a = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location;
+}
+}
+
+auto test1b = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location {
+struct __impl;
+  };
+}
+}
+auto test1c = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+#elif TEST == 2
+auto test2a = __builtin_source_location(); // expected-error {{'std::source_location::__impl' was not found}}
+
+namespace std {
+inline namespace NS {
+struct source_location {
+  struct __impl { int x; };
+};
+}
+}
+auto test2b = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+#elif TEST == 3
+namespace std {
+struct source_location {
+  struct __impl {
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+int other_member;
+  };
+};
+}
+auto test3 = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+#elif TEST == 4
+namespace std {
+struct source_location {
+  struct parent {};
+  struct __impl : public parent {
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+  };
+};
+}
+auto test4 = __builtin_source_location(); // expected-error {{'std::source_location::__impl' must be standard-layout and have only two 'const char *' fields '_M_file_name' and '_M_function_name', and two integral fields '_M_line' and '_M_column'}}
+
+
+#elif TEST == 5
+namespace std {
+struct source_location {
+  struct __impl {
+signed char _M_line; // odd integral type to choose, but ok!
+const char *_M_file_name;
+signed char _M_column;
+const char *_M_function_name;
+static int other_member; // static members are OK
+  };
+};
+}
+
+// Verify that the address cannot be used as a non-type template argument.

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 411692.
jyknight added a comment.

Minor tweaks to comments and docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGenCXX/builtin-source-location.cpp
  clang/test/SemaCXX/source_location.cpp
  clang/test/SemaCXX/source_location_err.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -6466,6 +6466,7 @@
   case Decl::Binding:
   case Decl::MSProperty:
   case Decl::MSGuid:
+  case Decl::UnnamedGlobalConstant:
   case Decl::TemplateParamObject:
   case Decl::IndirectField:
   case Decl::ObjCIvar:
Index: clang/test/SemaCXX/source_location_err.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/source_location_err.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=1 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=2 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=3 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=4 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=5 %s
+
+#if TEST == 1
+auto test1a = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location;
+}
+}
+
+auto test1b = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location {
+struct __impl;
+  };
+}
+}
+auto test1c = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+#elif TEST == 2
+auto test2a = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+namespace std {
+inline namespace NS {
+struct source_location {
+  struct __impl { int x; };
+};
+}
+}
+auto test2b = __builtin_source_location(); // expected-error {{std::source_location::__impl must be standard-layout and have only two 'const char *' fields _M_file_name and _M_function_name, and two integral fields _M_line and _M_column.}}
+
+#elif TEST == 3
+namespace std {
+struct source_location {
+  struct __impl {
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+int other_member;
+  };
+};
+}
+auto test3 = __builtin_source_location(); // expected-error {{std::source_location::__impl must be standard-layout and have only two 'const char *' fields _M_file_name and _M_function_name, and two integral fields _M_line and _M_column.}}
+
+#elif TEST == 4
+namespace std {
+struct source_location {
+  struct parent {};
+  struct __impl : public parent {
+char _M_line;
+const char *_M_file_name;
+char _M_column;
+const char *_M_function_name;
+  };
+};
+}
+auto test4 = __builtin_source_location(); // expected-error {{std::source_location::__impl must be standard-layout and have only two 'const char *' fields _M_file_name and _M_function_name, and two integral fields _M_line and _M_column.}}
+
+
+#elif TEST == 5
+namespace std {
+struct source_location {
+  struct __impl {
+signed char _M_line; // odd integral type to choose, but ok!
+const char *_M_file_name;
+signed char _M_column;
+const char *_M_function_name;
+static int other_member; // static members are OK
+  };
+};
+}
+
+// Verify that the address cannot be used as a non-type template argument.
+template 
+auto fn1() {return X;} // expected-note {{candidate template ignored: substitution failure: 

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ah, one more thing I missed -- you should add a release note for this 
functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120159

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this! I've got some initial comments on it.

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




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``, `` _M_line``, and
+`` _M_column``. The fields will be populated in the same
+manner as the above four builtins, except that ``_M_function_name`` is 
populated

Doesn't the type for `` 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?



Comment at: clang/include/clang/AST/DeclCXX.h:4191
 
+/// An artificial decl, representing am global anonymous constant value which 
is
+/// uniquified by value within a compilation.





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

I presume this is what was meant, right?



Comment at: clang/include/clang/AST/DeclCXX.h:4221
+  /// Print this in a human-readable format.
+  void printName(llvm::raw_ostream ) const override;
+

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



Comment at: clang/include/clang/AST/Expr.h:4709
 
-  bool isStringType() const {
+  bool isIntType() const {
 switch (getIdentKind()) {

Should this also be marked `LLVM_READONLY` as in the original interface?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11506-11509
+def err_std_source_location_impl_not_found : Error<
+  "std::source_location::__impl was not found; it must be defined before 
__builtin_source_location is called.">;
+def err_std_source_location_impl_malformed : Error<
+  "std::source_location::__impl must be standard-layout and have only two 
'const char *' fields _M_file_name and _M_function_name, and two integral 
fields _M_line and _M_column.">;





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

Do we need to bump this value now that we have a new kind of AST node?



Comment at: clang/lib/AST/Expr.cpp:2270
+// __builtin_FUNCTION() above returns!
+const Decl *CurDecl = dyn_cast_or_null(Context);
+Value.getStructField(F->getFieldIndex()) = MakeStringLiteral(

Under what conditions can `Context` be null? (should this be a `dyn_cast` 
instead?)



Comment at: clang/lib/AST/ExprClassification.cpp:468-471
 islvalue = isa(D) || isa(D) ||
-   isa(D) ||
-   isa(D) ||
-   isa(D) ||
+   isa(D) || isa(D) ||
+   isa(D) || isa(D) ||
isa(D) ||

Plus fix the formatting that I'm sure I broke.



Comment at: clang/lib/AST/ExprClassification.cpp:473-474
(Ctx.getLangOpts().CPlusPlus &&
 (isa(D) || isa(D) ||
  isa(D)));
 





Comment at: clang/lib/AST/ExprConstant.cpp:1982-1983
+// ... the address of an unnamed global constant
+return isa(D) || isa(D) ||
+   isa(D);
   }





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

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)



Comment at: clang/lib/Sema/SemaExpr.cpp:3257
 
-  // Enum constants are always r-values and never references.
+// Enum constants are always r-values and never references.
   // Unresolved using declarations are dependent.

Spurious indentation change?



Comment at: clang/lib/Sema/SemaExpr.cpp:3405
+  case Decl::UnnamedGlobalConstant:
+valueKind = VK_LValue;
+break;

[PATCH] D120159: [Clang] Implement __builtin_source_location.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: aaron.ballman, rsmith.
Herald added subscribers: dexonsmith, arphaman, martong.
Herald added a reviewer: shafik.
jyknight requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This builtin returns the address of a global instance of the
`std::source_location::__impl` type, which must be defined (with an
appropriate shape) before calling the builtin.

It will be used to implement std::source_location in libc++ in a
future change. The builtin is compatible with GCC's implementation,
and libstdc++'s usage. An intentional divergence is that GCC declares
the builtin's return type to be `const void*` (for
ease-of-implementation reasons), while Clang uses the actual type,
`const std::source_location::__impl*`.

In order to support this new functionality, I've also added a new
'UnnamedGlobalConstantDecl'. This artificial Decl is modeled after
MSGuidDecl, and is used to represent a generic concept of an lvalue
constant with global scope, deduplicated by its value. It's possible
that MSGuidDecl itself, or some of the other similar sorts of things
in Clang might be able to be refactored onto this more-generic
concept, but there's enough special-case weirdness in MSGuidDecl that
I gave up attempting to share code there, at least for now.

Finally, for compatibility with libstdc++'s  header,
I've added a second exception to the "cannot cast from void* to T* in
constant evaluation" rule. This seems a bit distasteful, but feels
like the best available option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120159

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DeclNodes.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGenCXX/builtin-source-location.cpp
  clang/test/SemaCXX/source_location.cpp
  clang/test/SemaCXX/source_location_err.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -6466,6 +6466,7 @@
   case Decl::Binding:
   case Decl::MSProperty:
   case Decl::MSGuid:
+  case Decl::UnnamedGlobalConstant:
   case Decl::TemplateParamObject:
   case Decl::IndirectField:
   case Decl::ObjCIvar:
Index: clang/test/SemaCXX/source_location_err.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/source_location_err.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=1 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=2 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=3 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=4 %s
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify -DTEST=5 %s
+
+#if TEST == 1
+auto test1a = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location;
+}
+}
+
+auto test1b = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+namespace std {
+inline namespace NS {
+  struct source_location {
+struct __impl;
+  };
+}
+}
+auto test1c = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+#elif TEST == 2
+auto test2a = __builtin_source_location(); // expected-error {{std::source_location::__impl was not found}}
+
+namespace std {
+inline namespace NS {
+struct source_location {
+  struct __impl { int x; };
+};
+}
+}
+auto test2b = __builtin_source_location(); // expected-error {{std::source_location::__impl must be standard-layout and have only two 'const char *' fields _M_file_name and _M_function_name, and two integral fields _M_line and _M_column.}}
+
+#elif TEST == 3