Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cling] Provide C value printer, adopted from Ruben De Smet! #17422

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ferdymercury
Copy link
Contributor

@ferdymercury ferdymercury commented Jan 14, 2025

This Pull request:

Changes or fixes:

Rebased version of #9272

Fixes root-project/cling#364
Fixes root-project/cling#419
and helps for jupyter-xeus/xeus-cling#404

@ferdymercury ferdymercury marked this pull request as draft January 14, 2025 09:57
@ferdymercury ferdymercury marked this pull request as ready for review January 14, 2025 10:28
@ferdymercury
Copy link
Contributor Author

It seems that this PR does not increase the number of errors in clingtest-alma9, taking as reference the errors in #16917

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cling changes seem fine. Cannot comment on the ci.

@ferdymercury
Copy link
Contributor Author

@bellenot Do you happen to know why check-cling doesn't work in-tree for Windows? thks!

Copy link

Test Results

    18 files      18 suites   4d 6h 36m 41s ⏱️
 2 695 tests  2 684 ✅ 0 💤 11 ❌
46 766 runs  46 751 ✅ 0 💤 15 ❌

For more details on these failures, see this check.

Results for commit ec49115.

@bellenot
Copy link
Member

It looks like clingtest-check-cling fails on several platforms...

@ferdymercury
Copy link
Contributor Author

It looks like clingtest-check-cling fails on several platforms...

Sorry, what I meant is that, on Windows, it looks as if it doesn't even compile. It gives this error:

  MSBuild version 17.9.5+33de0b227 for .NET Framework
  MSBUILD : error MSB1009: Project file does not exist.
  Switch: check-cling.vcxproj
  CMake Error at C:/ROOT-CI/src/cmake/modules/RootTestDriver.cmake:232 (message):
    error code: 1

(On other platforms, it fails because of a specific subtest does not pass.)

@bellenot
Copy link
Member

It looks like clingtest-check-cling fails on several platforms...

Sorry, what I meant is that, on Windows, it looks as if it doesn't even compile. It gives this error:

  MSBuild version 17.9.5+33de0b227 for .NET Framework
  MSBUILD : error MSB1009: Project file does not exist.
  Switch: check-cling.vcxproj
  CMake Error at C:/ROOT-CI/src/cmake/modules/RootTestDriver.cmake:232 (message):
    error code: 1

(On other platforms, it fails because of a specific subtest does not pass.)

OK, I'll check the branch locally (but can't test the CI related changes...)

@ferdymercury
Copy link
Contributor Author

OK, I'll check the branch locally (but can't test the CI related changes...)

Thanks. Maybe you don't need to check out any specific branch, just recompiling master with flag -Dclingtest=ON might show it.

@bellenot
Copy link
Member

Well, it cannot work on Windows. The PATH is wrong, the command is wrong, and then, when fixing those errors, even running the test fails with:

 C:\Python311\python.exe: can't open file 'C:\\root-dev\\build\\x64\\cvalueprinter\\interpreter\\llvm-project\\llvm\\bin\\llvm-lit.py': [Errno 2] No such file or directory

Still working on it...

@bellenot
Copy link
Member

Was all this designed to run on Windows at all?:

   Running the Cling regression tests
1:   llvm-lit.py: C:\root-dev\github\cvalueprinter\interpreter\llvm-project\llvm\utils\lit\lit\llvm\config.py:57: note: using lit tools: C:\Program Files\Git\usr\bin
1:   llvm-lit.py: C:\root-dev\github\cvalueprinter\interpreter\llvm-project\llvm\utils\lit\lit\TestingConfig.py:152: fatal: unable to parse config file 'C:/root-dev/github/cvalueprinter/interpreter/cling/test/lit.cfg', traceback: Traceback (most recent call last):
1:     File "C:\root-dev\github\cvalueprinter\interpreter\llvm-project\llvm\utils\lit\lit\TestingConfig.py", line 140, in load_from_path
1:       exec(compile(data, path, "exec"), cfg_globals, None)
1:     File "C:/root-dev/github/cvalueprinter/interpreter/cling/test/lit.cfg", line 20, in <module>
1:       if IsWindows and not execute_external:
1:                            ^^^^^^^^^^^^^^^^
1:   NameError: name 'execute_external' is not defined

