[clang] adds unary type transformations as compiler built-ins

[cjdb]

Summary

Adds

• __add_lvalue_reference
• __add_pointer
• __add_rvalue_reference
• __decay
• __make_signed
• __make_unsigned
• __remove_all_extents
• __remove_extent
• __remove_const
• __remove_volatile
• __remove_cv
• __remove_pointer
• __remove_reference
• __remove_cvref

These are all compiler built-in equivalents of the unary type traits
found in [[meta.trans]][1]. The compiler already has all of the
information it needs to answer these transformations, so we can skip
needing to make partial specialisations in standard library
implementations (we already do this for a lot of the query traits). This
will hopefully improve compile times, as we won't need use as much
memory in such a base part of the standard library.

[1]: http://wg21.link/meta.trans

Co-authored-by: zoecarver

[cjdb]

rebases to ToT

[rsmith]

A handful more comments, but I think we've basically converged here. I'm happy to take another look after you address these if you'd like (or you could ask someone else for a final pass), or for you to land this once you're happy.

Before landing, it'd be good to patch libc++ to use these intrinsics and run its testsuite to look for any unexpected behavior changes, if you've not already done so with this version of the patch.

clang/lib/AST/ASTContext.cpp

10814

I think we don't need this case any more.

clang/lib/Parse/ParseDeclCXX.cpp

1153–1154

This comment still needs to be addressed.

clang/lib/Parse/ParseExpr.cpp

1066–1067

Just curious: why do we only handle six of the traits here, rather than all of them?

clang/lib/Sema/SemaType.cpp

9232–9233

I've been pondering the proper behavior for make_signed on enum E : _BitInt(N) {}. I think the standard wording here is broken (see my email to the lib reflector, subject "make_signed / make_unsigned and padding / extended integer types"), but I'm not certain what the right rule is. Let's say that always producing a _BitInt type in that case is good enough for now. :)

9363

Have you considered moving the calls to getUnaryTransformType into this function? Right now they're scattered among the Builtin* helper functions, and it's hard to be sure that every code path calls it. I think it'd be a lot simpler to create the UnaryTransformType wrapper once, in a single place.

9364

Remove the static -- it's not correct to use a static local for this. There can be multiple Sema instances, and this will pick the types from whichever one was used the first time this function was called.

9366–9370

We don't have an __int128 on 32-bit targets. Maybe below you can form an ArrayRef and slice off the last element if __int128 is unsupported?

9377

Please only compute the size of BaseType once.

9380

Can this fail for an enum whose underlying type is _BitInt(N) for some unusual N?

[cjdb]

responds to @richardsmith's feedback

[cjdb]

Thanks for patiently reviewing! I'll do the libc++ stuff and a pilot run in some code. Hopefully can get it merged tomorrow :-)

clang/lib/Parse/ParseDeclCXX.cpp

1153–1154

Could've sworn I did this one, but it might've gotten mixed up in the D116280 switcheroo.

clang/lib/Parse/ParseExpr.cpp

1066–1067

Why indeed. Perhaps these were the original six I worked on and didn't come back to update this.

1749

You haven't countered this concern, so I'm going to close for now and will happily revisit if this one just slipped by unnoticed.

clang/lib/Sema/SemaType.cpp

9380

Nice catch. This wasn't supposed to happen, but I ended up changing the logic so that _BitInt is considered here instead.

[cjdb]

tl;dr fixes corner-case bug uncovered by libc++

I'd accidentally inverted the logic for when __int128 and unsigned __int128 would be available for underlying enum types. This led to the types being swapped (??) and there wasn't a test to catch it.

[aaron.ballman]

Pre-commit CI found build errors that should be addressed.

clang/include/clang/AST/TransformTypeTraits.def

1–2 ↗

