Page MenuHomePhabricator

[clang] Don't typo-fix an expression in a SFINAE context
ClosedPublic

Authored by Quuxplusone on Jan 18 2022, 2:02 PM.

Details

Summary

If this is a SFINAE context, then continuing to look up names (in particular, to treat a non-function as a function, and then do ADL) might too-eagerly complete a type that it's not safe to complete right now. We should just say "okay, that's a substitution failure" and not do any more work than absolutely required.

Fixes https://github.com/llvm/llvm-project/issues/52970

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 18 2022, 2:02 PM
Quuxplusone created this revision.

Clang-formatted.

@mizvekov or @rsmith: ping?
I'd like to get this in before Feb 1, which is the release/14.x branch date AFAIK; because it affects workarounds in libc++ and thus makes a difference whether we will be able to remove the workarounds in version 16 or version 17.

Add some more reviewers who've touched SemaCXX recently. Ping! If no objections are forthcoming, I'd like to land this on Friday morning.

urnathan accepted this revision.Jan 25 2022, 10:08 AM

go for it

This revision is now accepted and ready to land.Jan 25 2022, 10:08 AM
Quuxplusone reopened this revision.Jan 27 2022, 2:44 PM

I'm not sure if this caused
https://lab.llvm.org/buildbot/#/builders/60/builds/6350
https://lab.llvm.org/buildbot/#/builders/119/builds/7433
but I'm acting as if it did.

Anyone see how this patch might have caused a placeholder type to survive into mangling, on Windows specifically?

Assertion failed: !E->hasPlaceholderType() && "unexpected placeholder", file C:\buildbot\as-builder-2\x-aarch64\llvm-project\clang\lib\Sema\SemaType.cpp, line 9031
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: c:\\buildbot\\as-builder-2\\x-aarch64\\build\\bin\\clang++.exe -cc1 -triple aarch64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations -disable-free -clear-ast-before-backend -main-file-name deduct_F.pass.cpp -mrelocation-model static -mframe-pointer=non-leaf -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu generic -target-feature +neon -target-feature +v8a -target-abi aapcs -fallow-half-arguments-and-returns -mllvm -treat-scalable-fixed-error-as-warning -debugger-tuning=gdb -v -fcoverage-compilation-dir=C:\\buildbot\\as-builder-2\\x-aarch64\\build\\runtimes\\runtimes-bins\\libcxx\\test\\std\\utilities\\function.objects\\func.wrap\\func.wrap.func\\func.wrap.func.con -nostdinc++ -resource-dir c:\\buildbot\\as-builder-2\\x-aarch64\\build\\lib\\clang\\14.0.0 -I C:/buildbot/as-builder-2/x-aarch64/build/include\\aarch64-unknown-linux-gnu\\c++\\v1 -I C:/buildbot/as-builder-2/x-aarch64/build/include/c++/v1 -I C:/buildbot/as-builder-2/x-aarch64/build/runtimes/runtimes-bins/libcxx\\include\\c++build -D __STDC_FORMAT_MACROS -D __STDC_LIMIT_MACROS -D __STDC_CONSTANT_MACROS -I C:/buildbot/as-builder-2/x-aarch64/llvm-project/libcxx\\test/support -D _LIBCPP_DISABLE_AVAILABILITY -D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D _LIBCPP_HAS_THREAD_API_PTHREAD -D _LIBCPP_ABI_VERSION=1 -isysroot C:/buildbot/.aarch64-ubuntu -internal-isystem c:\\buildbot\\as-builder-2\\x-aarch64\\build\\lib\\clang\\14.0.0\\include -internal-isystem C:/buildbot/.aarch64-ubuntu/usr/local/include -internal-isystem C:/buildbot/.aarch64-ubuntu/usr/lib/gcc/aarch64-linux-gnu/5.4.0/../../../../aarch64-linux-gnu/include -internal-externc-isystem C:/buildbot/.aarch64-ubuntu/usr/include/aarch64-linux-gnu -internal-externc-isystem C:/buildbot/.aarch64-ubuntu/include -internal-externc-isystem C:/buildbot/.aarch64-ubuntu/usr/include -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -Wno-macro-redefined -std=c++2b -fdeprecated-macro -fdebug-compilation-dir=C:\\buildbot\\as-builder-2\\x-aarch64\\build\\runtimes\\runtimes-bins\\libcxx\\test\\std\\utilities\\function.objects\\func.wrap\\func.wrap.func\\func.wrap.func.con -ferror-limit 19 -fcoroutines-ts -fno-signed-char -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-ex1gsg43\\deduct_F-0ed146.o -x c++ C:\\buildbot\\as-builder-2\\x-aarch64\\llvm-project\\libcxx\\test\\std\\utilities\\function.objects\\func.wrap\\func.wrap.func\\func.wrap.func.con\\deduct_F.pass.cpp
1.	C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\std\utilities\function.objects\func.wrap\func.wrap.func\func.wrap.func.con\deduct_F.pass.cpp:136:37: current parser token ')'
 #0 0x00007ff65e92b105 (c:\buildbot\as-builder-2\x-aarch64\build\bin\clang++.exe+0xfdb105)
 #1 0x00007fffb07cd167 (C:\Windows\SYSTEM32\ucrtbase.DLL+0x6d167)
 #2 0x00007fffb07cdff1 (C:\Windows\SYSTEM32\ucrtbase.DLL+0x6dff1)
[...]
This revision is now accepted and ready to land.Jan 27 2022, 2:44 PM

The problem looks similar with what we met in: https://github.com/llvm/llvm-project/issues/52909#issuecomment-1021631943
Maybe both of the problem could be solved at the same time.

Anyone see how this patch might have caused a placeholder type to survive into mangling, on Windows specifically?

I didn't have a Windows and I would leave the office for some time so I couldn't see it for a time.

It seems like we need tryToRecoverWithCall to always identify a diagnostic explaining the problem, so just short-circuit-returning ExprError() with no diagnostic is not acceptable. Let's try this version instead.
(This is also now based on top of D118552.)

Move the regression test to clang/test/SemaTemplate/, alongside the one for D118552. Also, make it test the error message wording.

Poke CI (clang-format failed).

This revision was landed with ongoing or failed builds.Feb 1 2022, 12:17 PM
This revision was automatically updated to reflect the committed changes.

It's landed in Clang 14, but libc++14 still needs to support Clang 13 for another whole release (six months? a year?), so, no, we can't remove that workaround yet.
But you're correct that that's the reason I was kinda hurrying to get it fixed already before the release/14.x branch — it'll mean libc++ gets to remove the workaround 6 (12?) months earlier than otherwise.