From d5ecc717770ecefc2775ed6990ed0b2e6037171b Mon Sep 17 00:00:00 2001
From: Philippe Canal <pcanal@fnal.gov>
Date: Tue, 7 May 2019 17:27:43 -0500
Subject: [PATCH] Suppress valgrind warning about fUniqueID unitialized memory
used.
This is done by taking the check of fUniqueID and the assignment to fBuits
into a separate function and by including this function in the header
*but* marking it as "noinline" to prevent its inlining and preserve a
way to suppress the valgrind warning.
Introducing the macro R__NEVER_INLINE which expands to
inline __attribute__((noinline))
The performance degradation is less than 10% for a case doing
only creation and destruction of TObjects.
---
cmake/modules/RootConfiguration.cmake | 9 +++++++++
config/RConfigure.in | 1 +
core/base/inc/ROOT/RConfig.hxx | 12 ++++++++++++
core/base/inc/TObject.h | 7 ++-----
core/base/inc/TStorage.h | 27 +++++++++++++++++++++++++--
etc/valgrind-root.supp | 7 +++++++
6 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/cmake/modules/RootConfiguration.cmake b/cmake/modules/RootConfiguration.cmake
index a3de1aa1ef9..2f017d7d698 100644
--- a/cmake/modules/RootConfiguration.cmake
+++ b/cmake/modules/RootConfiguration.cmake
@@ -569,6 +569,15 @@ else()
set(has_found_attribute_always_inline undef)
endif()
+CHECK_CXX_SOURCE_COMPILES("
+inline __attribute__((noinline)) bool TestBit(unsigned long f) { return f != 0; };
+int main() { return TestBit(0); }" has_found_attribute_noinline)
+if(has_found_attribute_noinline)
+ set(has_found_attribute_noinline define)
+else()
+ set(has_found_attribute_noinline undef)
+endif()
+
#---root-config----------------------------------------------------------------------------------------------
ROOT_GET_OPTIONS(features ENABLED)
string(REPLACE "c++11" "cxx11" features ${features}) # change the name of the c++11 feature needed for root-config.in
diff --git a/config/RConfigure.in b/config/RConfigure.in
index ac73fc9ec4f..dc761c6ec0a 100644
--- a/config/RConfigure.in
+++ b/config/RConfigure.in
@@ -36,6 +36,7 @@
#@hasstdinvoke@ R__HAS_STD_INVOKE /**/
#@hasstdindexsequence@ R__HAS_STD_INDEX_SEQUENCE /**/
#@has_found_attribute_always_inline@ R__HAS_ATTRIBUTE_ALWAYS_INLINE /**/
+#@has_found_attribute_noinline@ R__HAS_ATTRIBUTE_NOINLINE /**/
#@hasllvm@ R__EXTERN_LLVMDIR @llvmdir@
#@useimt@ R__USE_IMT /**/
#@memory_term@ R__COMPLETE_MEM_TERMINATION /**/
diff --git a/core/base/inc/ROOT/RConfig.hxx b/core/base/inc/ROOT/RConfig.hxx
index 297ff5e01b7..fe8419353cf 100644
--- a/core/base/inc/ROOT/RConfig.hxx
+++ b/core/base/inc/ROOT/RConfig.hxx
@@ -571,6 +571,18 @@
#endif
#endif
+// See also https://nemequ.github.io/hedley/api-reference.html#HEDLEY_NEVER_INLINE
+// for other platforms.
+#ifdef R__HAS_ATTRIBUTE_NOINLINE
+#define R__NEVER_INLINE inline __attribute__((noinline))
+#else
+#if defined(_MSC_VER)
+#define R__NEVER_INLINE inline __declspec(noinline)
+#else
+#define R__NEVER_INLINE inline
+#endif
+#endif
+
/*---- unlikely / likely expressions -----------------------------------------*/
// These are meant to use in cases like:
// if (R__unlikely(expression)) { ... }
diff --git a/core/base/inc/TObject.h b/core/base/inc/TObject.h
index 15c4ccd265a..58c09609f69 100644
--- a/core/base/inc/TObject.h
+++ b/core/base/inc/TObject.h
@@ -227,7 +227,7 @@ inline TObject::TObject() : fBits(kNotDeleted) // Need to leave fUniqueID unset
{
// This will be reported by valgrind as uninitialized memory reads for
// object created on the stack, use $ROOTSYS/etc/valgrind-root.supp
- if (TStorage::FilledByObjectAlloc(&fUniqueID)) fBits |= kIsOnHeap;
+ TStorage::UpdateIsOnHeap(fUniqueID, fBits);
fUniqueID = 0;
@@ -247,10 +247,7 @@ inline TObject::TObject(const TObject &obj)
// This will be reported by valgrind as uninitialized memory reads for
// object created on the stack, use $ROOTSYS/etc/valgrind-root.supp
- if (TStorage::FilledByObjectAlloc(&fUniqueID))
- fBits |= kIsOnHeap;
- else
- fBits &= ~kIsOnHeap;
+ TStorage::UpdateIsOnHeap(fUniqueID, fBits);
fBits &= ~kIsReferenced;
fBits &= ~kCanDelete;
diff --git a/core/base/inc/TStorage.h b/core/base/inc/TStorage.h
index 13780cd6b27..c45ee031a5d 100644
--- a/core/base/inc/TStorage.h
+++ b/core/base/inc/TStorage.h
@@ -39,6 +39,12 @@ private:
static ReAllocFun_t fgReAllocHook; // custom ReAlloc
static ReAllocCFun_t fgReAllocCHook; // custom ReAlloc with length check
static Bool_t fgHasCustomNewDelete; // true if using ROOT's new/delete
+
+ //----- Private bits, clients can only test but not change them
+ enum {
+ kIsOnHeap = 0x01000000, ///< object is on heap
+ };
+
public:
static const UInt_t kObjectAllocMemValue = 0x99999999;
// magic number for ObjectAlloc
@@ -81,12 +87,13 @@ public:
static void AddToHeap(ULong_t begin, ULong_t end);
static Bool_t IsOnHeap(void *p);
- static Bool_t FilledByObjectAlloc(volatile UInt_t* member);
+ static Bool_t FilledByObjectAlloc(volatile const UInt_t* const member);
+ static void UpdateIsOnHeap(volatile const UInt_t &uniqueID, volatile UInt_t &bits);
ClassDef(TStorage,0) //Storage manager class
};
-inline Bool_t TStorage::FilledByObjectAlloc(volatile UInt_t *member) {
+inline Bool_t TStorage::FilledByObjectAlloc(volatile const UInt_t *const member) {
//called by TObject's constructor to determine if object was created by call to new
// This technique is necessary as there is one stack per thread
@@ -113,6 +120,22 @@ R__INTENTIONALLY_UNINIT_BEGIN
R__INTENTIONALLY_UNINIT_END
}
+// Assign the kIsOnHeap bit in 'bits' based on the pattern seen in uniqueID.
+// See Storage::FilledByObjectAlloc for details.
+// This routine is marked as inline with attribute noinline so that it never
+// inlined and thus can be used in a valgrind suppression file to suppress
+// the known/intentional uninitialized memory read but still be a 'quick'
+// function call to avoid losing performance at object creation.
+// Moving the function into the source file, results in doubling of the
+// overhead (compared to inlining)
+R__NEVER_INLINE void TStorage::UpdateIsOnHeap(volatile const UInt_t &uniqueID, volatile UInt_t &bits) {
+ if (TStorage::FilledByObjectAlloc(&uniqueID))
+ bits |= kIsOnHeap;
+ else
+ bits &= ~kIsOnHeap;
+}
+
+
inline size_t TStorage::GetMaxBlockSize() { return fgMaxBlockSize; }
inline void TStorage::SetMaxBlockSize(size_t size) { fgMaxBlockSize = size; }
diff --git a/etc/valgrind-root.supp b/etc/valgrind-root.supp
index 620970343d9..6ecf043be22 100644
--- a/etc/valgrind-root.supp
+++ b/etc/valgrind-root.supp
@@ -1064,6 +1064,13 @@
...
}
+{
+ <insert_a_suppression_name_here>
+ Memcheck:Cond
+ fun:_ZN8TStorage14UpdateIsOnHeapERVKjRVj
+ ...
+}
+
######## Minimizer
{
--
GitLab