(On Diff #452001)

Have to fix the formatting manually though.

clang/include/clang/Sema/DeclSpec.h

424–431

Errr, this isn't Python, so that [-1] terrifies me. It took me a good solid ten minutes to convince myself this was actually valid. Any interest in something along the lines of this form?

clang/lib/AST/TypePrinter.cpp

1156–1157

I believe you can.

clang/lib/Parse/ParseDecl.cpp

3476

clang/lib/Sema/SemaType.cpp

9267

9305–9308

9350–9352

An interesting test case for you to consider (both for enumeration underlying types as well as types in general): signed _BitInt requires at least two bits, so trying to do a make_signed on _BitInt(1) should be diagnosed.

[cjdb]

applies Aaron's feedback

[cjdb]

clang/include/clang/Sema/DeclSpec.h

424–431

I have many questions about the decisions I made in this snippet.

clang/lib/AST/TypePrinter.cpp

1156–1157

It looks like other things also have this as just empty, so maybe it's best to keep it?

[aaron.ballman]

LGTM, though I did have a nit about diagnostic wording that you can feel free to take or leave at your discretion. Thank you for this work, this was a very large patch with a whole lot of review comments, but I think what we ended up with is really great!

clang/include/clang/Sema/DeclSpec.h

424–431

It was definitely a great way to wake up this morning. "Wait, what? WHAT?!?" followed by a bunch of furious standards reading. :-D

clang/lib/AST/TypePrinter.cpp

1156–1157

That's fine by me!

clang/test/SemaCXX/type-traits.cpp

3505

This diagnostic is a bit unfortunate because it was given a non-bool integer, so I don't know that users will know how to fix the issue from the text. I leave it to your discretion where to split that diagnostic up with a %select so that _BitInt is special or not; I don't think users will hit this diagnostic all that often, but I do have a little bit of a worry about template metaprogramming accidentally getting into this state.

[cjdb]

adds a clearer diagnostic for unsigned _BitInt(1)

[cjdb]

moves TransformTypeTraits.def from clang/AST to clang/Basic

There are dowstream issues with having this definition file in the
former, because of the Bazel rules describing the relationship
between Basic and AST. Since Basic is the base and moving it there
is an NFC, I've taken the initiative to move it ahead of downstrem
usage. (Updating changelist for transperancy, but still merging
with main today.)

[thakis]

This breaks building on windows: http://45.33.8.238/win/64423/step_4.txt

Please take a look and revert for now if it takes a while to fix.

[mstorsjo]

This also broke building with GCC 9 on Ubuntu 20.04:

[5/225] ASTNodeAPI.json
FAILED: tools/clang/lib/Tooling/ASTNodeAPI.json
cd /home/martin/code/llvm-project/llvm/build/tools/clang/lib/Tooling && /home/martin/code/llvm-project/llvm/build/bin/clang-ast-dump --skip-processing=0 -I /home/martin/code/llvm-project/llvm/build/lib/clang/16.0.0/include -I /home/martin/code/llvm-project/llvm/tools/clang/include -I /home/martin/code/llvm-project/llvm/build/tools/clang/include -I /home/martin/code/llvm-project/llvm/build/include -I /home/martin/code/llvm-project/llvm/include -I /usr/include/c++/9 -I /usr/include/x86_64-linux-gnu/c++/9 -I /usr/include/c++/9/backward -I /usr/lib/gcc/x86_64-linux-gnu/9/include -I /usr/local/include -I /usr/include/x86_64-linux-gnu -I /usr/include --json-output-path /home/martin/code/llvm-project/llvm/build/tools/clang/lib/Tooling/ASTNodeAPI.json
clang-ast-dump: ../tools/clang/lib/Sema/DeclSpec.cpp:833: bool clang::DeclSpec::SetTypeSpecType(clang::DeclSpec::TST, clang::SourceLocation, const char*&, unsigned int&, const clang::PrintingPolicy&): Assertion `!isDeclRep(T) && !isTypeRep(T) && !isExprRep(T) && "rep required for these type-spec kinds!"' failed.
Aborted (core dumped)

[thakis]

Reverted this (and follow-ups) in aacf1a9742f714dd432117d82d19a007289c3dee for now.

[cjdb]

changes so this patch doesn't break things:

1. changes from begin/end to data/size
2. changes from ArrayRef to array since an ArrayRef to a prvalue is undefined (and causes segfaults when building with GCC)

[fdeazeve]

I believe this patch might have broken LLDB bots https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46269/

I've confirmed that d2d77e050b32 is good while e9ef45635b77 (this commit) breaks some tests.

To repro, a build of llvm+lldb+libcxx+libcxxapi and

# inside build dir
/bin/llvm-lit -v tools/lldb/test/API/lang/objc/modules-objc-property/TestModulesObjCProperty.py

Alternatively, running the check-lldb target should do it. But there is one other unrelated failure that might show up in that target

[fdeazeve]

Forgot to add the failure message:

/usr/include/c++/v1/experimental/propagate_const:138:11: error: no template named 'remove_reference_t'; did you mean 'remove_reference'?
  typedef remove_reference_t<decltype(*_VSTD::declval<_Tp&>())> element_type;

[fdeazeve]

It seems like non-lldb bots are also failing, if it makes it easier to reproduce:
https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c

[cjdb]

In D116203#3739856, @fdeazeve wrote:

It seems like non-lldb bots are also failing, if it makes it easier to reproduce:
https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c

Thanks for flagging this. Based on the fact it only seems to be remove_reference_t, I'm wondering if this is an issue with D131732 (which was merged immediately after D116203).

[fdeazeve]

In D116203#3740015, @cjdb wrote:

In D116203#3739856, @fdeazeve wrote:

It seems like non-lldb bots are also failing, if it makes it easier to reproduce:
https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c

Thanks for flagging this. Based on the fact it only seems to be remove_reference_t, I'm wondering if this is an issue with D131732 (which was merged immediately after D116203).

I'm not sure, to expand on my previous message, I tested these two commits which appear one right after the other in the log:

• e9ef45635b77 (14 hou..) cjdb@g.. [clang] adds unary type transformations as compiler built-ins
• d2d77e050b32 (15 hou..) Ting.W.. [PowerPC][Coroutines] Add tail-call check with call information for coroutines

The tests pass on d2d77e050b32 and fail on e9ef45635b77

[aprantl]

@cjdb Would you mind reverting the patch until we figured out a solution to unblock the CI?

[cjdb]

In D116203#3740227, @aprantl wrote:

@cjdb Would you mind reverting the patch until we figured out a solution to unblock the CI?

Sure, go ahead. I'm not even able to get to a point where I can repro the issue, as I get the following:

Command Output (stdout):
--
lldb version 16.0.0git ([e-mail]:llvm/llvm-project.git revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9)
  clang revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9
  llvm revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9
Unable to load lldb extension module.  Possible reasons for this include:
  1) LLDB was built with LLDB_ENABLE_PYTHON=0
  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to
     the version of Python that LLDB built and linked against, and PYTHONPATH
     should contain the Lib directory for the same python distro, as well as the
     location of LLDB's site-packages folder.
  3) A different version of Python than that which was built against is exported in
     the system's PATH environment variable, causing conflicts.
  4) The executable '/tmp/lldb-breaks/bin/lldb' could not be found.  Please check 
     that it exists and is executable.

--
Command Output (stderr):
--
Traceback (most recent call last):
  File "/home/cjdb/projects/llvm-bisect/lldb/test/API/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/home/cjdb/projects/llvm-bisect/lldb/packages/Python/lldbsuite/test/dotest.py", line 892, in run_suite
    import lldb
ModuleNotFoundError: No module named 'lldb'

1 and 4 seem to be fine.

[cjdb]

I think there's a forward fix available, but since I can't repro locally, I'm going to need to push it to main and observe how the tools fare.

[fdeazeve]

In D116203#3740843, @cjdb wrote:

I think there's a forward fix available, but since I can't repro locally, I'm going to need to push it to main and observe how the tools fare.

I need to be AFK for a bit now, but I can test locally on my end if you upload the patch here

[dschuff]

This broke us in emscripten as well (building with trunk clang against a recent-but-not-trunk version of libcxx). I can test the fix if you want.

[cjdb]

e137fb6fb85 should fix this issue. Thanks for your patience!