Skip to content

[core] Reduce symbol search only to when autoloading is enabled. #14287

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.

Sign up for GitHub

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

Merged
vgvassilev merged 4 commits into from
Jan 15, 2024
Merged

[core] Reduce symbol search only to when autoloading is enabled. #14287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
984d562
[core] Reduce symbol search only to when autoloading is enabled.
vgvassilev Jan 8, 2024
c22be55
[core] Evaluate initializer of constant variables
vgvassilev Jan 8, 2024
566e73c
[core] Prefer variable initializer over symbol search
vgvassilev Jan 8, 2024
55145f0
Revert "[core] Materialize symbols for TError variables"
vgvassilev Dec 20, 2023
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
17 changes: 9 additions & 8 deletions core/foundation/inc/TError.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,15 @@

class TVirtualMutex;

R__EXTERN const Int_t kUnset;
R__EXTERN const Int_t kPrint;
R__EXTERN const Int_t kInfo;
R__EXTERN const Int_t kWarning;
R__EXTERN const Int_t kError;
R__EXTERN const Int_t kBreak;
R__EXTERN const Int_t kSysError;
R__EXTERN const Int_t kFatal;
constexpr Int_t kUnset = -1;
constexpr Int_t kPrint = 0;
constexpr Int_t kInfo = 1000;
constexpr Int_t kWarning = 2000;
constexpr Int_t kError = 3000;
constexpr Int_t kBreak = 4000;
constexpr Int_t kSysError = 5000;
constexpr Int_t kFatal = 6000;


// TROOT sets the error ignore level handler, the system error message handler, and the error abort handler on
// construction such that the "Root.ErrorIgnoreLevel" environment variable is used for the ignore level
Expand Down
2 changes: 1 addition & 1 deletion core/foundation/src/RLogger.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ inline bool RLogHandlerDefault::Emit(const RLogEntry &entry)
if (!entry.fLocation.fFuncName.empty())
strm << " in " << entry.fLocation.fFuncName;

static const int errorLevelOld[] = {kFatal /*unset*/, kFatal, kError, kWarning, kInfo, kInfo /*debug*/};
static constexpr const int errorLevelOld[] = {kFatal /*unset*/, kFatal, kError, kWarning, kInfo, kInfo /*debug*/};
(*::GetErrorHandler())(errorLevelOld[cappedLevel], entry.fLevel == ELogLevel::kFatal, strm.str().c_str(),
entry.fMessage.c_str());
return true;
Expand Down
9 changes: 0 additions & 9 deletions core/foundation/src/TError.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@ to be replaced by the proper DefaultErrorHandler()
#include <cerrno>
#include <string>

const Int_t kUnset = -1;
const Int_t kPrint = 0;
const Int_t kInfo = 1000;
const Int_t kWarning = 2000;
const Int_t kError = 3000;
const Int_t kBreak = 4000;
const Int_t kSysError = 5000;
const Int_t kFatal = 6000;