And I would need to patch LLVM, so not sure I'm going to embark into this...

@bellenot
Copy link
Member

I would suggest to completely disable it on Windows, like:

diff --git a/interpreter/CMakeLists.txt b/interpreter/CMakeLists.txt
index 419ff4028a..3d1319b414 100644
--- a/interpreter/CMakeLists.txt
+++ b/interpreter/CMakeLists.txt
@@ -114,8 +114,10 @@ if(clingtest)
   if (NOT builtin_llvm)
     set(CLINGTEST_EXECUTABLE CLING=${CMAKE_CURRENT_BINARY_DIR}/llvm-project/llvm/${CMAKE_CFG_INTDIR}/bin/cling)
   endif()
-  ROOT_ADD_TEST(clingtest-check-cling COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target check-cling
-                                       ENVIRONMENT ${CLINGTEST_EXECUTABLE})
+  if(NOT MSVC)
+    ROOT_ADD_TEST(clingtest-check-cling COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target check-cling
+                                         ENVIRONMENT ${CLINGTEST_EXECUTABLE})
+  endif()
 else()
   #---Build LLVM/Clang with symbol visibility=hidden--------------------------------------------------
   set(CMAKE_CXX_VISIBILITY_PRESET hidden)

And remove the windows related changes in your PR, unless someone disagrees (@vgvassilev ?)

@ferdymercury
Copy link
Contributor Author

ferdymercury commented Jan 20, 2025

Ok. But how can I then check if the test I changed in this PR gives a correct result in Windows or breaks sth? Or could you do that locally for this PR? (I have no Windows machine to check)

@bellenot
Copy link
Member

Ok. But how can I then check if the test I changed in this PR gives a correct result in Windows or breaks sth? Or could you do that locally for this PR? (I have no Windows machine to check)

I can do it locally (hopefully tomorrow)

@bellenot
Copy link
Member

@ferdymercury BTW, interpreter\cling\test\Driver\C.c is not going to work on Windows the way it is:

// RUN: cat %s | %cling -x c -Xclang -verify 2>&1 | FileCheck %s
// RUN: cat %s | %cling -x c -fsyntax-only -Xclang -verify 2>&1

There is no cat on Windows, and I don't even know if the flags are supported. Anyway, I'll build cling (since it is not built with ROOT) and run it on Windows (I'll find a way) and I'll let you know

@bellenot
Copy link
Member

bellenot commented Jan 21, 2025

So here is what I get on Windows with your changes:

C:\root-dev\build\x64\llvm-project>type C.c | cling -x c -Xclang -verify

***************** CLING *****************
* Type C code and press enter to run it *
*            Type .q to exit            *
*****************************************
CHECK 123 0000007E45B8FA40
CHECK 4
error: 'expected-error' diagnostics seen but not expected:
  File input_line_14 Line 3: ValueExtractionSynthesizer could not find: 'cling namespace'.
  File input_line_15 Line 2: ValueExtractionSynthesizer could not find: 'cling namespace'.
error: 'expected-warning' diagnostics seen but not expected:
  File input_line_1 Line 5: omitting the parameter name in a function definition is a C23 extension
  File input_line_1 Line 5: omitting the parameter name in a function definition is a C23 extension

C:\root-dev\build\x64\llvm-project>type C.c | cling -x c -fsyntax-only -Xclang -verify

***************** CLING *****************
* Type C code and press enter to run it *
*            Type .q to exit            *
*****************************************
error: 'expected-error' diagnostics seen but not expected:
  File input_line_14 Line 3: ValueExtractionSynthesizer could not find: 'cling namespace'.
  File input_line_15 Line 2: ValueExtractionSynthesizer could not find: 'cling namespace'.

C:\root-dev\build\x64\llvm-project>

But at least it doesn't print these errors anymore:

error: 'expected-error' diagnostics seen but not expected:
  File input_line_14 Line 3: ValueExtractionSynthesizer could not find: 'cling::runtime::internal::setValueNoAlloc'.
  File input_line_15 Line 2: ValueExtractionSynthesizer could not find: 'cling::runtime::internal::setValueNoAlloc'.

@ferdymercury
Copy link
Contributor Author

Thanks a lot Bertrand for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants