Nice!

In D143524#4150286, @aaron.ballman wrote:

In D143524#4148024, @v.g.vassilev wrote:

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

We need to ensure the diagnostic is not so noisy that people disable it, though. That means a low false positive rate and a straight-forward way to silence the diagnostic on a case-by-case basis.

In D143524#4148271, @philnik wrote:

It looks like this warning is incompatible with -Wctad-maybe-unsupported. It warns that the deduction guide is unused, but the deduction guide is required suppress -Wctad-maybe-unsupported. https://godbolt.org/z/G8bMjYsbn

In D143524#4148039, @philnik wrote:

In D143524#4148024, @v.g.vassilev wrote:

In D143524#4148006, @philnik wrote:

The emitted warnings from the libc++ CI look like a false-positive to me. While the functions are never called, they are used in an unevaluated context. I would expect -Wunused warnings to only be emitted when I can just remove the code without problems, which doesn't seem to be the case here. It would probably just get turned off again by lots of people if there are too many false-positives, which I don't think is the goal here.

From what I see is that most of the templates are marked with static which means internal linkage. Entities with internal linkage in header files are essentially different across translation units which is an ODR violation. I believe the discussion here gives more insights of how this works: https://reviews.llvm.org/D29877

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

I missed the static at the beginning. That explains the warning, thanks! I agree this should be fixed. I'll look into making a patch to enable -Wunused-template and fix any problems. Hopefully there aren't too many.

In D143524#4148006, @philnik wrote:

The emitted warnings from the libc++ CI look like a false-positive to me. While the functions are never called, they are used in an unevaluated context. I would expect -Wunused warnings to only be emitted when I can just remove the code without problems, which doesn't seem to be the case here. It would probably just get turned off again by lots of people if there are too many false-positives, which I don't think is the goal here.

In D142176#4075822, @lhames wrote:

Here's a potential testcase, for anyone who wants to try this out. You'll need to build LLVM, compiler-rt, and libunwind:

C++ source for testcase:

int main(int argc, char *argv[]) {
  try {
    throw 42;
  } catch (int X) {
    return X;
  }
  return 0;
}

This can be compiled and run under the llvm-jitlink test tool on Darwin with:

% clang++ -c -o eh.o eh.cpp
% DYLD_LIBRARY_PATH=/path/to/llvm-build/lib ORC_RT_DEBUG=1 llvm-jitlink -orc-runtime=/path/to/llvm-build/lib/clang/16/lib/darwin/liborc_rt_osx.a eh.o
__unw_add/remove_find_dynamic_unwind_sections available. Using callback-based frame info lookup.
...

You can verify that the exception was caught and handled correctly by printing the return code:

% echo $?
42

This seems like a good candidate for a post commit review.

We should probably add some tests here. Alternatively we can add the tests from https://reviews.llvm.org/D143148 but that'd make this patch bulkier and probably harder to review.

ping

clang-format

Added a testcase by Sunho!

In D119708#4060336, @jansvoboda11 wrote:

In D119708#4059254, @Hahnfeld wrote:

Hello, sorry for the late heads-up, but this functionality is used by ROOT: https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282 Any chance of bringing this back?

Hi. Your downstream callback never fills in RecoveryPath and always returns false, meaning it will never reach the unsafe block of code I had issue with: HeaderInfo.AddSearchPath(DL, isAngled).
Technically, we could bring back a safe/trimmed-down version of this callback that doesn't take the out-parameter and returns void. But I don't think it's the right call, since that would essentially re-introduce dead code to upstream.
This is probably best kept downstream, unless there are other downstream users.

In D131858#4058807, @erichkeane wrote:

In D131858#4054348, @v.g.vassilev wrote:

In D131858#4052031, @erichkeane wrote:

In D131858#4052026, @v.g.vassilev wrote:

Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman and @erichkeane for the efforts saving @mizvekov work over the summer. I believe he has sporadic access to internet and soon he will be back to normal. Great example of team work here!!

Note we don't yet have the evidence that this is savable yet, we should know in the next day or so. At minimum, at least 1 of his patches needs to be reverted due to the memory regression.

I see that https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6584/ picked up your revert and seems successful. Then the next build is green as well and then starts failing for a reason that’s unrelated to this patch.

Yep! So only 1 of the patches (D136566 needs reverting/further attention).

In D131858#4052031, @erichkeane wrote:

In D131858#4052026, @v.g.vassilev wrote:

Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman and @erichkeane for the efforts saving @mizvekov work over the summer. I believe he has sporadic access to internet and soon he will be back to normal. Great example of team work here!!

Note we don't yet have the evidence that this is savable yet, we should know in the next day or so. At minimum, at least 1 of his patches needs to be reverted due to the memory regression.

Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman and @erichkeane for the efforts saving @mizvekov work over the summer. I believe he has sporadic access to internet and soon he will be back to normal. Great example of team work here!!

In D131858#4047785, @erichkeane wrote:

@mizvekov : I'm looking at the revert of this since no work seems to have been done to fix @steven_wu 's problem. Unfortunately, it seems that reverting this requires reverting https://reviews.llvm.org/D134604 plus all of the "Instantiate" patches, which undoes basically all of your work.

