From 9f6c0b34311bbba58a44b7e502f5c5d37bc21565 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Thu, 1 Aug 2024 10:39:22 +0200 Subject: [PATCH 1/3] [cling] Forcefully clean up TemplateIdAnnotations Upstream Clang keeps TemplateIdAnnotations around "if they might still be in the token stream." See upstream commit for more details: https://github.com/llvm/llvm-project/commit/6163aa96799cbad7f2f58e02c5bebee9647056a5 (included in Clang 11, in ROOT since the upgrade to LLVM 13) This reasoning doesn't apply when we fully reset the Parser state in ParserStateRAII's destructor, and we expect the swapped out vector of TemplateIdAnnotations to be empty in order to not leak. Fixes #16121 --- interpreter/cling/lib/Utils/ParserStateRAII.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/interpreter/cling/lib/Utils/ParserStateRAII.cpp b/interpreter/cling/lib/Utils/ParserStateRAII.cpp index d53c3b8c00311..d0eae481986d8 100644 --- a/interpreter/cling/lib/Utils/ParserStateRAII.cpp +++ b/interpreter/cling/lib/Utils/ParserStateRAII.cpp @@ -50,10 +50,13 @@ cling::ParserStateRAII::~ParserStateRAII() { // Note: Consuming the EOF token will pop the include stack. // { - // Cleanup the TemplateIds before swapping the previous set back. - Parser::DestroyTemplateIdAnnotationsRAIIObj CleanupTemplateIds(*P); + // Forcefully clean up the TemplateIds, ignoring additional checks in + // MaybeDestroyTemplateIds called from DestroyTemplateIdAnnotationsRAIIObj, + // before swapping the previous set back. + P->DestroyTemplateIds(); } P->TemplateIds.swap(OldTemplateIds); + assert(OldTemplateIds.empty()); if (SkipToEOF) P->SkipUntil(tok::eof); else From dd237fcfaba3b85f6856ebd850fde63462747e26 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Thu, 1 Aug 2024 11:31:45 +0200 Subject: [PATCH 2/3] [ntuple] Prevent leaking RNTuple anchors Since RNTuple does not inherit from TObject, it is not tracked by the TFile it came from and must be deleted manually. --- tree/ntuple/v7/doc/architecture.md | 2 +- tree/ntuple/v7/test/ntuple_basics.cxx | 3 ++- tree/ntuple/v7/test/ntuple_compat.cxx | 2 +- tree/ntuple/v7/test/ntuple_extended.cxx | 9 ++++++--- tree/ntuple/v7/test/ntuple_storage.cxx | 6 ++++-- tree/ntupleutil/v7/inc/ROOT/RNTupleInspector.hxx | 2 +- tree/ntupleutil/v7/test/ntuple_inspector.cxx | 2 +- tutorials/v7/ntuple/ntpl008_import.C | 2 +- 8 files changed, 17 insertions(+), 11 deletions(-) diff --git a/tree/ntuple/v7/doc/architecture.md b/tree/ntuple/v7/doc/architecture.md index e11ea47777701..287d274c4c8f8 100644 --- a/tree/ntuple/v7/doc/architecture.md +++ b/tree/ntuple/v7/doc/architecture.md @@ -40,7 +40,7 @@ Walkthrough: Reading Data ```c++ auto file = std::make_unique("data.root"); -auto ntuple = file->Get("ntpl"); +auto ntuple = std::unique_ptr(file->Get("ntpl")); // Option 1: entire row // The reader creates a page source; the page source creates a model from the on-disk information diff --git a/tree/ntuple/v7/test/ntuple_basics.cxx b/tree/ntuple/v7/test/ntuple_basics.cxx index fa12cdbb3b78f..abfee7171c402 100644 --- a/tree/ntuple/v7/test/ntuple_basics.cxx +++ b/tree/ntuple/v7/test/ntuple_basics.cxx @@ -214,7 +214,8 @@ TEST(RNTuple, FileAnchor) auto readerB = RNTupleReader::Open("B", fileGuard.GetPath()); auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str())); - auto readerA = RNTupleReader::Open(*f->Get("A")); + auto ntuple = std::unique_ptr(f->Get("A")); + auto readerA = RNTupleReader::Open(*ntuple); EXPECT_EQ(1U, readerA->GetNEntries()); EXPECT_EQ(1U, readerB->GetNEntries()); diff --git a/tree/ntuple/v7/test/ntuple_compat.cxx b/tree/ntuple/v7/test/ntuple_compat.cxx index d184d0876eb7c..49bfeda808d13 100644 --- a/tree/ntuple/v7/test/ntuple_compat.cxx +++ b/tree/ntuple/v7/test/ntuple_compat.cxx @@ -120,7 +120,7 @@ TEST(RNTupleCompat, FwdCompat_FutureNTuple) { auto tfile = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "READ")); assert(!tfile->IsZombie()); - auto *ntuple = tfile->Get(kNtupleObjName); + auto ntuple = std::unique_ptr(tfile->Get(kNtupleObjName)); EXPECT_EQ(ntuple->GetVersionEpoch(), RXTuple{}.fVersionEpoch); EXPECT_EQ(ntuple->GetVersionMajor(), RXTuple{}.fVersionMajor); EXPECT_EQ(ntuple->GetVersionMinor(), RXTuple{}.fVersionMinor); diff --git a/tree/ntuple/v7/test/ntuple_extended.cxx b/tree/ntuple/v7/test/ntuple_extended.cxx index 3ad9171fcf8de..bb3d836cdfffa 100644 --- a/tree/ntuple/v7/test/ntuple_extended.cxx +++ b/tree/ntuple/v7/test/ntuple_extended.cxx @@ -167,7 +167,8 @@ TEST(RNTuple, LargeFile1) { auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "READ")); EXPECT_TRUE(f); - auto reader = RNTupleReader::Open(*f->Get("myNTuple")); + auto ntuple = std::unique_ptr(f->Get("myNTuple")); + auto reader = RNTupleReader::Open(*ntuple); auto rdEnergy = reader->GetView("energy"); double chksumRead = 0.0; @@ -257,11 +258,13 @@ TEST(RNTuple, LargeFile2) auto s2 = f->Get("s2"); EXPECT_EQ("two", *s2); - auto reader = RNTupleReader::Open(*f->Get("small")); + auto small = std::unique_ptr(f->Get("small")); + auto reader = RNTupleReader::Open(*small); reader->LoadEntry(0); EXPECT_EQ(42.0f, *reader->GetModel().GetDefaultEntry().GetPtr("pt")); - reader = RNTupleReader::Open(*f->Get("large")); + auto large = std::unique_ptr(f->Get("large")); + reader = RNTupleReader::Open(*large); auto viewE = reader->GetView("E"); double chksumRead = 0.0; for (auto i : reader->GetEntryRange()) { diff --git a/tree/ntuple/v7/test/ntuple_storage.cxx b/tree/ntuple/v7/test/ntuple_storage.cxx index 40837a412385f..6ae450594ffd2 100644 --- a/tree/ntuple/v7/test/ntuple_storage.cxx +++ b/tree/ntuple/v7/test/ntuple_storage.cxx @@ -399,7 +399,8 @@ TEST(RNTuple, PageFillingString) TEST(RNTuple, OpenHTTP) { std::unique_ptr file(TFile::Open("http://root.cern/files/tutorials/ntpl004_dimuon_v1rc2.root")); - auto reader = RNTupleReader::Open(*file->Get("Events")); + auto Events = std::unique_ptr(file->Get("Events")); + auto reader = RNTupleReader::Open(*Events); reader->LoadEntry(0); } #endif @@ -416,7 +417,8 @@ TEST(RNTuple, TMemFile) writer->Fill(); } - auto reader = RNTupleReader::Open(*file.Get("ntpl")); + auto ntpl = std::unique_ptr(file.Get("ntpl")); + auto reader = RNTupleReader::Open(*ntpl); auto pt = reader->GetModel().GetDefaultEntry().GetPtr("pt"); reader->LoadEntry(0); EXPECT_EQ(*pt, 42.0); diff --git a/tree/ntupleutil/v7/inc/ROOT/RNTupleInspector.hxx b/tree/ntupleutil/v7/inc/ROOT/RNTupleInspector.hxx index fea302a4a7b57..aaef8f5207b0b 100644 --- a/tree/ntupleutil/v7/inc/ROOT/RNTupleInspector.hxx +++ b/tree/ntupleutil/v7/inc/ROOT/RNTupleInspector.hxx @@ -63,7 +63,7 @@ using ROOT::Experimental::RNTuple; using ROOT::Experimental::RNTupleInspector; auto file = TFile::Open("data.rntuple"); -auto rntuple = file->Get("NTupleName"); +auto rntuple = std::unique_ptr(file->Get("NTupleName")); auto inspector = RNTupleInspector::Create(rntuple); std::cout << "The compression factor is " << inspector->GetCompressionFactor() diff --git a/tree/ntupleutil/v7/test/ntuple_inspector.cxx b/tree/ntupleutil/v7/test/ntuple_inspector.cxx index ecc84f3456bf0..999abaa64f478 100644 --- a/tree/ntupleutil/v7/test/ntuple_inspector.cxx +++ b/tree/ntupleutil/v7/test/ntuple_inspector.cxx @@ -27,7 +27,7 @@ TEST(RNTupleInspector, CreateFromPointer) } std::unique_ptr file(TFile::Open(fileGuard.GetPath().c_str())); - auto ntuple = file->Get("ntuple"); + auto ntuple = std::unique_ptr(file->Get("ntuple")); auto inspector = RNTupleInspector::Create(*ntuple); EXPECT_EQ(inspector->GetDescriptor()->GetName(), "ntuple"); } diff --git a/tutorials/v7/ntuple/ntpl008_import.C b/tutorials/v7/ntuple/ntpl008_import.C index acb5783683329..6b32198fd947f 100644 --- a/tutorials/v7/ntuple/ntpl008_import.C +++ b/tutorials/v7/ntuple/ntpl008_import.C @@ -53,7 +53,7 @@ void ntpl008_import() std::cerr << "cannot open " << kNTupleFileName << std::endl; return; } - auto ntpl = file->Get("Events"); + auto ntpl = std::unique_ptr(file->Get("Events")); auto reader = RNTupleReader::Open(*ntpl); reader->PrintInfo(); From ea9c9cb64f8b6539dd4d5cdb6d8ca0d793d52c57 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Thu, 1 Aug 2024 11:38:52 +0200 Subject: [PATCH 3/3] [ntuple] Null buffers in RFileSimple Valgrind complains that "Syscall param write(buf) points to uninitialised byte(s)". Fixes commit 0bfd511e1a ("Implement buffering in RFileSimple") --- tree/ntuple/v7/src/RMiniFile.cxx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tree/ntuple/v7/src/RMiniFile.cxx b/tree/ntuple/v7/src/RMiniFile.cxx index c8fe7f08e6a45..1a234b5d30605 100644 --- a/tree/ntuple/v7/src/RMiniFile.cxx +++ b/tree/ntuple/v7/src/RMiniFile.cxx @@ -1310,7 +1310,9 @@ ROOT::Experimental::Internal::RNTupleFileWriter::RFileSimple::RFileSimple() static_assert(kBlockSize % kBlockAlign == 0, "invalid block size"); std::align_val_t blockAlign{kBlockAlign}; fHeaderBlock = static_cast(::operator new[](kHeaderBlockSize, blockAlign)); + memset(fHeaderBlock, 0, kHeaderBlockSize); fBlock = static_cast(::operator new[](kBlockSize, blockAlign)); + memset(fBlock, 0, kBlockSize); } ROOT::Experimental::Internal::RNTupleFileWriter::RFileSimple::~RFileSimple() @@ -1411,7 +1413,8 @@ void ROOT::Experimental::Internal::RNTupleFileWriter::RFileSimple::Write(const v if (retval != kBlockSize) throw RException(R__FAIL(std::string("write failed: ") + strerror(errno))); - // TODO: do we want to null the buffer contents? + // Null the buffer contents for good measure. + memset(fBlock, 0, kBlockSize); } fBlockOffset = blockOffset;