Page MenuHomePhabricator

[Clang] Implement __builtin_source_location.
ClosedPublic

Authored by jyknight on Feb 18 2022, 1:11 PM.

Details

Summary

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

Diff Detail

Event Timeline

jyknight created this revision.Feb 18 2022, 1:11 PM
jyknight requested review of this revision.Feb 18 2022, 1:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2022, 1:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

clang/docs/LanguageExtensions.rst
3437–3438

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?

clang/include/clang/AST/DeclCXX.h
4209
4210

I presume this is what was meant, right?

4239

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.

clang/include/clang/AST/Expr.h
4710

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

clang/include/clang/Basic/DiagnosticSemaKinds.td
11560–11563
clang/include/clang/Serialization/ASTBitCodes.h
44

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

clang/lib/AST/Expr.cpp
2231

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

clang/lib/AST/ExprClassification.cpp
467–468

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

467–472
clang/lib/AST/ExprConstant.cpp
1982–1983
8846–8848

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)

clang/lib/Sema/SemaExpr.cpp
3295

Spurious indentation change?

3443

Perhaps stupid question, but do we want this to be an lvalue as opposed to a prvalue?

14131–14134

Might as well combine these as well.

16325
16372

I would set the count to 5 instead of 0 because otherwise someone could do something stupid like:

namespace std {
struct source_location {
  struct __impl {
    int Muahahahahhahaha;
    // ... The other four fields.
  };
};
}
clang/lib/Sema/SemaTemplate.cpp
7012–7015
clang/test/SemaCXX/source_location.cpp
383–388

Why are these commented out?

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

jyknight updated this revision to Diff 411692.Feb 27 2022, 10:48 AM

Minor tweaks to comments and docs.

jyknight updated this revision to Diff 411811.Feb 28 2022, 7:34 AM
jyknight marked 15 inline comments as done.

Update per review comments.

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.

clang/docs/LanguageExtensions.rst
3437–3438

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.

clang/include/clang/AST/DeclCXX.h
4210

Yep.

4239

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.

clang/include/clang/AST/Expr.h
4710

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.

clang/include/clang/Serialization/ASTBitCodes.h
44

Probably so. It looks like we don't generally bump this number, but that's probably accidental.

clang/lib/AST/Expr.cpp
2231

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.

clang/lib/AST/ExprConstant.cpp
8846–8848

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

clang/lib/Sema/SemaExpr.cpp
3443

I think so? I was thinking of it as acting effectively like a string literal.

clang/test/SemaCXX/source_location.cpp
383–388

Oops -- intended to update those to the proper strings. Fixed.

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?

clang/docs/LanguageExtensions.rst
3437–3438

Okay, I'm fine adding that support later; I definitely agree about the compatibility concerns.

clang/include/clang/AST/DeclCXX.h
4239

How the heck did I miss that? Sorry for the noise.

clang/include/clang/AST/Expr.h
4710

SGTM

clang/lib/AST/ExprConstant.cpp
8846–8848

I'm fine adding the hack, but the restrictions I was thinking of was forcing it to only work in a system header.

clang/lib/Sema/SemaExpr.cpp
3443

I was thinking the same thing, but I had thought string literals were a prvalue the same as integer literals, oops. :-)

jyknight marked 8 inline comments as done.Feb 28 2022, 10:29 AM

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?

|-FunctionTemplateDecl 0x10ef3288 <line:11:1, line:12:16> col:6 func
| |-TemplateTypeParmDecl 0x10ef3068 <line:11:11, col:20> col:20 referenced typename depth 0 index 0 Ty
| |-FunctionDecl 0x10ef31e8 <line:12:1, col:16> col:6 func 'auto () -> Ty'
| `-FunctionDecl 0x10ef36d8 <col:1, col:16> 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:1, line:16:1> line:14:5 main 'int ()'
  `-CompoundStmt 0x10ef38c0 <col:12, line:16:1>
    `-CallExpr 0x10ef38a0 <line:15:3, col:47> 'const std::source_location::__impl *':'const std::source_location::__impl *'
      `-ImplicitCastExpr 0x10ef3888 <col:3, col:45> 'auto (*)() -> const std::source_location::__impl *' <FunctionToPointerDecay>
        `-DeclRefExpr 0x10ef37d0 <col:3, col:45> '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 <auto T = __builtin_source_location()>
    class X {};

X<> x;

Results in:

test.cc:12:20: error: non-type template argument does not refer to any declaration
template <auto T = __builtin_source_location()>
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cc:15:3: note: while checking a default template argument used here
X<> x;
~~^
clang/lib/AST/ExprConstant.cpp
8846–8848

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

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?

|-FunctionTemplateDecl 0x10ef3288 <line:11:1, line:12:16> col:6 func
| |-TemplateTypeParmDecl 0x10ef3068 <line:11:11, col:20> col:20 referenced typename depth 0 index 0 Ty
| |-FunctionDecl 0x10ef31e8 <line:12:1, col:16> col:6 func 'auto () -> Ty'
| `-FunctionDecl 0x10ef36d8 <col:1, col:16> 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:1, line:16:1> line:14:5 main 'int ()'
  `-CompoundStmt 0x10ef38c0 <col:12, line:16:1>
    `-CallExpr 0x10ef38a0 <line:15:3, col:47> 'const std::source_location::__impl *':'const std::source_location::__impl *'
      `-ImplicitCastExpr 0x10ef3888 <col:3, col:45> 'auto (*)() -> const std::source_location::__impl *' <FunctionToPointerDecay>
        `-DeclRefExpr 0x10ef37d0 <col:3, col:45> '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 <auto T = __builtin_source_location()>
    class X {};

X<> x;

Results in:

test.cc:12:20: error: non-type template argument does not refer to any declaration
template <auto T = __builtin_source_location()>
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
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.

clang/lib/AST/ExprConstant.cpp
8846–8848

okay, that's fair.

jyknight updated this revision to Diff 412087.Mar 1 2022, 7:00 AM
jyknight marked an inline comment as done.

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

Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 10:20 AM
martong removed a subscriber: martong.Mar 10 2022, 2:38 AM

Ping; any more comments?

This revision is now accepted and ready to land.Mar 21 2022, 8:47 AM
erichkeane accepted this revision.Mar 21 2022, 8:48 AM

no comments, looks fine to me. Thanks!

This revision was landed with ongoing or failed builds.Mar 28 2022, 3:29 PM
This revision was automatically updated to reflect the committed changes.
kda added a subscriber: kda.Mar 29 2022, 2:15 PM

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

Validating if reversion clears it up.

Then will revert.

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.

kda added a comment.Mar 29 2022, 3:20 PM

Fix looks good. Thank you.