In D126266#3629145, @vsapsai wrote:

Hmm, I'm concerned with the pieces added purely for testing. Specifically, FileEntriesToReread and TestHelper friend functions. Need to think how else we can test it.

Let's move forward and we can remove the XFAIL once https://reviews.llvm.org/D128968 lands.

I am adding some more reviewers as I think this patch is ready to go.

Thanks for the clarification! LGTM.

Looks good to me but can we add a test? Do we have builders with that target?

Historically, the patch was developed before the extended support of virtual files in Clang. Given that this is very hard to test and it looks like this is not needed on our end anymore. I'd propose to pause this for now.

This landed slightly modified here: https://github.com/llvm/llvm-project/commit/4d54e543abd5d0a8b0a321f8c292252f4895693a

LGTM!

According to godbolt this example does not crash anymore.

In D125944#3544341, @v.g.vassilev wrote:

@Purva-Chaudhari, can you rebase this patch, seems that it is not buildable.

@rsmith, we need to do something similar in cling to handle pending template instantiations, I guess the question is if we can avoid instantiating the templates we don't need and still survive.

I think this landed in a different form.

LGTM, modulo the comment.

After landing https://reviews.llvm.org/D126682 we might be able to write a test in clang-repl such as:

I think this patch is not needed in the main branch as https://github.com/llvm/llvm-project/commit/f91b6f8159b101688eef87c75179209699c0d405 removes the need of calling invalidateCache altogether.

Can we write a test for this?

Thanks for working on this. Here are some more comments.

@t184256, do we still need this? Should I commit it in case you don't have commit access?

I'd propose to move forward here and rely on eventual post-commit review.

Lgtm!

Let's move forward with this and rely on eventual post-commit review.

I think we are in a good shape to broaden the list of reviewers.

In D126682#3546865, @junaire wrote:

this depends on https://reviews.llvm.org/D126684

This looks reasonable to me...

This looks good to me, modulo the inline comment.

In D126781#3554845, @rjmccall wrote:

Okay, I understand. So, first off, I wouldn't really call that a "weak" symbol rather than, say, a lazily-emitted symbol; "weak" already has plenty of different senses, and we should try to avoid coining more. Also, your patch description makes it sound like there's a general bug you're fixing rather than specifically an issue with re-using a single CodeGenerator to generate a succession of modules; please remember that people reading your commit messages don't necessarily know you or contextualize your patches by knowing that you work on Cling.

Thanks for the prompt reply.

In D123674#3546722, @uabelho wrote:

Hi,

I noticed that the testcase Interpreter/execute.cpp starts failing with this patch when run/compiled with asan:

Failed Tests (1):
  Clang :: Interpreter/execute.cpp

Seen in the buildbot run
https://lab.llvm.org/buildbot/#/builders/5/builds/24221

The run on the commit before passed:
https://lab.llvm.org/buildbot/#/builders/5/builds/24220

This patch fixes our test case which we intended to fix with:

From 17e94a6a2adc3eb8d5fcc532eb1f3e57cc59b0fd Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev@gmail.com>
Date: Mon, 23 May 2022 21:36:43 +0000
Subject: [PATCH] Fix type printing to not include the suffix when not needed.

LGTM!

This is not an issue anymore for us.

@Purva-Chaudhari, can you rebase this patch, seems that it is not buildable.

@Purva-Chaudhari, this looks reasonable to me. I am adding more reviewers to discuss the way we refer other tests from clang-repl.

LGTM! Apologies for the delay.

Landed in https://github.com/llvm/llvm-project/commit/788e0f7f3e96a9d61c2412e452c4589e2693b79a

Just to add that the invalidateCache is important for cling and clang-repl where we do something like:

Thanks for the clarification! LGTM!

Order fwd decls alphabetically.

ping.

In D120206#3335121, @tstellar wrote:

I have a patch for this already: D117745. I will commit it now.

That change landed in 4fb0805c6525b13e50067b9ddfe8677a0b7b2d7c

Lgtm!

Address comments and fix the windows bot failure.

In D112349#3111927, @erichkeane wrote:

In D112349#3111873, @ibookstein wrote:

And how is Cling expecting CFE to deal with partial knowledge situations at the implementation level? To deal with exactly the non-local cases that the current violations address?
If there's no prescriptive way forward to dealing with these cases (so they're tech debt without a remediation plan), then as far as I'm concerned this case sits exactly under the same tech-debt umbrella of the existing violations and the way forward is using the same violating solution for this case too.
We definitely shouldn't block the IR verification indefinitely on this impasse. Once an acceptable solution is found, this will also be part of that refactor.

