[cling] Review workarounds for upstreamed patches

hahnjo commented 11 months ago

[ ] DeclUnloader::VisitNamedDecl: https://reviews.llvm.org/D119675 was committed in https://github.com/llvm/llvm-project/commit/8c0eb32d2aa0bc73c176d7b25f47bdf37f967d3b, available since LLVM 15: https://github.com/root-project/root/blob/44299a000236b7d16a5458917167ebbd306e11c3/interpreter/cling/lib/Interpreter/DeclUnloader.cpp#L559-L563
[ ] ExternalInterpreterSource::ImportDecl https://github.com/root-project/root/blob/44299a000236b7d16a5458917167ebbd306e11c3/interpreter/cling/lib/Interpreter/ExternalInterpreterSource.cpp#L100-L118

I'm pretty sure there are more - these are the first results grepping for FIXME and TODO in interpreter/cling/.

ferdymercury commented 11 months ago

not sure if it should go in this issue, but llvm13 should be renamed to 16 in a couple of places:

/opt/root_src/bindings/pyroot/cppyy/cppyy/etc/valgrind-cppyy-cling.supp:340:   fun:_ZN4llvm13FPPassManager13runOnFunctionERNS_8FunctionE
/opt/root_src/bindings/pyroot/cppyy/cppyy/etc/valgrind-cppyy-cling.supp:350:   fun:_ZN4llvm13FPPassManager16doInitializationERNS_6ModuleE
/opt/root_src/bindings/pyroot/pythonizations/test/CMakeLists.txt:116:        if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/cmake/modules/RootBuildOptions.cmake:188:ROOT_BUILD_OPTION(llvm13_broken_tests OFF "Enable broken tests with LLVM 13 on Windows")
/opt/root_src/cmake/modules/SetUpWindows.cmake:48:    if(llvm13_broken_tests)
/opt/root_src/core/thread/test/CMakeLists.txt:15:if(NOT MSVC OR CMAKE_SIZEOF_VOID_P EQUAL 8 OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:138:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:169:  if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:178:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:242:    if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:252:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/tmva/tmva/test/CMakeLists.txt:30:    if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/tree/ntuple/v7/test/CMakeLists.txt:40:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/tutorials/CMakeLists.txt:409:if(MSVC AND NOT llvm13_broken_tests)
/opt/root_src/cmake/modules/SetUpWindows.cmake:49:      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DR__ENABLE_LLVM13_BROKEN_TESTS")

hahnjo commented 11 months ago

/opt/root_src/bindings/pyroot/cppyy/cppyy/etc/valgrind-cppyy-cling.supp:340:   fun:_ZN4llvm13FPPassManager13runOnFunctionERNS_8FunctionE
/opt/root_src/bindings/pyroot/cppyy/cppyy/etc/valgrind-cppyy-cling.supp:350:   fun:_ZN4llvm13FPPassManager16doInitializationERNS_6ModuleE

This is not llvm13 as a version number, but is a mangled C++ name.

/opt/root_src/bindings/pyroot/pythonizations/test/CMakeLists.txt:116:        if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/cmake/modules/RootBuildOptions.cmake:188:ROOT_BUILD_OPTION(llvm13_broken_tests OFF "Enable broken tests with LLVM 13 on Windows")
/opt/root_src/cmake/modules/SetUpWindows.cmake:48:    if(llvm13_broken_tests)
/opt/root_src/core/thread/test/CMakeLists.txt:15:if(NOT MSVC OR CMAKE_SIZEOF_VOID_P EQUAL 8 OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:138:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:169:  if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:178:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:242:    if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/test/CMakeLists.txt:252:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/tmva/tmva/test/CMakeLists.txt:30:    if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/tree/ntuple/v7/test/CMakeLists.txt:40:if(NOT MSVC OR llvm13_broken_tests)
/opt/root_src/tutorials/CMakeLists.txt:409:if(MSVC AND NOT llvm13_broken_tests)
/opt/root_src/cmake/modules/SetUpWindows.cmake:49:      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DR__ENABLE_LLVM13_BROKEN_TESTS")

I argue that this is correct: These tests were broken as part of the upgrade to LLVM 13. They need to be reviewed, fixed (or checked that they are fixed by the upgrade to LLVM 16), and re-enabled.

dpiparo commented 5 months ago

@hahnjo Is this still relevant?

hahnjo commented 2 months ago

Yes, I still see the FIXMEs in the code after the upgrade to LLVM 18. FYI @devajithvs this is the issue we were discussing yesterday...

devajithvs commented 2 months ago

Yes, I will look into the remaining FIXMEs and TODOs.

root-project / root

[cling] Review workarounds for upstreamed patches #14210