I'm starting it now, but its unfortunate that we are going to lose this much of your work.

Latest test run: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48911/execution/node/74/log/
I modified the expected string slightly to make it work: https://reviews.llvm.org/rG811ad246ac7b
Should be all good now. Thanks for looking into this!

In D127284#3969593, @Michael137 wrote:

In D127284#3969458, @v.g.vassilev wrote:

In D127284#3969446, @v.g.vassilev wrote:

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

@Michael137, I think I understand what happens. This patch introduces a new language option which is not benign from modules perspective. However, lldb sets up modules the old way and then switches to the incremental processing mode. We have two ways to fix this:

• Pass -Xclang -fincremental-extensions in ClangModulesDeclVendor.cpp and delete instance->getPreprocessor().enableIncrementalProcessing();; or
• Apply https://reviews.llvm.org/D139258 which already does that.

Can you check if https://reviews.llvm.org/D139258 passes the lldb testsuite. It does pass my reproduction by hand (I still cannot run full of the lldb-api tests)?

Thanks for taking a look. I tried with the suggested patch on the Objective-C API tests and there's only 1 test failure remaining there:

lang/objc/modules-compile-error/TestModulesCompileError.py

Expecting sub string: "module.h:4:1: error: unknown type name 'syntax_error_for_lldb_to_find'" (was not found)

The test expects an error string that looks like unknown type name 'syntax_error_for_lldb_to_find' but gets use of 'undeclared identifier 'syntax_error_for_lldb_to_find'

Haven't looked much further than that. Does that sound familiar to you? I see a similar test fix as part of this patch. Maybe it's just a matter of fixing up the expected string.

I think we should revert for now until https://reviews.llvm.org/D139258 is ready to go. Just to unblock the buildbot

In D127284#3969446, @v.g.vassilev wrote:

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

In D127284#3969398, @Michael137 wrote:

In D127284#3969308, @v.g.vassilev wrote:

In D127284#3969249, @v.g.vassilev wrote:

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Hi @Michael137, I cannot reproduce these failures on unix. Is there some special way I need to compile. I will try reproducing them on OSX. Did you have time to debug where the tests fail?

I cannot reproduce this on osx either.

Hmmm how are you running the tests? It reproduces pretty consistently with the patch and doesn't without it. I just run ninja check-lldb-api (assuming you built with LLDB_INCLUDE_TESTS=ON)

Let me try with a clean build

In D127284#3969249, @v.g.vassilev wrote:

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Hi @Michael137, I cannot reproduce these failures on unix. Is there some special way I need to compile. I will try reproducing them on OSX. Did you have time to debug where the tests fail?

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Fixed the diff. I accidentally have added the commits from https://reviews.llvm.org/D139016

I screwed up with the last diff. Let me fix it in a bit...

Address comments, fix a fixme.

I have modified it a little before landing it to capture better the intent and our off-list conversation.

Sorry, selected the wrong option. This looks good to me. I can commit that change for you as I know you do not have commit access.

This looks good to me. I can commit that change for you as I know you do not have commit access.

rebase.

I think I have addressed all comments from @aaron.ballman and @rsmith.

Teach CodeGen to squash contiguous top-level stmt decl blocks into a single llvm::Function. Rebase.

Address comments, support better C in clang-repl.

Rebase + fix the failure on windows

Usually @rsmith has something up in the sleeve in these situations ;)

In D137787#3924727, @Hahnfeld wrote:

Yes, I fully agree that having a test is desirable, I just didn't manage so far. Maybe it takes somebody with deep AST knowledge to explain under what circumstances DtorDecl->getParent() is *not* the canonical Decl. Maybe this could help crafting a test case, even outside of modules maybe

Remove several fixmes. Now we can deal with templates, deduction guides and operators.

Address comments. Call isCurrentClassName before calling isConstructorDeclarator.

This LGTM but I'd like to add reviewers who changed this area recently.

In D127284#3902487, @aaron.ballman wrote:

It looks like precommit CI caught a relevant issue that should be addressed.

Address more comments.

Rebase.

Address comments.

This looks reasonable to me but let's have another pair of eyes.

Address review comments

Adding Aaron as a reviewer who hopefully could speed up the review.

Let's move forward with this patch and rely on a post-commit review if necessary.

Thanks for working on this! I left some comments. I did not look very deep in the patch but it seems quite consistent to the rest of the code in Sema.

ping...

Maybe we can add a little more context. In the case of static compilation the file system is pretty much read-only and taking a snapshot of it usually is sufficient. In the interactive C++ case the compilation is longer and people can create and include files, etc. In that case we often do not want to open files or cache failures unless is absolutely necessary. We might be able to write a test for this in clang/unittests/Lex/HeaderSearchTest.cpp. However, this patch improves consistency in the interfaces by forwarding the already provided flags to the callees and if we cannot write a reasonable test we should be able to accept it without.

@tapaswenipathak, ping...

Rely on the CXXScopeSpec to detect more accurately if we are parsing a constructor declarator.