My understanding is that the REPL setup is that the 'IR' just needs to be in a state where it doesn't require reverts/rewrites later, so that we can do partial-back-end-code-generation as we go along. That said, I'm not positive as to the implications. I'm just repeating the discussion the CFE code-owner made at the time.

IMO, the 'acceptable' solution is to have a way to forward-declare an ifunc in IR so that we can fill in the resolver later on. From your description earlier, it seems that this would work as we could emit it as an 'unknown symbol' (as if it was an undefined function declaration), and would be completely implementable in the CFE.

So it would change your plan from earlier to:

When processing cpu_specific, emit the ifunc "x.ifunc", with no resolver;
When processing cpu_dispatch:

Get/Create the ifunc, then pull up the resolver.
If the resolver is null (as it should be), create one and update the 'ifunc'.
Generate said resolver.

In D107049#3099816, @uabelho wrote:

In D107049#3096807, @v.g.vassilev wrote:

Can you share your cmake config line, the target triple and architecture?

CC='/mycompiler/compiler-clang/bin/clang -march=corei7' CXX='/mycompiler/compiler-clang/bin/clang++ -march=corei7' LDFLAGS='-L/mycompiler/compiler-clang/lib64 -Wl,--disable-new-dtags,-R/mycompiler/compiler-clang/lib/x86_64-unknown-linux-gnu/c++:/mycompiler/compiler-clang/lib64:/proj/bbi_twh/wh_bbi/x86_64-Linux3/z3/4.8.8-1/lib64 -ldl' cmake /repo/llvm --debug-trycompile -G Ninja -DCMAKE_MAKE_PROGRAM=ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS='-Wno-error=unused-command-line-argument' -DCMAKE_CXX_FLAGS='-stdlib=libc++' -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX=/compiler-clang -DLLVM_APPEND_VC_REV=OFF -DLLVM_CCACHE_DIR=/repo/.ccache -DLLVM_CCACHE_BUILD=ON -DLLVM_CCACHE_PROGRAM=/app/vbuild/RHEL7-x86_64/ccache/3.2.2/bin/ccache -DLLVM_CCACHE_NOHASHDIR=ON -DLLVM_CCACHE_BASEDIR=/repo -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_WERROR=ON -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;lld' -DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_Z3_INSTALL_DIR=/proj/bbi_twh/wh_bbi/x86_64-Linux3/z3/4.8.8-1 -DLLVM_LIT_ARGS='-sv --threads 14' -DLLVM_ENABLE_PLUGINS=OFF -DLLVM_USE_SANITIZER='Address' -DLLVM_ENABLE_LIBPFM=OFF -DLLVM_ENABLE_LIBXML2=OFF -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_STATIC_LINK_CXX_STDLIB=ON -DCLANG_ENABLE_ARCMT=OFF -DCLANG_TOOLING_BUILD_AST_INTROSPECTION=OFF -DCLANG_ROUND_TRIP_CC1_ARGS=OFF

In D107049#3096727, @uabelho wrote:

Hi,

We're seeing a problem with this patch in our downstream (not public) buildbots. With an asan-built compiler we see the following:

