diff --git a/cmake/modules/RootCTest.cmake b/cmake/modules/RootCTest.cmake index e434770ce0729..3cbc55098c073 100644 --- a/cmake/modules/RootCTest.cmake +++ b/cmake/modules/RootCTest.cmake @@ -51,17 +51,19 @@ foreach(d ${test_list}) endif() endforeach() -# When ninja is in use, tests that compile an executable might try to rebuild the entire build tree. -# If multiple of these are invoked in parallel, ninja will suffer from race conditions. +# When ninja or the Microsoft generator are in use, tests that compile an executable might try +# to rebuild the entire build tree. If multiple of these are invoked in parallel, ninja will +# suffer from race conditions. # To solve this, do the following: # - Add a test that updates the build tree (equivalent to "ninja all"). This one will run in complete isolation. # - Make all tests that require a ninja build depend on the above test. # - Use a RESOURCE_LOCK on all tests that invoke ninja, so no two tests will invoke ninja in parallel -if(CMAKE_GENERATOR MATCHES Ninja) - add_test(NAME ninja-build-all - COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR}) - set_tests_properties(ninja-build-all PROPERTIES - RESOURCE_LOCK NINJA_BUILD - FIXTURES_SETUP NINJA_BUILD_ALL +if(GeneratorNeedsBuildSerialization) + add_test(NAME cmake-build-all + COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --config ${build_configuration}) + set_tests_properties(cmake-build-all PROPERTIES + RESOURCE_LOCK CMAKE_BUILD + FIXTURES_SETUP CMAKE_BUILD_ALL RUN_SERIAL True) + set(GeneratorNeedsBuildSerialization True) endif() diff --git a/cmake/modules/RootMacros.cmake b/cmake/modules/RootMacros.cmake index f7f5183fe45c5..f702d88413e79 100644 --- a/cmake/modules/RootMacros.cmake +++ b/cmake/modules/RootMacros.cmake @@ -45,6 +45,10 @@ else() set(ROOT_LIBRARY_PROPERTIES ${ROOT_LIBRARY_PROPERTIES} ${ROOT_LIBRARY_PROPERTIES_NO_VERSION} ) endif() +if (NOT DEFINED ROOT_root_CMD) + set(ROOT_root_CMD $) +endif() + include(CMakeParseArguments) #--------------------------------------------------------------------------------------------------- @@ -2657,9 +2661,9 @@ macro(ROOTTEST_GENERATE_DICTIONARY dictname) -- ${always-make}) set_property(TEST ${GENERATE_DICTIONARY_TEST} PROPERTY ENVIRONMENT ${ROOTTEST_ENVIRONMENT}) - if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) + if(GeneratorNeedsBuildSerialization) + set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() if (ARG_FIXTURES_SETUP) @@ -2765,9 +2769,9 @@ macro(ROOTTEST_GENERATE_REFLEX_DICTIONARY dictionary) -- ${always-make}) set_property(TEST ${GENERATE_REFLEX_TEST} PROPERTY ENVIRONMENT ${ROOTTEST_ENVIRONMENT}) - if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST ${GENERATE_REFLEX_TEST} APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST ${GENERATE_REFLEX_TEST} APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) + if(GeneratorNeedsBuildSerialization) + set_property(TEST ${GENERATE_REFLEX_TEST} APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST ${GENERATE_REFLEX_TEST} APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() if (ARG_FIXTURES_SETUP) @@ -2877,9 +2881,9 @@ macro(ROOTTEST_GENERATE_EXECUTABLE executable) RESOURCE_LOCK ${ARG_RESOURCE_LOCK}) endif() - if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST ${GENERATE_EXECUTABLE_TEST} APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST ${GENERATE_EXECUTABLE_TEST} APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) + if(GeneratorNeedsBuildSerialization) + set_property(TEST ${GENERATE_EXECUTABLE_TEST} APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST ${GENERATE_EXECUTABLE_TEST} APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja) diff --git a/core/base/inc/TBuffer.h b/core/base/inc/TBuffer.h index d6cae935560ed..af6b0bb000f8d 100644 --- a/core/base/inc/TBuffer.h +++ b/core/base/inc/TBuffer.h @@ -69,6 +69,15 @@ class TBuffer : public TObject { Int_t Write(const char *name, Int_t opt, Long64_t bufsize) const override { return TObject::Write(name, opt, bufsize); } + //////////////////////////////////////////////////////////////////////////////// + /// Reserve space for a leading byte count and return the position where to + /// store the byte count value. + /// + /// \param[in] cl The class for which we are reserving the byte count, used for error reporting. + /// \return The position (cntpos) where the byte count should be stored later, + /// or kOverflowPosition if the position exceeds kMaxCountPosition + virtual UInt_t ReserveByteCount(const TClass *) = 0; + public: enum EMode { kRead = 0, kWrite = 1 }; enum EStatusBits { @@ -126,6 +135,58 @@ class TBuffer : public TObject { virtual Long64_t CheckByteCount(ULong64_t startpos, ULong64_t bcnt, const char *classname) = 0; virtual void SetByteCount(ULong64_t cntpos, Bool_t packInVersion = kFALSE)= 0; + /// \class TBuffer::ByteCountWriter + /// \ingroup Base + /// \brief RAII helper to automatically write the byte count for an object + /// to be used in the rare case where writing the class version number + /// and the byte count are decoupled. + /// + /// `ByteCountWriter` encapsulates the pattern: + /// 1. Reserve space for a leading byte count with ReserveByteCount(). + /// 2. Stream the object content. + /// 3. Finalize the byte count with SetByteCount(). + /// + /// \note Create the instance as a local variable and keep it alive until all + /// the bytes that should be counted have been written to the buffer. + /// + /// ### Example + /// \code{.cpp} + /// void MyClass::Streamer(TBuffer &b) + /// { + /// if (b.IsWriting()) { + /// // Reserve space for the byte count and auto-finalize on scope exit. + /// TBuffer::ByteCountWriter bcnt(b, MyClass::Class()); + /// + /// b.WriteClass(MyClass::Class()); + /// // ... stream members ... + /// } else { + /// // ... read members ... + /// } + /// } + /// \endcode + class ByteCountWriter { + TBuffer &fBuffer; + Bool_t fPackInVersion; + ULong64_t fCntPos; + public: + ByteCountWriter() = delete; + ByteCountWriter(const ByteCountWriter&) = delete; + ByteCountWriter& operator=(const ByteCountWriter&) = delete; + ByteCountWriter(ByteCountWriter&&) = delete; + ByteCountWriter& operator=(ByteCountWriter&&) = delete; + + ByteCountWriter(TBuffer &buf, const TClass *cl, Bool_t packInVersion = kFALSE) : fBuffer(buf), fPackInVersion(packInVersion) { + // We could split ReserveByteCount in a 32bit version that uses + // the ByteCountStack and another version that always returns the + // long range position. For now keep it 'simpler' by always using + // the stack. + fCntPos = fBuffer.ReserveByteCount(cl); + } + ~ByteCountWriter() { + fBuffer.SetByteCount(fCntPos, fPackInVersion); + } + }; + virtual void SkipVersion(const TClass *cl = nullptr) = 0; virtual Version_t ReadVersion(UInt_t *start = nullptr, UInt_t *bcnt = nullptr, const TClass *cl = nullptr) = 0; virtual Version_t ReadVersionNoCheckSum(UInt_t *start = nullptr, UInt_t *bcnt = nullptr) = 0; @@ -135,6 +196,23 @@ class TBuffer : public TObject { virtual void *ReadObjectAny(const TClass* cast) = 0; virtual void SkipObjectAny() = 0; + /** \brief Skip, based on a known start position and byte count, to the end of the current object. + * + * \warning Advanced use only. + * + * This overload exists primarily for error handling within a Streamer. + * + * A typical use case is a Streamer with a flow like: + * - Read version and bytecount + * - Start reading the data + * - Detect an error (e.g. missing some information for a CollectionProxy) + * - Properly set the cursor to the end of the object to allow reading to continue. + * + * Because the actual byte count information for \e large byte counts is kept only on the + * internal byte-count stack, there are only two viable options to support this use case: + * provide this overload, or make the stack accessible (e.g. via a getter). + */ + virtual void SkipObjectAny(Long64_t start, UInt_t bytecount) = 0; virtual void TagStreamerInfo(TVirtualStreamerInfo* info) = 0; virtual void IncrementLevel(TVirtualStreamerInfo* info) = 0; diff --git a/core/cont/src/TArray.cxx b/core/cont/src/TArray.cxx index a5283e5523d53..6589c88c510c1 100644 --- a/core/cont/src/TArray.cxx +++ b/core/cont/src/TArray.cxx @@ -90,18 +90,16 @@ void TArray::WriteArray(TBuffer &b, const TArray *a) b << (UInt_t) 0; } else { + TClass *cl = a->IsA(); // Reserve space for leading byte count - UInt_t cntpos = UInt_t(b.Length()); - b.SetBufferOffset(Int_t(cntpos+sizeof(UInt_t))); + TBuffer::ByteCountWriter bcnt(b, cl); - TClass *cl = a->IsA(); b.WriteClass(cl); - ((TArray *)a)->Streamer(b); + const_cast(a)->Streamer(b); - // Write byte count - b.SetByteCount(cntpos); + // The byte count is written automatically by the ByteCountWriter destructor } } diff --git a/core/meta/src/TStreamerElement.cxx b/core/meta/src/TStreamerElement.cxx index 561fca629178d..ef74518c37948 100644 --- a/core/meta/src/TStreamerElement.cxx +++ b/core/meta/src/TStreamerElement.cxx @@ -556,9 +556,12 @@ void TStreamerElement::Streamer(TBuffer &R__b) if (R__v > 3) { if (TestBit(kHasRange)) GetRange(GetTitle(),fXmin,fXmax,fFactor); } - //R__b.CheckByteCount(R__s, R__c, TStreamerElement::IsA()); R__b.ClassEnd(TStreamerElement::Class()); + // Dubious reset to the expected end it was added as part the commit + // log "Several protections added in case of multiple files being read/updated + // in parallel." R__b.SetBufferOffset(R__s+R__c+sizeof(UInt_t)); + R__b.CheckByteCount(R__s, R__c, TStreamerElement::Class()); ResetBit(TStreamerElement::kCache); ResetBit(TStreamerElement::kWrite); @@ -832,6 +835,7 @@ void TStreamerBase::Streamer(TBuffer &R__b) } R__b.ClassEnd(TStreamerBase::Class()); R__b.SetBufferOffset(R__s+R__c+sizeof(UInt_t)); + R__b.CheckByteCount(R__s, R__c, TStreamerBase::Class()); } else { R__b.WriteClassBuffer(TStreamerBase::Class(),this); } @@ -1001,7 +1005,11 @@ void TStreamerBasicPointer::Streamer(TBuffer &R__b) R__b >> fCountVersion; fCountName.Streamer(R__b); fCountClass.Streamer(R__b); + // Dubious reset to the expected end it was added as part the commit + // log "Several protections added in case of multiple files being read/updated + // in parallel." R__b.SetBufferOffset(R__s+R__c+sizeof(UInt_t)); + R__b.CheckByteCount(R__s, R__c, TStreamerBasicPointer::Class()); } else { R__b.WriteClassBuffer(TStreamerBasicPointer::Class(),this); } @@ -1105,7 +1113,11 @@ void TStreamerLoop::Streamer(TBuffer &R__b) R__b >> fCountVersion; fCountName.Streamer(R__b); fCountClass.Streamer(R__b); + // Dubious reset to the expected end it was added as part the commit + // log "Several protections added in case of multiple files being read/updated + // in parallel." R__b.SetBufferOffset(R__s+R__c+sizeof(UInt_t)); + R__b.CheckByteCount(R__s, R__c, TStreamerLoop::Class()); } else { R__b.WriteClassBuffer(TStreamerLoop::Class(),this); } diff --git a/io/io/inc/TBufferFile.h b/io/io/inc/TBufferFile.h index 143f8ac374a69..0c532786d7e51 100644 --- a/io/io/inc/TBufferFile.h +++ b/io/io/inc/TBufferFile.h @@ -53,7 +53,17 @@ class TBufferFile : public TBufferIO { InfoList_t fInfoStack; ///< Stack of pointers to the TStreamerInfos using ByteCountLocator_t = std::size_t; // This might become a pair if we implement chunked keys - using ByteCountStack_t = std::vector; + struct ByteCountLocationInfo { + ///< Position where the byte count value is stored + ByteCountLocator_t locator; + ///< Class for which the byte count is reserved. Usually this is the + ///< the base class or type of the pointer. + const TClass* cl; + ///< Alternative class that might used. Usually this is the derived + ///< class or the actual type of the object. + const TClass* alt; + }; + using ByteCountStack_t = std::vector; ByteCountStack_t fByteCountStack; ///(fBufCur - fBuffer) + assert( (cntpos == kOverflowPosition || + (cntpos < kOverflowPosition && (sizeof(UInt_t) + cntpos) < static_cast(fBufCur - fBuffer))) && (fBufCur >= fBuffer) && static_cast(fBufCur - fBuffer) <= std::numeric_limits::max() && "Byte count position is after the end of the buffer"); @@ -361,7 +362,13 @@ void TBufferFile::SetByteCount(ULong64_t cntpos, Bool_t packInVersion) R__ASSERT(!fByteCountStack.empty()); if (cntpos == kOverflowPosition) { // The position is above 4GB but was cached using a 32 bit variable. - cntpos = fByteCountStack.back(); + cntpos = fByteCountStack.back().locator; + } else { + // This assert allows to reject cases that used to be valid (missing or + // redundant call to SetByteCount) but are now an error because the + // position is passed through a stack (and redundantly via the argument + // for 'small' buffers). + R__ASSERT(cntpos == fByteCountStack.back().locator && "Byte count position on stack does not match the passed cntpos"); } fByteCountStack.pop_back(); // if we are not in the same TKey chunk or if the cntpos is too large to fit in UInt_t @@ -412,9 +419,45 @@ Long64_t TBufferFile::CheckByteCount(ULong64_t startpos, ULong64_t bcnt, const T { R__ASSERT(!fByteCountStack.empty() && startpos <= kOverflowPosition && bcnt <= kOverflowCount && "Byte count stack is empty or invalid startpos/bcnt"); + auto classMatcher = [](const TClass *stackClass, const TClass *passedClass, const char *passedClassName) { + return !stackClass + || (!passedClass && !passedClassName) + || (passedClass && passedClass == stackClass) + || (passedClassName && stackClass->GetName() && strcmp(passedClassName, stackClass->GetName()) == 0); + }; if (startpos == kOverflowPosition) { // The position is above 4GB but was cached using a 32 bit variable. - startpos = fByteCountStack.back(); + startpos = fByteCountStack.back().locator; + // See below + R__ASSERT((fByteCountStack.back().cl == nullptr || clss == fByteCountStack.back().cl) + && "Class on the byte count position stack does not match the passed class"); + } else { + // This assert allows to reject cases that used to be valid (missing or + // redundant call to SetByteCount) but are now an error because the + // position is passed through a stack (and redundantly via the argument + // for 'small' buffers). + const auto stackClass = fByteCountStack.back().cl; + const auto altStackClass = fByteCountStack.back().alt; + const bool classMatches = classMatcher(stackClass, clss, classname) || + classMatcher(altStackClass, clss, classname); + if (startpos != fByteCountStack.back().locator) { + const char *stackClassName = stackClass ? stackClass->GetName() : "not specified"; + const char *passedClassName = clss ? clss->GetName() : classname ? classname : "not specified"; + Fatal("CheckByteCount", + "Incorrect Streamer Function.\n\tByte count position stack mismatch: expected %zu but got %llu.\n\tClass on stack is %s, passed class is %s", + fByteCountStack.back().locator, startpos, + stackClassName ? stackClassName : "nullptr", passedClassName ? passedClassName : "nullptr"); + } else if (!classMatches) { + const char *stackClassName = stackClass ? stackClass->GetName() : nullptr; + const char *passedClassName = clss ? clss->GetName() : classname; + if (classMatcher(stackClass, clss, classname)) { + // In case the mismatch comes from the alt class. + stackClassName = altStackClass ? altStackClass->GetName() : nullptr; + } + Fatal("CheckByteCount", + "Incorrect Streamer Function.\n\tClass mismatch for byte count position %llu: expected %s but got %s", + startpos, stackClassName ? stackClassName : "nullptr", passedClassName ? passedClassName : "nullptr"); + } } if (bcnt == kOverflowCount) { // in case we are checking a byte count for which we postponed @@ -2569,7 +2612,26 @@ void TBufferFile::SkipObjectAny() { UInt_t start, count; ReadVersion(&start, &count); - SetBufferOffset(start+count+sizeof(UInt_t)); + if (count == kOverflowPosition) + SetBufferOffset(start+ fByteCountStack.back().locator + sizeof(UInt_t)); + else + SetBufferOffset(start+count+sizeof(UInt_t)); + // The byte count location is pushed on the stack only if there is + // actually a byte count. + if (count) + fByteCountStack.pop_back(); +} + +//////////////////////////////////////////////////////////////////////////////// +/// Skip any kind of object from buffer + +void TBufferFile::SkipObjectAny(Long64_t start, UInt_t count) +{ + if (count == kOverflowPosition) + SetBufferOffset(start + fByteCountStack.back().locator + sizeof(UInt_t)); + else + SetBufferOffset(start + count + sizeof(UInt_t)); + fByteCountStack.pop_back(); } //////////////////////////////////////////////////////////////////////////////// @@ -2593,10 +2655,12 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) InitMap(); // before reading object save start position - UInt_t startpos = UInt_t(fBufCur-fBuffer); + ULong64_t startpos = static_cast(fBufCur-fBuffer); + ULong64_t cntpos = startpos <= kMaxCountPosition ? startpos : kOverflowPosition; // attempt to load next object as TClass clCast UInt_t tag; // either tag or byte count + // ReadClass will push on fByteCountStack if needed. TClass *clRef = ReadClass(clCast, &tag); TClass *clOnfile = nullptr; Int_t baseOffset = 0; @@ -2613,7 +2677,7 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) Error("ReadObject", "got object of wrong class! requested %s but got %s", clCast->GetName(), clRef->GetName()); - CheckByteCount(startpos, tag, (TClass *)nullptr); // avoid mis-leading byte count error message + CheckByteCount(cntpos, tag, (TClass *)nullptr); // avoid mis-leading byte count error message return 0; // We better return at this point } baseOffset = 0; // For now we do not support requesting from a class that is the base of one of the class for which there is transformation to .... @@ -2628,7 +2692,7 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) //we cannot mix a compiled class with an emulated class in the inheritance Error("ReadObject", "trying to read an emulated class (%s) to store in a compiled pointer (%s)", clRef->GetName(),clCast->GetName()); - CheckByteCount(startpos, tag, (TClass *)nullptr); // avoid mis-leading byte count error message + CheckByteCount(cntpos, tag, (TClass *)nullptr); // avoid mis-leading byte count error message return 0; } } @@ -2640,7 +2704,7 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) obj = (char *) (Longptr_t)fMap->GetValue(startpos+kMapOffset); if (obj == (void*) -1) obj = nullptr; if (obj) { - CheckByteCount(startpos, tag, (TClass *)nullptr); + CheckByteCount(cntpos, tag, (TClass *)nullptr); return (obj + baseOffset); } } @@ -2652,7 +2716,7 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) MapObject((TObject*) -1, startpos+kMapOffset); else MapObject((void*)nullptr, nullptr, fMapCount); - CheckByteCount(startpos, tag, (TClass *)nullptr); + CheckByteCount(cntpos, tag, (TClass *)nullptr); return 0; } @@ -2711,7 +2775,7 @@ void *TBufferFile::ReadObjectAny(const TClass *clCast) // let the object read itself clRef->Streamer( obj, *this, clOnfile ); - CheckByteCount(startpos, tag, clRef); + CheckByteCount(cntpos, tag, clRef); } return obj+baseOffset; @@ -2767,7 +2831,7 @@ void TBufferFile::WriteObjectClass(const void *actualObjectStart, const TClass * } // reserve space for leading byte count - UInt_t cntpos = ReserveByteCount(); + UInt_t cntpos = ReserveByteCount(actualClass); // write class of object first Int_t mapsize = fMap->Capacity(); // The slot depends on the capacity and WriteClass might induce an increase. @@ -2808,11 +2872,9 @@ TClass *TBufferFile::ReadClass(const TClass *clReq, UInt_t *objTag) R__ASSERT(IsReading()); // read byte count and/or tag (older files don't have byte count) - TClass *cl; if (fBufCur < fBuffer || fBufCur > fBufMax) { fBufCur = fBufMax; - cl = (TClass*)-1; - return cl; + return (TClass*)-1; } UInt_t bcnt, tag, startpos = 0; *this >> bcnt; @@ -2823,8 +2885,10 @@ TClass *TBufferFile::ReadClass(const TClass *clReq, UInt_t *objTag) fVersion = 1; // When objTag is not used, the caller is not interested in the byte // count and will not (can not) call CheckByteCount. - if (objTag) - fByteCountStack.push_back(fBufCur - fBuffer); + if (objTag) { + // Note: the actual class is set later on. + fByteCountStack.push_back({fBufCur - fBuffer - sizeof(UInt_t), clReq, nullptr}); + } startpos = UInt_t(fBufCur-fBuffer); *this >> tag; } @@ -2835,6 +2899,7 @@ TClass *TBufferFile::ReadClass(const TClass *clReq, UInt_t *objTag) return 0; } + TClass *cl; if (tag == kNewClassTag) { // got a new class description followed by a new object @@ -2882,7 +2947,11 @@ TClass *TBufferFile::ReadClass(const TClass *clReq, UInt_t *objTag) } // return bytecount in objTag - if (objTag) *objTag = (bcnt & ~kByteCountMask); + if (objTag) { + *objTag = (bcnt & ~kByteCountMask); + if (cl) + fByteCountStack.back().alt = cl; + } // case of unknown class if (!cl) cl = (TClass*)-1; @@ -3015,7 +3084,7 @@ Version_t TBufferFile::ReadVersion(UInt_t *startpos, UInt_t *bcnt, const TClass // before reading object save start position auto full_startpos = fBufCur - fBuffer; *startpos = full_startpos <= kMaxCountPosition ? UInt_t(full_startpos) : kOverflowPosition; - fByteCountStack.push_back(full_startpos); + fByteCountStack.push_back({(size_t)full_startpos, cl, nullptr}); } // read byte count (older files don't have byte count) @@ -3154,7 +3223,8 @@ Version_t TBufferFile::ReadVersionNoCheckSum(UInt_t *startpos, UInt_t *bcnt) // before reading object save start position auto full_startpos = fBufCur - fBuffer; *startpos = full_startpos < kMaxCountPosition ? UInt_t(full_startpos) : kOverflowPosition; - fByteCountStack.push_back(full_startpos); + // TODO: Extend ReadVersionNoCheckSum to take the class pointer. + fByteCountStack.push_back({(size_t)full_startpos, nullptr, nullptr}); } // read byte count (older files don't have byte count) @@ -3279,7 +3349,7 @@ UInt_t TBufferFile::WriteVersion(const TClass *cl, Bool_t useBcnt) UInt_t cntpos = 0; if (useBcnt) { // reserve space for leading byte count - cntpos = ReserveByteCount(); + cntpos = ReserveByteCount(cl); } Version_t version = cl->GetClassVersion(); @@ -3308,7 +3378,7 @@ UInt_t TBufferFile::WriteVersionMemberWise(const TClass *cl, Bool_t useBcnt) UInt_t cntpos = 0; if (useBcnt) { // reserve space for leading byte count - cntpos = ReserveByteCount(); + cntpos = ReserveByteCount(cl); } Version_t version = cl->GetClassVersion(); @@ -3464,14 +3534,15 @@ UInt_t TBufferFile::CheckObject(UInt_t offset, const TClass *cl, Bool_t readClas /// Reserve space for a leading byte count and return the position where to /// store the byte count value. /// +/// \param[in] cl The class for which we are reserving the byte count, used for error reporting. /// \return The position (cntpos) where the byte count should be stored later, /// or kOverflowPosition if the position exceeds kMaxCountPosition -UInt_t TBufferFile::ReserveByteCount() +UInt_t TBufferFile::ReserveByteCount(const TClass *cl) { // reserve space for leading byte count - auto full_cntpos = fBufCur - fBuffer; - fByteCountStack.push_back(full_cntpos); + size_t full_cntpos = fBufCur - fBuffer; + fByteCountStack.push_back({full_cntpos, cl, nullptr}); *this << (UInt_t)kByteCountMask; // placeholder for byte count return full_cntpos <= kMaxCountPosition ? full_cntpos : kOverflowPosition; } diff --git a/io/io/src/TBufferJSON.cxx b/io/io/src/TBufferJSON.cxx index de880d809b583..63dc652cee39e 100644 --- a/io/io/src/TBufferJSON.cxx +++ b/io/io/src/TBufferJSON.cxx @@ -2589,6 +2589,7 @@ void *TBufferJSON::ReadObjectAny(const TClass *expectedClass) /// Skip any kind of object from buffer void TBufferJSON::SkipObjectAny() {} +void TBufferJSON::SkipObjectAny(Long64_t, UInt_t) {} //////////////////////////////////////////////////////////////////////////////// /// Write object to buffer. Only used from TBuffer diff --git a/io/io/src/TStreamerInfo.cxx b/io/io/src/TStreamerInfo.cxx index 3900b54b37cf8..4fb7814c48b8c 100644 --- a/io/io/src/TStreamerInfo.cxx +++ b/io/io/src/TStreamerInfo.cxx @@ -5503,7 +5503,11 @@ void TStreamerInfo::Streamer(TBuffer &R__b) R__b.ClassMember("fElements","TObjArray*"); R__b >> fElements; R__b.ClassEnd(TStreamerInfo::Class()); + // Dubious reset to the expected end it was added as part the commit + // log "Several protections added in case of multiple files being read/updated + // in parallel." R__b.SetBufferOffset(R__s+R__c+sizeof(UInt_t)); + R__b.CheckByteCount(R__s, R__c, TStreamerInfo::Class()); ResetBit(kIsCompiled); ResetBit(kBuildOldUsed); ResetBit(kBuildRunning); diff --git a/io/io/src/TStreamerInfoReadBuffer.cxx b/io/io/src/TStreamerInfoReadBuffer.cxx index 8135f9416fc82..611abf936bacb 100644 --- a/io/io/src/TStreamerInfoReadBuffer.cxx +++ b/io/io/src/TStreamerInfoReadBuffer.cxx @@ -1221,7 +1221,7 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr, } } } - b.CheckByteCount(start,count,aElement->GetFullName()); + b.CheckByteCount(start, count, aElement->GetClass()); continue; } if (pstreamer == 0) { @@ -1279,7 +1279,7 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr, // and the collection is always empty, // So let's skip the rest (which requires the StreamerInfo of the valueClass ... which we do not have) - b.SetBufferOffset(start+count+sizeof(UInt_t)); + b.SkipObjectAny(start, count); continue; } diff --git a/io/io/test/CMakeLists.txt b/io/io/test/CMakeLists.txt index b65a1886a6d17..c39c0eacf39a0 100644 --- a/io/io/test/CMakeLists.txt +++ b/io/io/test/CMakeLists.txt @@ -41,6 +41,21 @@ endif() # ROOT_ADD_TEST(io-io-test-TMapFileTest COMMAND TMapFileTest complete) #endif() -ROOTTEST_ADD_TEST(testLargeByteCounts - MACRO testByteCount.C - OUTREF testByteCount.ref) +# Detect bitness. +# FIXME: This is also done in roottest/CMakeLists.txt which is included 'too late' for +# the tests here. We should unify this logic and do it only once. +if(CMAKE_SIZEOF_VOID_P EQUAL 8) + set(_64BIT 1) +elseif(CMAKE_SIZEOF_VOID_P EQUAL 4) + set(_32BIT 1) +endif() + +if(NOT _32BIT) + ROOTTEST_COMPILE_MACRO(testByteCount.cxx + FIXTURES_SETUP io-io-tobj-fixture) + + ROOTTEST_ADD_TEST(testLargeByteCounts + MACRO testByteCount.cxx+ + OUTREF testByteCount.ref + FIXTURES_REQUIRED io-io-tobj-fixture) +endif() diff --git a/io/io/test/testByteCount.C b/io/io/test/testByteCount.cxx similarity index 96% rename from io/io/test/testByteCount.C rename to io/io/test/testByteCount.cxx index b152e3700aa75..07a6966786e56 100644 --- a/io/io/test/testByteCount.C +++ b/io/io/test/testByteCount.cxx @@ -1,6 +1,14 @@ +#include "TBufferFile.h" +#include "TExMap.h" + +#include +#include + + +int testByteCount() { int errors = 0; - int expectedByteCounts = 1; + unsigned int expectedByteCounts = 1; // TBufferFile currently reject size larger than 2GB. // SetBufferOffset does not check against the size, diff --git a/io/io/test/testByteCount.ref b/io/io/test/testByteCount.ref index a5a2b78b46929..2ac0fa3121d7c 100644 --- a/io/io/test/testByteCount.ref +++ b/io/io/test/testByteCount.ref @@ -1,2 +1,3 @@ Processing io/io/test/testByteCount.C... The end. +(int) 0 diff --git a/io/sql/inc/TBufferSQL2.h b/io/sql/inc/TBufferSQL2.h index 1c7c7e696adae..1217c4f370c57 100644 --- a/io/sql/inc/TBufferSQL2.h +++ b/io/sql/inc/TBufferSQL2.h @@ -149,6 +149,7 @@ class TBufferSQL2 final : public TBufferText { void *ReadObjectAny(const TClass *clCast) final; void SkipObjectAny() final; + void SkipObjectAny(Long64_t start, UInt_t bytecount) final; void IncrementLevel(TVirtualStreamerInfo *) final; void SetStreamerElementNumber(TStreamerElement *elem, Int_t comp_type) final; diff --git a/io/sql/src/TBufferSQL2.cxx b/io/sql/src/TBufferSQL2.cxx index eae8309d44560..c0d5d59dea4cc 100644 --- a/io/sql/src/TBufferSQL2.cxx +++ b/io/sql/src/TBufferSQL2.cxx @@ -862,6 +862,14 @@ void TBufferSQL2::SkipObjectAny() { } +//////////////////////////////////////////////////////////////////////////////// +/// Skip any kind of object from buffer +/// Actually skip only one node on current level of xml structure + +void TBufferSQL2::SkipObjectAny(Long64_t, UInt_t) +{ +} + //////////////////////////////////////////////////////////////////////////////// /// Write object to buffer. Only used from TBuffer diff --git a/io/xml/inc/TBufferXML.h b/io/xml/inc/TBufferXML.h index 4210e1eb159dc..319b0aa2e454e 100644 --- a/io/xml/inc/TBufferXML.h +++ b/io/xml/inc/TBufferXML.h @@ -77,6 +77,7 @@ class TBufferXML final : public TBufferText, public TXMLSetup { void *ReadObjectAny(const TClass *clCast) final; void SkipObjectAny() final; + void SkipObjectAny(Long64_t start, UInt_t bytecount) final; void IncrementLevel(TVirtualStreamerInfo *) final; void SetStreamerElementNumber(TStreamerElement *elem, Int_t comp_type) final; diff --git a/io/xml/src/TBufferXML.cxx b/io/xml/src/TBufferXML.cxx index 6aafce756b643..fdfa96d3a2bcd 100644 --- a/io/xml/src/TBufferXML.cxx +++ b/io/xml/src/TBufferXML.cxx @@ -1510,6 +1510,15 @@ void TBufferXML::SkipObjectAny() ShiftStack("skipobjectany"); } +//////////////////////////////////////////////////////////////////////////////// +/// Skip any kind of object from buffer +/// Actually skip only one node on current level of xml structure + +void TBufferXML::SkipObjectAny(Long64_t, UInt_t) +{ + ShiftStack("skipobjectany"); +} + //////////////////////////////////////////////////////////////////////////////// /// Write object to buffer. Only used from TBuffer diff --git a/roofit/roofitcore/src/RooCategory.cxx b/roofit/roofitcore/src/RooCategory.cxx index 054f44d760e61..4b416087b426d 100644 --- a/roofit/roofitcore/src/RooCategory.cxx +++ b/roofit/roofitcore/src/RooCategory.cxx @@ -445,6 +445,7 @@ void RooCategory::Streamer(TBuffer &R__b) installLegacySharedProp(props); // props was allocated by I/O system, we cannot delete here in case it gets reused + R__b.CheckByteCount(R__s, R__c, RooCategory::IsA()); } else if (R__v == 2) { RooAbsCategoryLValue::Streamer(R__b); @@ -453,6 +454,7 @@ void RooCategory::Streamer(TBuffer &R__b) props->Streamer(R__b); installLegacySharedProp(props.get()); + R__b.CheckByteCount(R__s, R__c, RooCategory::IsA()); } else { // Starting at v3, ranges are shared using a shared pointer, which cannot be read by ROOT's I/O. // Instead, ranges are written as a normal pointer, and here we restore the sharing. @@ -461,8 +463,6 @@ void RooCategory::Streamer(TBuffer &R__b) _rangesPointerForIO = nullptr; } - R__b.CheckByteCount(R__s, R__c, RooCategory::IsA()); - } else { // Since we cannot write shared pointers yet, assign the shared ranges to a normal pointer, // write, and restore. diff --git a/roofit/roofitcore/src/RooDataHist.cxx b/roofit/roofitcore/src/RooDataHist.cxx index 73fd5b60e5e0b..6c0a8112c65af 100644 --- a/roofit/roofitcore/src/RooDataHist.cxx +++ b/roofit/roofitcore/src/RooDataHist.cxx @@ -2352,7 +2352,6 @@ void RooDataHist::Streamer(TBuffer &R__b) { if (R__v > 2) { R__b.ReadClassBuffer(RooDataHist::Class(),this,R__v,R__s,R__c); - R__b.CheckByteCount(R__s, R__c, RooDataHist::IsA()); initialize(nullptr, false); } else { diff --git a/roottest/CMakeLists.txt b/roottest/CMakeLists.txt index 7b290c389767d..427bb0e311fc7 100644 --- a/roottest/CMakeLists.txt +++ b/roottest/CMakeLists.txt @@ -29,10 +29,10 @@ if(MSVC) execute_process(COMMAND ${ROOT_CONFIG_EXECUTABLE} "--tutdir" OUTPUT_VARIABLE ROOT_TUTORIALS_DIR OUTPUT_STRIP_TRAILING_WHITESPACE) cmake_path(CONVERT "${ROOT_TUTORIALS_DIR}" TO_CMAKE_PATH_LIST ROOT_TUTORIALS_DIR) set(ROOT_root_CMD ${ROOTSYS}/bin/root.exe) - set(ROOT_hadd_CMD ${ROOTSYS}/bin/hadd.exe) - set(ROOT_genreflex_CMD ${ROOTSYS}/bin/genreflex.exe) - set(ROOT_rootcint_CMD ${ROOTSYS}/bin/rootcint.exe) - set(ROOT_rootcling_CMD ${ROOTSYS}/bin/rootcling.exe) + set(ROOT_hadd_CMD ${ROOTSYS}/bin/hadd.exe CACHE INTERNAL "ROOTTEST variable for hadd executable") + set(ROOT_genreflex_CMD ${ROOTSYS}/bin/genreflex.exe CACHE INTERNAL "ROOTTEST variable for genreflex executable") + set(ROOT_rootcint_CMD ${ROOTSYS}/bin/rootcint.exe CACHE INTERNAL "ROOTTEST variable for rootcint executable") + set(ROOT_rootcling_CMD ${ROOTSYS}/bin/rootcling.exe CACHE INTERNAL "ROOTTEST variable for rootcling executable") if(CMAKE_GENERATOR MATCHES Ninja) set(ROOT_LIBRARIES Core RIO Net Hist Gpad Graf Tree Rint Matrix MathCore) else() @@ -59,10 +59,10 @@ else() execute_process(COMMAND ${ROOT_CONFIG_EXECUTABLE} "--tutdir" OUTPUT_VARIABLE ROOT_TUTORIALS_DIR OUTPUT_STRIP_TRAILING_WHITESPACE) set(ROOT_LIBRARIES Core RIO Net Hist Gpad Tree Rint Matrix MathCore) set(ROOT_root_CMD ${ROOTSYS}/bin/root.exe) - set(ROOT_hadd_CMD ${ROOTSYS}/bin/hadd) - set(ROOT_genreflex_CMD ${ROOTSYS}/bin/genreflex) - set(ROOT_rootcint_CMD ${ROOTSYS}/bin/rootcint) - set(ROOT_rootcling_CMD rootcling) + set(ROOT_hadd_CMD ${ROOTSYS}/bin/hadd CACHE INTERNAL "ROOTTEST variable for hadd executable") + set(ROOT_genreflex_CMD ${ROOTSYS}/bin/genreflex CACHE INTERNAL "ROOTTEST variable for genreflex executable") + set(ROOT_rootcint_CMD ${ROOTSYS}/bin/rootcint CACHE INTERNAL "ROOTTEST variable for rootcint executable") + set(ROOT_rootcling_CMD rootcling CACHE INTERNAL "ROOTTEST variable for rootcling executable") endif() get_filename_component(ROOT_LIBRARY_DIR "${ROOTSYS}/lib" ABSOLUTE) diff --git a/roottest/cling/dict/ROOT-8096/CMakeLists.txt b/roottest/cling/dict/ROOT-8096/CMakeLists.txt index 07c916ce01b19..e86dfb94915d3 100644 --- a/roottest/cling/dict/ROOT-8096/CMakeLists.txt +++ b/roottest/cling/dict/ROOT-8096/CMakeLists.txt @@ -20,9 +20,9 @@ add_test(NAME roottest-cling-dict-ROOT-8096-build -- ${always-make}) # --target ${targetname_libgen}${fast}) set_property(TEST roottest-cling-dict-ROOT-8096-build PROPERTY ENVIRONMENT ${ROOTTEST_ENVIRONMENT}) -if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST roottest-cling-dict-ROOT-8096-build APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST roottest-cling-dict-ROOT-8096-build APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) +if(GeneratorNeedsBuildSerialization) + set_property(TEST roottest-cling-dict-ROOT-8096-build APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST roottest-cling-dict-ROOT-8096-build APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja) diff --git a/roottest/cling/stl/dicts/CMakeLists.txt b/roottest/cling/stl/dicts/CMakeLists.txt index c31d0a3ee25b3..58eb53d5d9839 100644 --- a/roottest/cling/stl/dicts/CMakeLists.txt +++ b/roottest/cling/stl/dicts/CMakeLists.txt @@ -11,7 +11,7 @@ ROOTTEST_LINKER_LIBRARY(stldictTest TEST MyClass1.cpp MyClass2.cpp MyClass3.cpp # of targets. Doing so right now would build the dictionaries twice. ROOT_ADD_TEST(roottest-cling-stl-dicts-build COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} ${build_config} --target stldictTest${fast} -- ${always-make}) -if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST roottest-cling-stl-dicts-build APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST roottest-cling-stl-dicts-build APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) +if(GeneratorNeedsBuildSerialization) + set_property(TEST roottest-cling-stl-dicts-build APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST roottest-cling-stl-dicts-build APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() diff --git a/roottest/root/io/rootcint/sigbug/CMakeLists.txt b/roottest/root/io/rootcint/sigbug/CMakeLists.txt index a87613d25dbe6..29643d57e2638 100644 --- a/roottest/root/io/rootcint/sigbug/CMakeLists.txt +++ b/roottest/root/io/rootcint/sigbug/CMakeLists.txt @@ -19,7 +19,7 @@ add_test(NAME ${GENERATE_DICTIONARY_TEST} ${build_config} --target ${dictname}${fast} -- ${always-make}) -if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) +if(GeneratorNeedsBuildSerialization) + set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST ${GENERATE_DICTIONARY_TEST} APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() diff --git a/roottest/root/io/tmpifile/CMakeLists.txt b/roottest/root/io/tmpifile/CMakeLists.txt index 840f475bcba53..d8ce592226b0a 100644 --- a/roottest/root/io/tmpifile/CMakeLists.txt +++ b/roottest/root/io/tmpifile/CMakeLists.txt @@ -13,9 +13,9 @@ ROOTTEST_ADD_TEST(split-fail ROOTTEST_ADD_TEST(libjetevent-build COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} ${build_config} --target JetEvent${fast} -- ${always-make}) -if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST roottest-root-io-tmpifile-libjetevent-build APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST roottest-root-io-tmpifile-libjetevent-build APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) +if(GeneratorNeedsBuildSerialization) + set_property(TEST roottest-root-io-tmpifile-libjetevent-build APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST roottest-root-io-tmpifile-libjetevent-build APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() ROOTTEST_ADD_TEST(sync-rate diff --git a/roottest/root/io/transient/base/CMakeLists.txt b/roottest/root/io/transient/base/CMakeLists.txt index c59bacde1dfa2..820907cd30af4 100644 --- a/roottest/root/io/transient/base/CMakeLists.txt +++ b/roottest/root/io/transient/base/CMakeLists.txt @@ -10,9 +10,9 @@ add_test(NAME roottest-root-io-transient-base-build --target base${fast} -- ${always-make}) set_property(TEST roottest-root-io-transient-base-build PROPERTY FIXTURES_SETUP root-io-transient-base-build) -if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST roottest-root-io-transient-base-build APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST roottest-root-io-transient-base-build APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) +if(GeneratorNeedsBuildSerialization) + set_property(TEST roottest-root-io-transient-base-build APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST roottest-root-io-transient-base-build APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() ROOTTEST_ADD_TEST(WriteFile diff --git a/roottest/root/meta/genreflex/ROOT-5768/CMakeLists.txt b/roottest/root/meta/genreflex/ROOT-5768/CMakeLists.txt index 18ffb14556114..26027743a4683 100644 --- a/roottest/root/meta/genreflex/ROOT-5768/CMakeLists.txt +++ b/roottest/root/meta/genreflex/ROOT-5768/CMakeLists.txt @@ -27,8 +27,8 @@ if(NOT MSVC OR win_broken_tests) ROOTTEST_ADD_TEST(PyCoolLib-build COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} ${build_config} --target PyCoolLib${fast} -- ${always-make} FIXTURES_REQUIRED PyCool-reflex-dict) - if(CMAKE_GENERATOR MATCHES Ninja) - set_property(TEST roottest-root-meta-genreflex-ROOT-5768-PyCoolLib-build APPEND PROPERTY RESOURCE_LOCK NINJA_BUILD) - set_property(TEST roottest-root-meta-genreflex-ROOT-5768-PyCoolLib-build APPEND PROPERTY FIXTURES_REQUIRED NINJA_BUILD_ALL) + if(GeneratorNeedsBuildSerialization) + set_property(TEST roottest-root-meta-genreflex-ROOT-5768-PyCoolLib-build APPEND PROPERTY RESOURCE_LOCK CMAKE_BUILD) + set_property(TEST roottest-root-meta-genreflex-ROOT-5768-PyCoolLib-build APPEND PROPERTY FIXTURES_REQUIRED CMAKE_BUILD_ALL) endif() endif() diff --git a/tree/tree/src/TBranch.cxx b/tree/tree/src/TBranch.cxx index 768434017ea86..e0cfeb01c36b2 100644 --- a/tree/tree/src/TBranch.cxx +++ b/tree/tree/src/TBranch.cxx @@ -3096,10 +3096,8 @@ void TBranch::Streamer(TBuffer& b) fBasketSeek [fWriteBasket] = fBasketSeek [fWriteBasket-1]; } - // Check Byte Count is not needed since it was done in ReadBuffer if (!fSplitLevel && fBranches.GetEntriesFast()) fSplitLevel = 1; gROOT->SetReadingObject(false); - b.CheckByteCount(R__s, R__c, TBranch::IsA()); if (IsA() == TBranch::Class()) { if (fNleaves == 0) { fReadLeaves = &TBranch::ReadLeaves0Impl;