io: Correct handling of byte count pos in TBufferFile::ReadObjectAny

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()

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 $<TARGET_FILE:root.exe>)
+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)

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;

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<TArray *>(a)->Streamer(b);
 
-      // Write byte count
-      b.SetByteCount(cntpos);
+      // The byte count is written automatically by the ByteCountWriter destructor
    }
 }
 

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);
    }

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<chunk_number, local_offset> if we implement chunked keys
-   using ByteCountStack_t = std::vector<ByteCountLocator_t>;
+   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<ByteCountLocationInfo>;
    ByteCountStack_t fByteCountStack; ///<! Stack to keep track of byte count storage positions
 
    using ByteCount_t = std::uint64_t; ///< Type used to store byte count values, can be changed to uint32_t if we implement chunked keys
@@ -72,7 +82,7 @@ class TBufferFile : public TBufferIO {
    Long64_t CheckByteCount(ULong64_t startpos, ULong64_t bcnt, const TClass *clss, const char* classname);
    void     CheckCount(UInt_t offset) override;
    UInt_t   CheckObject(UInt_t offset, const TClass *cl, Bool_t readClass = kFALSE);
-   UInt_t   ReserveByteCount();
+   UInt_t   ReserveByteCount(const TClass *cl) override;
 
    void  WriteObjectClass(const void *actualObjStart, const TClass *actualClass, Bool_t cacheReuse) override;
 
@@ -97,6 +107,7 @@ class TBufferFile : public TBufferIO {
 
    void      *ReadObjectAny(const TClass* cast) override;
    void       SkipObjectAny() override;
+   void       SkipObjectAny(Long64_t start, UInt_t bytecount) override;
 
    void       IncrementLevel(TVirtualStreamerInfo* info) override;
    void       SetStreamerElementNumber(TStreamerElement*,Int_t) override {}

io/io/inc/TBufferJSON.h

@@ -109,6 +109,7 @@ class TBufferJSON final : public TBufferText {
 
    void *ReadObjectAny(const TClass *clCast) final;
    void SkipObjectAny() final;
+   void SkipObjectAny(Long64_t start, UInt_t bytecount) final;
 
    // these methods used in streamer info to indicate currently streamed element,
    void IncrementLevel(TVirtualStreamerInfo *) final;

io/io/inc/TBufferText.h

@@ -23,6 +23,8 @@ class TBufferText : public TBufferIO {
    TBufferText();
    TBufferText(TBuffer::EMode mode, TObject *parent = nullptr);
 
+   UInt_t ReserveByteCount(const TClass *) override { return 0; }
+
 public:
    ~TBufferText() override;