Skip to content

[Core] New TSystem::CompileMacro flag, "h" #14067

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 74 additions & 13 deletions core/base/src/TSystem.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ allows a simple partial implementation for new OS'es.
#include "THashList.h"
#include "ThreadLocalStorage.h"

#include <dlfcn.h>
Copy link
Member

Choose a reason for hiding this comment

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

Is this used?

#include <functional>
#include <iostream>
#include <fstream>
Expand Down Expand Up @@ -2702,20 +2703,21 @@ 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
/// - h : compile only if the macro changed or it's the first time it's compiled and loaded.
/// - s : silence 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)
/// - - : 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
Expand Down Expand Up @@ -2783,7 +2785,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.
Expand All @@ -2804,8 +2806,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().
///
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Bool_t compileIfDifferent = kFALSE;
Bool_t compileOnlyIfDifferent = kFALSE;

if (opt) {
keep = (strchr(opt,'k')!=nullptr);
recompile = (strchr(opt,'f')!=nullptr);
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

We should check both the source and the optional corresponding header file.

if (!inputFile) {
::Error("ACLiC", "The input macro file '%s' could not be opened.", expFileName.Data());
return 0;
}
std::string fileContent((std::istreambuf_iterator<char>(inputFile)), (std::istreambuf_iterator<char>()));
inputFile.close();
fileContent += expFileName;
auto hash = std::hash<std::string>{}(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);} ";
Copy link
Member

Choose a reason for hiding this comment

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

dlsym is (was?) not portable to Windows. See TROOT.cxx TCling.cxx TProtoClass.cxx and TClass.cxx (where TROOT.cxx seems to be the more recent one. (and yes we could use a factorization of those 4 instances).

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) {
Expand Down Expand Up @@ -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) )
{
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

On windows, this means reopening a file that was just written, it may induced race condition with the file system locks (or maybe not). An alternative is to produce yet another file and have it included in the dictionary source file by rootcling by using, in the LinkDef.h the #pragma extra_include ... line.

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;
Expand Down
1 change: 1 addition & 0 deletions core/base/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ ROOT_ADD_GTEST(CoreBaseTests
TQObjectTests.cxx
TExceptionHandlerTests.cxx
TStringTest.cxx
TSystemTests.cxx
TBitsTests.cxx
LIBRARIES ${extralibs} RIO Core)

Expand Down
24 changes: 24 additions & 0 deletions core/base/test/TSystemTests.cxx
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#include "gtest/gtest.h"

#include "ROOT/TestSupport.hxx"

#include "TSystem.h"

#include <fstream>

TEST(TSystem, CompileMacroSimple)
{
const auto srcFileName = "testmacro.C";

std::ofstream srcFile;
srcFile.open(srcFileName);
srcFile << "int testmacro(){return 42;}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
srcFile << "int testmacro(){return 42;}";
srcFile << "int testmacro(){return 42;}\n";

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.");
Comment on lines +21 to +22
Copy link
Member

@pcanal pcanal Nov 16, 2023

Choose a reason for hiding this comment

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

We should also test that it is NOT being rebuild upon the timestamp being updated and that it IS being rebuild upon the source (or header files) being changed.

gSystem->Unlink(srcFileName);
}