From efd9893199d72a024e6de04345573757e2284a89 Mon Sep 17 00:00:00 2001 From: Danilo Piparo Date: Mon, 13 Nov 2023 17:51:37 +0100 Subject: [PATCH 1/3] [Doc] Correct a few typos in TSystem::CompileMacro doxygen --- core/base/src/TSystem.cxx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/core/base/src/TSystem.cxx b/core/base/src/TSystem.cxx index 90d3209209346..655a75e8bf8a4 100644 --- a/core/base/src/TSystem.cxx +++ b/core/base/src/TSystem.cxx @@ -2702,20 +2702,20 @@ static void R__WriteDependencyFile(const TString & build_loc, const TString &dep /// The possible options are: /// - k : keep the shared library after the session end. /// - f : force recompilation. -/// - g : compile with debug symbol -/// - O : optimized the code +/// - g : compile with debug symbol. +/// - O : optimize the code. /// - c : compile only, do not attempt to load the library. -/// - s : silence all informational output -/// - v : output all information output +/// - s : silence all information output. +/// - v : print all information output. /// - d : debug ACLiC, keep all the output files. -/// - - : if buildir is set, use a flat structure (see buildir below) +/// - - : if buildir is set, use a flat structure (see buildir below). /// /// If library_specified is specified, CompileMacro generates the file /// "library_specified".soext where soext is the shared library extension for /// the current platform. /// /// If build_dir is specified, it is used as an alternative 'root' for the -/// generation of the shared library. The library is stored in a sub-directories +/// generation of the shared library. The library is stored in a sub-directories /// of 'build_dir' including the full pathname of the script unless a flat /// directory structure is requested ('-' option). With the '-' option the libraries /// are created directly in the directory 'build_dir'; in particular this means that @@ -2783,7 +2783,7 @@ static void R__WriteDependencyFile(const TString & build_loc, const TString &dep /// root[2] .x myfunc.C++(10,20); /// ~~~ /// The user may sometimes try to compile a script before it has loaded all the -/// needed shared libraries. In this case we want to be helpful and output a +/// needed shared libraries. In this case we want to be helpful and output a /// list of the unresolved symbols. So if the loading of the created shared /// library fails, we will try to build a executable that contains the /// script. The linker should then output a list of missing symbols. @@ -2804,8 +2804,8 @@ static void R__WriteDependencyFile(const TString & build_loc, const TString &dep /// Unix.*.Root.IncludePath: -I$ROOTSYS/include /// WinNT.*.Root.IncludePath: -I%ROOTSYS%/include /// -/// Unix.*.Root.LinkedLibs: -L$ROOTSYS/lib -lBase .... -/// WinNT.*.Root.LinkedLibs: %ROOTSYS%/lib/*.lib msvcrt.lib .... +/// Unix.*.Root.LinkedLibs: -L$ROOTSYS/lib -lBase ... +/// WinNT.*.Root.LinkedLibs: %ROOTSYS%/lib/*.lib msvcrt.lib ... /// ~~~ /// And also support for MakeSharedLibs() and MakeExe(). /// From ca935f405649240a98f83181f772804b0b27eb9f Mon Sep 17 00:00:00 2001 From: Danilo Piparo Date: Thu, 16 Nov 2023 15:01:53 +0100 Subject: [PATCH 2/3] [Core] New TSystem::CompileMacro option "h" When TSystem::CompileMacro is invoked with new option "h", a hash of the macro name and content is produced and embedded as a symbol in the library. Then, even if the timestamp of the file is changed, if the hash is the same, the macro is not recompiled and the interpreter is not accessed. This fixes memory hoarding of workers of distributed systems when a. A C macro is used to accelerate some python functions b. A setup of the worker is not possible (for example only a setup per task is allowed) --- core/base/src/TSystem.cxx | 71 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/core/base/src/TSystem.cxx b/core/base/src/TSystem.cxx index 655a75e8bf8a4..96dc00d81ce29 100644 --- a/core/base/src/TSystem.cxx +++ b/core/base/src/TSystem.cxx @@ -46,6 +46,7 @@ allows a simple partial implementation for new OS'es. #include "THashList.h" #include "ThreadLocalStorage.h" +#include #include #include #include @@ -2705,8 +2706,9 @@ static void R__WriteDependencyFile(const TString & build_loc, const TString &dep /// - g : compile with debug symbol. /// - O : optimize the code. /// - c : compile only, do not attempt to load the library. +/// - h : compile only if the macro changed or it's the first time it's compiled and loaded. /// - s : silence all information output. -/// - v : print all information output. +/// - v : printgi all information output. /// - d : debug ACLiC, keep all the output files. /// - - : if buildir is set, use a flat structure (see buildir below). /// @@ -2827,6 +2829,7 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt, Bool_t withInfo = kTRUE; Bool_t verbose = kFALSE; Bool_t internalDebug = kFALSE; + Bool_t compileIfDifferent = kFALSE; if (opt) { keep = (strchr(opt,'k')!=nullptr); recompile = (strchr(opt,'f')!=nullptr); @@ -2842,7 +2845,50 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt, withInfo = strchr(opt, 's') == nullptr; verbose = strchr(opt, 'v') != nullptr; internalDebug = strchr(opt, 'd') != nullptr; + compileIfDifferent = strchr(opt,'h') != nullptr; } + + // ======= Get the right file names for the dictionary and the shared library + TString expFileName(filename); + ExpandPathName( expFileName ); + expFileName = gSystem->UnixPathName(expFileName); + + std::string hash_symbol_name = "__ROOT_Internal_CompileMacro_"; + if (compileIfDifferent) { + std::ifstream inputFile(expFileName); + if (!inputFile) { + ::Error("ACLiC", "The input macro file '%s' could not be opened.", expFileName.Data()); + return 0; + } + std::string fileContent((std::istreambuf_iterator(inputFile)), (std::istreambuf_iterator())); + inputFile.close(); + fileContent += expFileName; + auto hash = std::hash{}(fileContent); + hash_symbol_name += std::to_string(hash); + + // We need to check for the existence of the symbol from within libCling. This is because that library + // is loaded specifying RTLD_LOCAL and therefore hiding all symbols from the process as well as all the + // symbols of the libraries it loads. Therefore, we jit a helper for that. + static void* (*dlsym_jit)(const char *) = nullptr; + if (!dlsym_jit) { + const std::string dlsym_jit_name("__ROOT_Internal_CompileMacro_lsym_jit"); + std::string dlsym_jit_code = "void *"; + dlsym_jit_code += + dlsym_jit_name + "(const char* hash_symbol_name) {return dlsym(RTLD_DEFAULT, hash_symbol_name);} "; + gInterpreter->Declare(dlsym_jit_code.c_str()); + dlsym_jit = (void *(*)(const char *))gInterpreter->Calc(dlsym_jit_name.c_str()); + } + + auto hash_symbol = dlsym_jit(hash_symbol_name.c_str()); + if (hash_symbol) { + // We found the symbol that signals that the macro has been compiled and that the library is loaded. + // We have nothing else to do but to return happiness. + if (withInfo) + ::Info("ACLiC", "The macro has been already built and loaded according to its checksum."); + return 1; + } + } + if (mode==kDefault) { TString rootbuild = ROOTBUILD; if (rootbuild.Index("debug",0,TString::kIgnoreCase)==kNPOS) { @@ -2889,10 +2935,7 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt, incPath.Prepend(":.:"); incPath.Prepend(WorkingDirectory()); - // ======= Get the right file names for the dictionary and the shared library - TString expFileName(filename); - ExpandPathName( expFileName ); - expFileName = gSystem->UnixPathName(expFileName); + // ======= Get the right file names for the shared library TString library = expFileName; if (! IsAbsoluteFileName(library) ) { @@ -3580,6 +3623,24 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt, // ======= Load the library the script might depend on if (result) { + if (compileIfDifferent) { + // Add to the dictionary the symbol which will be checked + // to avoid to rebuild, unload and reload the library if requested by the user + // with the "h" option + std::ofstream dictfile(dict, std::ios::app); + if (!dictfile) { + // This is a sign of a serious issue + ::Error("ACLiC", "Trying to append symbol '%s' to the dictionary file failed.", hash_symbol_name.c_str()); + return 0; + } + dictfile + << "// This symbol is added to check if the library is already loaded without using the interpreter." + << std::endl + << "extern \"C\"" << std::endl + << "{void* " + hash_symbol_name + "() {return (void*) &__TheDictionaryInitializer;}}" << std::endl; + dictfile.close(); + } + TString linkedlibs = GetLibraries("", "S"); TString libtoload; TString all_libtoload; From 41c7b93716ac475e0cb12d495233efe798622134 Mon Sep 17 00:00:00 2001 From: Danilo Piparo Date: Thu, 16 Nov 2023 15:07:30 +0100 Subject: [PATCH 3/3] [Core] Test the TSystem::CompileMacro "h" option --- core/base/test/CMakeLists.txt | 1 + core/base/test/TSystemTests.cxx | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 core/base/test/TSystemTests.cxx diff --git a/core/base/test/CMakeLists.txt b/core/base/test/CMakeLists.txt index 9b52e834058e3..8d7856623f22e 100644 --- a/core/base/test/CMakeLists.txt +++ b/core/base/test/CMakeLists.txt @@ -21,6 +21,7 @@ ROOT_ADD_GTEST(CoreBaseTests TQObjectTests.cxx TExceptionHandlerTests.cxx TStringTest.cxx + TSystemTests.cxx TBitsTests.cxx LIBRARIES ${extralibs} RIO Core) diff --git a/core/base/test/TSystemTests.cxx b/core/base/test/TSystemTests.cxx new file mode 100644 index 0000000000000..da74e72c5ff7c --- /dev/null +++ b/core/base/test/TSystemTests.cxx @@ -0,0 +1,24 @@ +#include "gtest/gtest.h" + +#include "ROOT/TestSupport.hxx" + +#include "TSystem.h" + +#include + +TEST(TSystem, CompileMacroSimple) +{ + const auto srcFileName = "testmacro.C"; + + std::ofstream srcFile; + srcFile.open(srcFileName); + srcFile << "int testmacro(){return 42;}"; + srcFile.close(); + + gSystem->CompileMacro(srcFileName, "hs"); + ROOT_EXPECT_INFO(gSystem->CompileMacro(srcFileName), "ACLiC", + "unmodified script has already been compiled and loaded"); + ROOT_EXPECT_INFO(gSystem->CompileMacro(srcFileName, "h"), "ACLiC", + "The macro has been already built and loaded according to its checksum."); + gSystem->Unlink(srcFileName); +}