10:08:55 FAIL: Clang-Unit :: Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests/InterpreterTest.CatchException (25832 of 79960)
10:08:55 ******************** TEST 'Clang-Unit :: Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests/InterpreterTest.CatchException' FAILED ********************
10:08:55 Script:
10:08:55 --
10:08:55 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/tools/clang/unittests/Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests --gtest_filter=InterpreterTest.CatchException
10:08:55 --
10:08:55 Note: Google Test filter = InterpreterTest.CatchException
10:08:55 [==========] Running 1 test from 1 test suite.
10:08:55 [----------] Global test environment set-up.
10:08:55 [----------] 1 test from InterpreterTest
10:08:55 [ RUN      ] InterpreterTest.CatchException
10:08:55 libunwind: __unw_add_dynamic_fde: bad fde: FDE is really a CIE
10:08:55 libc++abi: terminating with uncaught exception of type custom_exception
10:08:55  #0 0x000000000052072b backtrace /repo/uabelho/flacc_6.144/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:4205:13
10:08:55  #1 0x0000000001873774 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/../lib/Support/Unix/Signals.inc:565:13
10:08:55  #2 0x000000000186c328 llvm::sys::RunSignalHandlers() /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/../lib/Support/Signals.cpp:0:5
10:08:55  #3 0x00000000018745a8 SignalHandler(int) /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/../lib/Support/Unix/Signals.inc:0:3
10:08:55  #4 0x00007f61246da630 __restore_rt sigaction.c:0:0
10:08:55  #5 0x00007f6122025387 raise (/lib64/libc.so.6+0x36387)
10:08:55  #6 0x00007f6122026a78 abort (/lib64/libc.so.6+0x37a78)
10:08:55  #7 0x000000000cbdedd6 (/repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/tools/clang/unittests/Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests+0xcbdedd6)
10:08:55  #8 0x000000000cbe4407 (/repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/tools/clang/unittests/Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests+0xcbe4407)
10:08:55  #9 0x000000000cbdecb3 std::__terminate(void (*)()) crtstuff.c:0:0
10:08:55 #10 0x000000000cbdca26 (/repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/llvm-main-sanitize-asan/llvm/build-all-bbisdk-asan/tools/clang/unittests/Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests+0xcbdca26)
10:08:55 #11 0x000000000cbdc9c0 __cxxabiv1::exception_cleanup_func(_Unwind_Reason_Code, _Unwind_Exception*) cxa_exception.cpp:0:0
10:08:55 #12 0x00007f611ea00171 
10:08:55 
10:08:55 ********************
10:08:55 Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
10:25:10 
10:25:10 1 warning(s) in tests
10:25:10 ********************
10:25:10 Failed Tests (1):
10:25:10   Clang-Unit :: Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests/InterpreterTest.CatchException
10:25:10

Does an asan-build run clean for everyone else?

Address comments

In D107049#3052557, @leonardchan wrote:

Hi. I think our clang builders are failing from this after the reland (https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8833962834812688769/overview):

Script:
--
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/unittests/Interpreter/ExceptionTests/./ClangReplInterpreterExceptionTests --gtest_filter=InterpreterTest.CatchException
--
Note: Google Test filter = InterpreterTest.CatchException
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from InterpreterTest
[ RUN      ] InterpreterTest.CatchException
JIT session error: Symbols not found: [ _Unwind_Resume, _ZSt9terminatev, _ZTVN10__cxxabiv117__class_type_infoE, __cxa_allocate_exception, __cxa_begin_catch, __cxa_end_catch, __cxa_free_exception, __cxa_throw, __gxx_personality_v0 ]
Failure value returned from cantFail wrapped call
Failed to materialize symbols: { (main, { __clang_call_terminate, _ZTI16custom_exception, _ZN16custom_exceptionC2EPKc, _ZTS16custom_exception, throw_exception }) }
UNREACHABLE executed at llvm/include/llvm/Support/Error.h:778!

Would you be able to send out a fix or revert?

In D111198#3048647, @v.g.vassilev wrote:

In D111198#3044293, @lhames wrote:

Vassil -- This is a generalization of the error fix that we discussed. Could you double check that it fixes the linker error that you saw?

After thinking about it for a while I'm on the fence about this approach -- on the one hand it's a reasonable cleanup, on the other hand I don't think we generally support linking -no-rtti LLVM libraries from -rtti code, and support for doing that (even partial support) might be an explicit non-goal for the project.

Another approach to fixing the example might just be to split it into two files. Pros of that approach: it deals with no-rtti/rtti crossover code The Right Way, Con: it complicates the example for everyone else.

@lhames, thanks a lot for the patch -- this makes D107049 compile. I do not have a strong opinion on how/should we bridge rtti/no-rtti. I feel this will come up more often due to clang-repl...

In D111198#3044293, @lhames wrote:

Vassil -- This is a generalization of the error fix that we discussed. Could you double check that it fixes the linker error that you saw?

After thinking about it for a while I'm on the fence about this approach -- on the one hand it's a reasonable cleanup, on the other hand I don't think we generally support linking -no-rtti LLVM libraries from -rtti code, and support for doing that (even partial support) might be an explicit non-goal for the project.

Another approach to fixing the example might just be to split it into two files. Pros of that approach: it deals with no-rtti/rtti crossover code The Right Way, Con: it complicates the example for everyone else.

Thanks for the patch!

Can you map state_not_recoverable to not_supported?