Update cling in ci to version 1.1 using roots llvm 16

[mcbarton]

This PR will update cling in the ci to version 1.1 using root llvm 16

[aaronj0]

@mcbarton this is already ongoing in #333, rebasing that PR should do the job. The reason interop should build now is because the offsets to obtain the JIT was changed in #330 . I just never rebased the CI pr after that

[mcbarton]

@mcbarton this is already ongoing in #333, rebasing that PR should do the job. The reason interop should build now is because the offsets to obtain the JIT was changed in #330 . I just never rebased the CI pr after that

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

[aaronj0]

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

It's not ready yet... The reason you see the old jobs is because it's many commits behind, but that is relatively easy to resolve which I am doing right now. I don't think opening a fresh PR is the solution.

We shouldn't have the cling version set to 'refs/heads/master'. This will just cause the cache is fill up quickly

As for 'refs/head/master', cling with llvm18 doesn't have a release tag yet. The purpose of this is to have a real-time test of upstream cling

[mcbarton]

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

It's not ready yet... The reason you see the old jobs is because it's many commits behind, but that is relatively easy to resolve which I am doing right now. I don't think opening a fresh PR is the solution.

We shouldn't have the cling version set to 'refs/heads/master'. This will just cause the cache is fill up quickly

As for 'refs/head/master', cling with llvm18 doesn't have a release tag yet. The purpose of this is to have a real-time test of upstream cling