Int_t gErrorIgnoreLevel = kUnset;
Int_t gErrorAbortLevel = kSysError+1;
Bool_t gPrintViaErrorHandler = kFALSE;
Expand Down
5 changes: 3 additions & 2 deletions core/metacling/src/TCling.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4726,6 +4726,9 @@ TInterpreter::DeclId_t TCling::GetDataMember(ClassInfo_t *opaque_cl, const char
DeclId_t d;
TClingClassInfo *cl = (TClingClassInfo*)opaque_cl;

// Could trigger deserialization of decls.
Copy link
Member

@dpiparo dpiparo Dec 27, 2023

Choose a reason for hiding this comment

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

I do not understand exactly why this had to move up in the cxx file: could you perhaps clarify, please?

Copy link
Member Author

@vgvassilev vgvassilev Dec 27, 2023

Choose a reason for hiding this comment

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

Because the tests make GetDataMember now deserialize.

Copy link
Member

@hahnjo hahnjo Jan 11, 2024

Choose a reason for hiding this comment

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

It seems I can just remove this and tests still pass on my system. Can you assess if this is still needed in the latest version of the PR? Shall we try reverting it?

Copy link
Member Author

@vgvassilev vgvassilev Jan 11, 2024

Choose a reason for hiding this comment

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

No, it was needed on my system.

cling::Interpreter::PushTransactionRAII RAII(GetInterpreterImpl());

if (cl) {
d = cl->GetDataMember(name);
// We check if the decl of the data member has an annotation which indicates
Expand Down Expand Up @@ -4756,8 +4759,6 @@ TInterpreter::DeclId_t TCling::GetDataMember(ClassInfo_t *opaque_cl, const char
LookupResult R(SemaR, DName, SourceLocation(), Sema::LookupOrdinaryName,
Sema::ForExternalRedeclaration);

// Could trigger deserialization of decls.
cling::Interpreter::PushTransactionRAII RAII(GetInterpreterImpl());
cling::utils::Lookup::Named(&SemaR, R);

LookupResult::Filter F = R.makeFilter();
Expand Down
30 changes: 19 additions & 11 deletions core/metacling/src/TClingCallbacks.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,24 @@ extern "C" {
}

class AutoloadLibraryMU : public llvm::orc::MaterializationUnit {
const TClingCallbacks &fCallbacks;
std::string fLibrary;
llvm::orc::SymbolNameVector fSymbols;
public:
AutoloadLibraryMU(const std::string &Library, const llvm::orc::SymbolNameVector &Symbols)
: MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), fLibrary(Library), fSymbols(Symbols)
AutoloadLibraryMU(const TClingCallbacks &cb, const std::string &Library, const llvm::orc::SymbolNameVector &Symbols)
: MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), fCallbacks(cb), fLibrary(Library), fSymbols(Symbols)
{
}

StringRef getName() const override { return "<Symbols from Autoloaded Library>"; }

void materialize(std::unique_ptr<llvm::orc::MaterializationResponsibility> R) override
{
if (!fCallbacks.IsAutoLoadingEnabled()) {
R->failMaterialization();
return;
}

llvm::orc::SymbolMap loadedSymbols;
llvm::orc::SymbolNameSet failedSymbols;
bool loadedLibrary = false;
Expand Down Expand Up @@ -152,19 +160,22 @@ class AutoloadLibraryMU : public llvm::orc::MaterializationUnit {
map[symbolName] = llvm::JITSymbolFlags::Exported;
return map;
}

std::string fLibrary;
llvm::orc::SymbolNameVector fSymbols;
};

class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator {
const TClingCallbacks &fCallbacks;
cling::Interpreter *fInterpreter;
public:
AutoloadLibraryGenerator(cling::Interpreter *interp) : fInterpreter(interp) {}
AutoloadLibraryGenerator(cling::Interpreter *interp, const TClingCallbacks& cb)
: fCallbacks(cb), fInterpreter(interp) {}

llvm::Error tryToGenerate(llvm::orc::LookupState &LS, llvm::orc::LookupKind K, llvm::orc::JITDylib &JD,
llvm::orc::JITDylibLookupFlags JDLookupFlags,
const llvm::orc::SymbolLookupSet &Symbols) override
{
if (!fCallbacks.IsAutoLoadingEnabled())
llvm::Error::success();

// If we get here, the symbols have not been found in the current process,
// so no need to check that again. Instead search for the library that
// provides the symbol and create one MaterializationUnit per library to
Expand Down Expand Up @@ -199,7 +210,7 @@ class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator {
}

for (auto &&KV : found) {
auto MU = std::make_unique<AutoloadLibraryMU>(KV.first, std::move(KV.second));
auto MU = std::make_unique<AutoloadLibraryMU>(fCallbacks, KV.first, std::move(KV.second));
if (auto Err = JD.define(MU))
return Err;
}
Expand All @@ -210,9 +221,6 @@ class AutoloadLibraryGenerator : public llvm::orc::DefinitionGenerator {

return llvm::Error::success();
}

private:
cling::Interpreter *fInterpreter;
};

TClingCallbacks::TClingCallbacks(cling::Interpreter *interp, bool hasCodeGen) : InterpreterCallbacks(interp)
Expand All @@ -222,7 +230,7 @@ TClingCallbacks::TClingCallbacks(cling::Interpreter *interp, bool hasCodeGen) :
m_Interpreter->declare("namespace __ROOT_SpecialObjects{}", &T);
fROOTSpecialNamespace = dyn_cast<NamespaceDecl>(T->getFirstDecl().getSingleDecl());

interp->addGenerator(std::make_unique<AutoloadLibraryGenerator>(interp));
interp->addGenerator(std::make_unique<AutoloadLibraryGenerator>(interp, *this));
}
}

Expand Down
2 changes: 1 addition & 1 deletion core/metacling/src/TClingCallbacks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class TClingCallbacks : public cling::InterpreterCallbacks {
void Initialize();

void SetAutoLoadingEnabled(bool val = true) { fIsAutoLoading = val; }
bool IsAutoLoadingEnabled() { return fIsAutoLoading; }
bool IsAutoLoadingEnabled() const { return fIsAutoLoading; }

void SetAutoParsingSuspended(bool val = true) { fIsAutoParsingSuspended = val; }
bool IsAutoParsingSuspended() { return fIsAutoParsingSuspended; }
Expand Down
11 changes: 7 additions & 4 deletions core/metacling/src/TClingDataMemberInfo.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,9 @@ Longptr_t TClingDataMemberInfo::Offset()
// static constexpr Long64_t something = std::numeric_limits<Long64_t>::max();
cling::Interpreter::PushTransactionRAII RAII(fInterp);

if (Longptr_t addr = reinterpret_cast<Longptr_t>(fInterp->getAddressOfGlobal(GlobalDecl(VD))))
return addr;
auto evalStmt = VD->ensureEvaluatedStmt();
if (evalStmt && evalStmt->Value) {
// We can't reassign constexpr or const variables. We can compute the
// initializer.
if (VD->hasInit() && (VD->isConstexpr() || VD->getType().isConstQualified())) {
if (const APValue* val = VD->evaluateValue()) {
if (VD->getType()->isIntegralType(C)) {
return reinterpret_cast<Longptr_t>(val->getInt().getRawData());
Expand Down Expand Up @@ -388,6 +387,10 @@ Longptr_t TClingDataMemberInfo::Offset()
} // not integral type
} // have an APValue
} // have an initializing value

// Try the slow operation.
if (Longptr_t addr = reinterpret_cast<Longptr_t>(fInterp->getAddressOfGlobal(GlobalDecl(VD))))
return addr;
}
// FIXME: We have to explicitly check for not enum constant because the
// implementation of getAddressOfGlobal relies on mangling the name and in
Expand Down
32 changes: 32 additions & 0 deletions core/metacling/test/TClingDataMemberInfoTests.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,3 +220,35 @@ class Outer {
auto *dmInnerProtected = (TDataMember*)TClass::GetClass("DMLookup::Outer")->GetListOfDataMembers()->FindObject("InnerProtected");
EXPECT_EQ(dmInnerProtected->Property(), kIsProtected | kIsClass);
}

TEST(TClingDataMemberInfo, Offset)
{
gInterpreter->Declare("int ROOT_7459 = 42; ROOT_7459++;");
EXPECT_TRUE(43 == *(int*)gROOT->GetGlobal("ROOT_7459")->GetAddress());

gInterpreter->Declare("constexpr int var1 = 1;");
EXPECT_TRUE(1 == *(int*)gROOT->GetGlobal("var1")->GetAddress());

gInterpreter->Declare("static constexpr int var2 = -2;");
EXPECT_TRUE(-2 == *(int*)gROOT->GetGlobal("var2")->GetAddress());

gInterpreter->Declare("const float var3 = 3.1;");
EXPECT_FLOAT_EQ(3.1, *(float*)gROOT->GetGlobal("var3")->GetAddress());

gInterpreter->Declare("static const double var4 = 4.2;");
EXPECT_DOUBLE_EQ(4.2, *(double*)gROOT->GetGlobal("var4")->GetAddress());

// Make sure ROOT's Core constexpr constants work
EXPECT_EQ(3000, *(int*)gROOT->GetGlobal("kError")->GetAddress());

#ifdef R__USE_CXXMODULES
// gGeoManager is defined in the Geom libraries and we want to make sure we
// do not load it when autoloading is off. We can only test this in modules
// mode because gGeoManager is not part of the PCH and non-modular ROOT has
// header parsing and autoloading coupled leading to redundant load of
// libGeom at gROOT->GetGlobal time.
TGlobal *GeoManagerInfo = gROOT->GetGlobal("gGeoManager");
TInterpreter::SuspendAutoLoadingRAII autoloadOff(gInterpreter);
EXPECT_TRUE(-1L == (ptrdiff_t)GeoManagerInfo->GetAddress());
#endif // R__USE_CXXMODULES
}