@aaronj0 We can't have a real time test of the cling based on llvm 18 in the ci as it would be constantly be rebuilding the cache, overwhelming the little space we have very quickly. If you drop this from the PR and make it specifically about cling 1.1 and llvm 16, and revert the names back to be consistent then I'll drop this PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.55%. Comparing base (f794dee) to head (1055a9c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage   74.55%   74.55%           
=======================================
  Files           8        8           
  Lines        3211     3211           
=======================================
  Hits         2394     2394           
  Misses        817      817           

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[aaronj0]

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

It's not ready yet... The reason you see the old jobs is because it's many commits behind, but that is relatively easy to resolve which I am doing right now. I don't think opening a fresh PR is the solution.

We shouldn't have the cling version set to 'refs/heads/master'. This will just cause the cache is fill up quickly

As for 'refs/head/master', cling with llvm18 doesn't have a release tag yet. The purpose of this is to have a real-time test of upstream cling

@aaronj0 We can't have a real time test of the cling based on llvm 18 in the ci as it would be constantly be rebuilding the cache, overwhelming the little space we have very quickly. If you drop this from the PR and make it specifically about cling 1.1 and llvm 16, and revert the names back to be consistent then I'll drop this PR.

We will cut a tag for it, or use a commit. Either way I don't see the point of duplicating the previous PR

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[Vipul-Cariappa]

LGTM! Maybe we should also update the README with updated versions for build instructions.

---

I spent some time debugging the failure. It is caused when there is an error in the code we are executing (source).

Stack trace goes through

https://github.com/root-project/cling/blob/b5e06a2a7b0832f034d7e405f42f0770364b607f/lib/Interpreter/IncrementalParser.cpp#L914

Destructor of llvm::Expected https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L551-L552

https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L714-L715

The logic looks valid in my eyes.

The error msg is

Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error

With the following patch for cling the CppInterOp tests pass

diff --git a/lib/Interpreter/IncrementalParser.cpp b/lib/Interpreter/IncrementalParser.cpp
index 89cd78d..2aea40d 100644
--- a/lib/Interpreter/IncrementalParser.cpp
+++ b/lib/Interpreter/IncrementalParser.cpp
@@ -911,8 +911,11 @@ namespace cling {
     PP.EnterSourceFile(FID, /*DirLookup*/nullptr, NewLoc);
     m_Consumer->getTransaction()->setBufferFID(FID);
 
-    if (!ParseOrWrapTopLevelDecl())
+    llvm::Expected<bool> res = ParseOrWrapTopLevelDecl();
+    if (!res) {
+      llvm::consumeError(std::move(res.takeError()));
       return kFailed;
+    }
 
     if (PP.getLangOpts().DelayedTemplateParsing) {
       // Microsoft-specific:

@vgvassilev may have to take a look once.

[vgvassilev]

LGTM! Maybe we should also update the README with updated versions for build instructions.

I spent some time debugging the failure. It is caused when there is an error in the code we are executing (source).

Stack trace goes through

https://github.com/root-project/cling/blob/b5e06a2a7b0832f034d7e405f42f0770364b607f/lib/Interpreter/IncrementalParser.cpp#L914

Destructor of llvm::Expected https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L551-L552

https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L714-L715

The logic looks valid in my eyes.

The error msg is

Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error

With the following patch for cling the CppInterOp tests pass

diff --git a/lib/Interpreter/IncrementalParser.cpp b/lib/Interpreter/IncrementalParser.cpp
index 89cd78d..2aea40d 100644
--- a/lib/Interpreter/IncrementalParser.cpp
+++ b/lib/Interpreter/IncrementalParser.cpp
@@ -911,8 +911,11 @@ namespace cling {
     PP.EnterSourceFile(FID, /*DirLookup*/nullptr, NewLoc);
     m_Consumer->getTransaction()->setBufferFID(FID);
 
-    if (!ParseOrWrapTopLevelDecl())
+    llvm::Expected<bool> res = ParseOrWrapTopLevelDecl();
+    if (!res) {
+      llvm::consumeError(std::move(res.takeError()));
       return kFailed;
+    }
 
     if (PP.getLangOpts().DelayedTemplateParsing) {
       // Microsoft-specific:

@vgvassilev may have to take a look once.

Makes sense. Please open a pr.

[Vipul-Cariappa]

Makes sense. Please open a pr.

PR at cling?

[vgvassilev]

Makes sense. Please open a pr.

PR at cling?

Yes.

[Vipul-Cariappa]

Hi @vgvassilev I have opened the PR at root-project/cling#538.

I was thinking... after merging the PR at Cling, we will have to create a new release, v1.2, before we can use that fix here?
Is it better to just include the fix as a patch file in patches directory in this repo and apply it?

FYI, with this patch, CppInterOp tests pass, but cppyy tests fail. I will start looking into it.

Any thoughts @mcbarton

[vgvassilev]

Hi @vgvassilev I have opened the PR at root-project/cling#538.

I was thinking... after merging the PR at Cling, we will have to create a new release, v1.2, before we can use that fix here? Is it better to just include the fix as a patch file in patches directory in this repo and apply it?

FYI, with this patch, CppInterOp tests pass, but cppyy tests fail. I will start looking into it.

Any thoughts @mcbarton

Yeah, I could release cling...

[mcbarton]

@Vipul-Cariappa I have added your patch to the workflow to see it working. I have disabled building cppyy, until you determine what is the issue stopping cppyy from passing with cling 1.1. If this patch works then I will update the documentation.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[mcbarton]

@Vipul-Cariappa any idea how to fix the failing osx jobs? The errror is the same as what you fixed for Ubuntu.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[Vipul-Cariappa]

Debugging OSX failures at #359.

Looks like Cpp::CreateInterpreter fails.
Backtrace:

I will need to rebuild cling and llvm in debug. It is going to be time consuming.

[Vipul-Cariappa]

Hi @mcbarton.

If I run git diff on cling, I do not see my patch applied.
I guess my patch is not applied?

Do we need to rebuild the cache?

Or update the logic of retrieving the cache?
I see some complicated key in the retrieval step

path: |
          llvm-project
          ${{ matrix.cling=='On' && 'cling' || '' }}
key: ${{ env.CLING_HASH }}-${{ runner.os }}-${{ matrix.os }}-${{ matrix.compiler }}-clang-${{ matrix.clang-runtime }}.x-patch-${{ hashFiles(format('patches/llvm/clang{0}-*.patch', matrix.clang-runtime)) || 'none' }}

I am coming to this conclusion by looking at the CI run of dbc5d38 (Update cling16-1-Error-Handling.patch to add blank line) commit. I see that ubuntu-cling still fails for the same reason.
With that patch Ubuntu-Cling should pass, at least it passes in my local machine.

[mcbarton]

@Vipul-Cariappa The Ubuntu job passes with your patch. See https://github.com/compiler-research/CppInterOp/actions/runs/12100202323/job/33748369555

[Vipul-Cariappa]

Weird. Looks like the OSX failure may not be caused in this round of CI run.
I am speculating here., as OSX-arm passed. Waiting for OSX-X86 and Ubuntu to finish.

---

@Vipul-Cariappa The Ubuntu job passes with your patch. See https://github.com/compiler-research/CppInterOp/actions/runs/12100202323/job/33748369555

Which commit is this?

Any idea why Ubuntu-Cling fails in my PR https://github.com/compiler-research/CppInterOp/actions/runs/12112309439/job/33766010006?pr=359? It fails with the same error, my patch is supposed to fix.