From 20fd012adb231c7a2bb8402afe21fd659e1d41fb Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Tue, 5 Dec 2023 16:31:58 +1100 Subject: [PATCH] Valgrind: suppress warnings with MemPool & MEM_* trailing padding Suppress false positive Valgrind warnings which flooded the output. - BLI_mempool alloc/free & iteration. - Set alignment padding bytes at the end of MEM_* allocations as "defined" since this causes many false positive warnings in blend file writing and MEMFILE comparisons. - Set MEM_* allocations as undefined when `--debug-memory` is passed in to account for debug initialization. - Initialize pad bytes in TextLine allocations. --- intern/guardedalloc/CMakeLists.txt | 4 + .../intern/mallocn_guarded_impl.c | 41 +++++++++- .../intern/mallocn_lockfree_impl.c | 41 +++++++++- source/blender/blenkernel/intern/text.cc | 31 ++++---- source/blender/blenlib/intern/BLI_mempool.c | 74 +++++++++++++++++-- 5 files changed, 164 insertions(+), 27 deletions(-) diff --git a/intern/guardedalloc/CMakeLists.txt b/intern/guardedalloc/CMakeLists.txt index 906203ddb89..81abc5a1092 100644 --- a/intern/guardedalloc/CMakeLists.txt +++ b/intern/guardedalloc/CMakeLists.txt @@ -6,6 +6,10 @@ if(HAVE_MALLOC_STATS_H) add_definitions(-DHAVE_MALLOC_STATS_H) endif() +if(WITH_MEM_VALGRIND) + add_definitions(-DWITH_MEM_VALGRIND) +endif() + set(INC PUBLIC . ) diff --git a/intern/guardedalloc/intern/mallocn_guarded_impl.c b/intern/guardedalloc/intern/mallocn_guarded_impl.c index e216af615a9..c4cc1dd69c5 100644 --- a/intern/guardedalloc/intern/mallocn_guarded_impl.c +++ b/intern/guardedalloc/intern/mallocn_guarded_impl.c @@ -19,6 +19,12 @@ #include "MEM_guardedalloc.h" +/* Quiet warnings when dealing with allocated data written into the blend file. + * This also rounds up and causes warnings which we don't consider bugs in practice. */ +#ifdef WITH_MEM_VALGRIND +# include "valgrind/memcheck.h" +#endif + /* to ensure strict conversions */ #include "../../source/blender/blenlib/BLI_strict_flags.h" @@ -445,14 +451,28 @@ void *MEM_guarded_mallocN(size_t len, const char *str) { MemHead *memh; +#ifdef WITH_MEM_VALGRIND + const size_t len_unaligned = len; +#endif len = SIZET_ALIGN_4(len); memh = (MemHead *)malloc(len + sizeof(MemHead) + sizeof(MemTail)); if (LIKELY(memh)) { make_memhead_header(memh, len, str); - if (UNLIKELY(malloc_debug_memset && len)) { - memset(memh + 1, 255, len); + + if (LIKELY(len)) { + if (UNLIKELY(malloc_debug_memset)) { + memset(memh + 1, 255, len); + } +#ifdef WITH_MEM_VALGRIND + if (malloc_debug_memset) { + VALGRIND_MAKE_MEM_UNDEFINED(memh + 1, len_unaligned); + } + else { + VALGRIND_MAKE_MEM_DEFINED((const char *)(memh + 1) + len_unaligned, len - len_unaligned); + } +#endif /* WITH_MEM_VALGRIND */ } #ifdef DEBUG_MEMCOUNTER @@ -510,6 +530,9 @@ void *MEM_guarded_mallocN_aligned(size_t len, size_t alignment, const char *str) */ assert(alignment < 1024); +#ifdef WITH_MEM_VALGRIND + const size_t len_unaligned = len; +#endif len = SIZET_ALIGN_4(len); MemHead *memh = (MemHead *)aligned_malloc( @@ -524,8 +547,18 @@ void *MEM_guarded_mallocN_aligned(size_t len, size_t alignment, const char *str) make_memhead_header(memh, len, str); memh->alignment = (short)alignment; - if (UNLIKELY(malloc_debug_memset && len)) { - memset(memh + 1, 255, len); + if (LIKELY(len)) { + if (UNLIKELY(malloc_debug_memset)) { + memset(memh + 1, 255, len); + } +#ifdef WITH_MEM_VALGRIND + if (malloc_debug_memset) { + VALGRIND_MAKE_MEM_UNDEFINED(memh + 1, len_unaligned); + } + else { + VALGRIND_MAKE_MEM_DEFINED((const char *)(memh + 1) + len_unaligned, len - len_unaligned); + } +#endif /* WITH_MEM_VALGRIND */ } #ifdef DEBUG_MEMCOUNTER diff --git a/intern/guardedalloc/intern/mallocn_lockfree_impl.c b/intern/guardedalloc/intern/mallocn_lockfree_impl.c index 07d43425113..3191480b1f7 100644 --- a/intern/guardedalloc/intern/mallocn_lockfree_impl.c +++ b/intern/guardedalloc/intern/mallocn_lockfree_impl.c @@ -16,6 +16,12 @@ #include "MEM_guardedalloc.h" +/* Quiet warnings when dealing with allocated data written into the blend file. + * This also rounds up and causes warnings which we don't consider bugs in practice. */ +#ifdef WITH_MEM_VALGRIND +# include "valgrind/memcheck.h" +#endif + /* to ensure strict conversions */ #include "../../source/blender/blenlib/BLI_strict_flags.h" @@ -244,13 +250,27 @@ void *MEM_lockfree_mallocN(size_t len, const char *str) { MemHead *memh; +#ifdef WITH_MEM_VALGRIND + const size_t len_unaligned = len; +#endif len = SIZET_ALIGN_4(len); memh = (MemHead *)malloc(len + sizeof(MemHead)); if (LIKELY(memh)) { - if (UNLIKELY(malloc_debug_memset && len)) { - memset(memh + 1, 255, len); + + if (LIKELY(len)) { + if (UNLIKELY(malloc_debug_memset)) { + memset(memh + 1, 255, len); + } +#ifdef WITH_MEM_VALGRIND + if (malloc_debug_memset) { + VALGRIND_MAKE_MEM_UNDEFINED(memh + 1, len_unaligned); + } + else { + VALGRIND_MAKE_MEM_DEFINED((const char *)(memh + 1) + len_unaligned, len - len_unaligned); + } +#endif /* WITH_MEM_VALGRIND */ } memh->len = len; @@ -305,6 +325,9 @@ void *MEM_lockfree_mallocN_aligned(size_t len, size_t alignment, const char *str */ size_t extra_padding = MEMHEAD_ALIGN_PADDING(alignment); +#ifdef WITH_MEM_VALGRIND + const size_t len_unaligned = len; +#endif len = SIZET_ALIGN_4(len); MemHeadAligned *memh = (MemHeadAligned *)aligned_malloc( @@ -317,8 +340,18 @@ void *MEM_lockfree_mallocN_aligned(size_t len, size_t alignment, const char *str */ memh = (MemHeadAligned *)((char *)memh + extra_padding); - if (UNLIKELY(malloc_debug_memset && len)) { - memset(memh + 1, 255, len); + if (LIKELY(len)) { + if (UNLIKELY(malloc_debug_memset)) { + memset(memh + 1, 255, len); + } +#ifdef WITH_MEM_VALGRIND + if (malloc_debug_memset) { + VALGRIND_MAKE_MEM_UNDEFINED(memh + 1, len_unaligned); + } + else { + VALGRIND_MAKE_MEM_DEFINED((const char *)(memh + 1) + len_unaligned, len - len_unaligned); + } +#endif /* WITH_MEM_VALGRIND */ } memh->len = len | (size_t)MEMHEAD_ALIGN_FLAG; diff --git a/source/blender/blenkernel/intern/text.cc b/source/blender/blenkernel/intern/text.cc index 454f75c8968..2e37e971808 100644 --- a/source/blender/blenkernel/intern/text.cc +++ b/source/blender/blenkernel/intern/text.cc @@ -57,6 +57,8 @@ static void txt_delete_line(Text *text, TextLine *line); static void txt_delete_sel(Text *text); static void txt_make_dirty(Text *text); +static TextLine *txt_line_malloc() ATTR_MALLOC ATTR_WARN_UNUSED_RESULT; + /** \} */ /* -------------------------------------------------------------------- */ @@ -66,7 +68,6 @@ static void txt_make_dirty(Text *text); static void text_init_data(ID *id) { Text *text = (Text *)id; - TextLine *tmp; BLI_assert(MEMCMP_STRUCT_AFTER_IS_ZERO(text, id)); @@ -79,7 +80,7 @@ static void text_init_data(ID *id) BLI_listbase_clear(&text->lines); - tmp = (TextLine *)MEM_mallocN(sizeof(TextLine), "textline"); + TextLine *tmp = txt_line_malloc(); tmp->line = (char *)MEM_mallocN(1, "textline_string"); tmp->format = nullptr; @@ -125,7 +126,7 @@ static void text_copy_data(Main * /*bmain*/, ID *id_dst, const ID *id_src, const /* Walk down, reconstructing. */ LISTBASE_FOREACH (TextLine *, line_src, &text_src->lines) { - TextLine *line_dst = static_cast(MEM_mallocN(sizeof(*line_dst), __func__)); + TextLine *line_dst = txt_line_malloc(); line_dst->line = BLI_strdupn(line_src->line, line_src->len); line_dst->len = line_src->len; @@ -362,9 +363,7 @@ static void text_from_buf(Text *text, const uchar *buffer, const int len) lines_count = 0; for (i = 0; i < len; i++) { if (buffer[i] == '\n') { - TextLine *tmp; - - tmp = (TextLine *)MEM_mallocN(sizeof(TextLine), "textline"); + TextLine *tmp = txt_line_malloc(); tmp->line = (char *)MEM_mallocN(llen + 1, "textline_string"); tmp->format = nullptr; @@ -392,9 +391,7 @@ static void text_from_buf(Text *text, const uchar *buffer, const int len) * - last character in buffer is \n. in this case new line is needed to * deal with newline at end of file. (see #28087) (sergey) */ if (llen != 0 || lines_count == 0 || buffer[len - 1] == '\n') { - TextLine *tmp; - - tmp = (TextLine *)MEM_mallocN(sizeof(TextLine), "textline"); + TextLine *tmp = txt_line_malloc(); tmp->line = (char *)MEM_mallocN(llen + 1, "textline_string"); tmp->format = nullptr; @@ -591,6 +588,14 @@ void BKE_text_file_modified_ignore(Text *text) /** \name Editing Utility Functions * \{ */ +static TextLine *txt_line_malloc() +{ + TextLine *l = static_cast(MEM_mallocN(sizeof(TextLine), "textline")); + /* Quiet VALGRIND warning, may avoid unintended differences with MEMFILE undo as well. */ + memset(l->_pad0, 0, sizeof(l->_pad0)); + return l; +} + static void make_new_line(TextLine *line, char *newline) { if (line->line) { @@ -607,9 +612,7 @@ static void make_new_line(TextLine *line, char *newline) static TextLine *txt_new_linen(const char *str, int str_len) { - TextLine *tmp; - - tmp = (TextLine *)MEM_mallocN(sizeof(TextLine), "textline"); + TextLine *tmp = txt_line_malloc(); tmp->line = static_cast(MEM_mallocN(str_len + 1, "textline_string")); tmp->format = nullptr; @@ -1406,7 +1409,7 @@ void txt_from_buf_for_undo(Text *text, const char *buf, size_t buf_len) const char *buf_step_next = strchr(buf_step, '\n'); const int len = buf_step_next - buf_step; - TextLine *l = static_cast(MEM_mallocN(sizeof(TextLine), "textline")); + TextLine *l = txt_line_malloc(); l->line = static_cast(MEM_mallocN(len + 1, "textline_string")); l->len = len; l->format = nullptr; @@ -1681,7 +1684,7 @@ void txt_split_curline(Text *text) /* Make the new TextLine */ - ins = static_cast(MEM_mallocN(sizeof(TextLine), "textline")); + ins = txt_line_malloc(); ins->line = left; ins->format = nullptr; ins->len = text->curc; diff --git a/source/blender/blenlib/intern/BLI_mempool.c b/source/blender/blenlib/intern/BLI_mempool.c index f673ade25a9..cf42866a450 100644 --- a/source/blender/blenlib/intern/BLI_mempool.c +++ b/source/blender/blenlib/intern/BLI_mempool.c @@ -242,12 +242,16 @@ static BLI_freenode *mempool_chunk_add(BLI_mempool *pool, BLI_freenode *next; BLI_asan_unpoison(curnode, pool->esize - POISON_REDZONE_SIZE); - +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(curnode, pool->esize - POISON_REDZONE_SIZE); +#endif curnode->next = next = NODE_STEP_NEXT(curnode); curnode->freeword = FREEWORD; BLI_asan_poison(curnode, pool->esize); - +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(curnode, pool->esize); +#endif curnode = next; } } @@ -256,8 +260,14 @@ static BLI_freenode *mempool_chunk_add(BLI_mempool *pool, BLI_freenode *next; BLI_asan_unpoison(curnode, pool->esize - POISON_REDZONE_SIZE); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(curnode, pool->esize - POISON_REDZONE_SIZE); +#endif curnode->next = next = NODE_STEP_NEXT(curnode); BLI_asan_poison(curnode, pool->esize); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(curnode, pool->esize); +#endif curnode = next; } @@ -266,14 +276,27 @@ static BLI_freenode *mempool_chunk_add(BLI_mempool *pool, /* terminate the list (rewind one) * will be overwritten if 'curnode' gets passed in again as 'last_tail' */ - BLI_asan_unpoison(curnode, pool->esize - POISON_REDZONE_SIZE); - BLI_asan_poison(curnode, pool->esize); + if (POISON_REDZONE_SIZE > 0) { + BLI_asan_unpoison(curnode, pool->esize - POISON_REDZONE_SIZE); + BLI_asan_poison(curnode, pool->esize); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(curnode, pool->esize - POISON_REDZONE_SIZE); + VALGRIND_MAKE_MEM_UNDEFINED(curnode, pool->esize); +#endif + } curnode = NODE_STEP_PREV(curnode); BLI_asan_unpoison(curnode, pool->esize - POISON_REDZONE_SIZE); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(curnode, pool->esize - POISON_REDZONE_SIZE); +#endif + curnode->next = NULL; BLI_asan_poison(curnode, pool->esize); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(curnode, pool->esize); +#endif #ifdef USE_TOTALLOC pool->totalloc += pool->pchunk; @@ -282,8 +305,14 @@ static BLI_freenode *mempool_chunk_add(BLI_mempool *pool, /* final pointer in the previously allocated chunk is wrong */ if (last_tail) { BLI_asan_unpoison(last_tail, pool->esize - POISON_REDZONE_SIZE); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(last_tail, pool->esize - POISON_REDZONE_SIZE); +#endif last_tail->next = CHUNK_DATA(mpchunk); BLI_asan_poison(last_tail, pool->esize); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(last_tail, pool->esize); +#endif } return curnode; @@ -295,6 +324,9 @@ static void mempool_chunk_free(BLI_mempool_chunk *mpchunk, BLI_mempool *pool) BLI_asan_unpoison(mpchunk, sizeof(BLI_mempool_chunk) + pool->esize * pool->csize); #else UNUSED_VARS(pool); +#endif +#ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(mpchunk, sizeof(BLI_mempool_chunk) + pool->esize * pool->csize); #endif MEM_freeN(mpchunk); } @@ -394,6 +426,17 @@ void *BLI_mempool_alloc(BLI_mempool *pool) free_pop = pool->free; BLI_asan_unpoison(free_pop, pool->esize - POISON_REDZONE_SIZE); +#ifdef WITH_MEM_VALGRIND + VALGRIND_MEMPOOL_ALLOC(pool, free_pop, pool->esize - POISON_REDZONE_SIZE); + /* Mark as define, then undefine immediately before returning so: + * - `free_pop->next` can be read without reading "undefined" memory. + * - `freeword` can be set without causing the memory to be considered "defined". + * + * These could be handled on a more granular level - dealing with defining & underlining these + * members explicitly but that requires more involved calls, + * adding overhead for no real benefit. */ + VALGRIND_MAKE_MEM_DEFINED(free_pop, pool->esize - POISON_REDZONE_SIZE); +#endif BLI_assert(pool->chunk_tail->next == NULL); @@ -405,7 +448,7 @@ void *BLI_mempool_alloc(BLI_mempool *pool) pool->totused++; #ifdef WITH_MEM_VALGRIND - VALGRIND_MEMPOOL_ALLOC(pool, free_pop, pool->esize); + VALGRIND_MAKE_MEM_UNDEFINED(free_pop, pool->esize - POISON_REDZONE_SIZE); #endif return (void *)free_pop; @@ -687,6 +730,9 @@ void *BLI_mempool_iterstep(BLI_mempool_iter *iter) ret = curnode; BLI_asan_unpoison(ret, iter->pool->esize - POISON_REDZONE_SIZE); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(ret, iter->pool->esize - POISON_REDZONE_SIZE); +# endif if (++iter->curindex != iter->pool->pchunk) { curnode = POINTER_OFFSET(curnode, esize); @@ -696,10 +742,16 @@ void *BLI_mempool_iterstep(BLI_mempool_iter *iter) iter->curchunk = iter->curchunk->next; if (UNLIKELY(iter->curchunk == NULL)) { BLI_asan_unpoison(ret, iter->pool->esize - POISON_REDZONE_SIZE); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(ret, iter->pool->esize - POISON_REDZONE_SIZE); +# endif void *ret2 = (ret->freeword == FREEWORD) ? NULL : ret; if (ret->freeword == FREEWORD) { BLI_asan_poison(ret, iter->pool->esize); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(ret, iter->pool->esize); +# endif } return ret2; @@ -727,6 +779,9 @@ void *mempool_iter_threadsafe_step(BLI_mempool_threadsafe_iter *ts_iter) ret = curnode; BLI_asan_unpoison(ret, esize - POISON_REDZONE_SIZE); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_DEFINED(ret, iter->pool->esize); +# endif if (++iter->curindex != iter->pool->pchunk) { curnode = POINTER_OFFSET(curnode, esize); @@ -746,6 +801,9 @@ void *mempool_iter_threadsafe_step(BLI_mempool_threadsafe_iter *ts_iter) if (UNLIKELY(iter->curchunk == NULL)) { if (ret->freeword == FREEWORD) { BLI_asan_poison(ret, esize); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(ret, iter->pool->esize); +# endif mempool_asan_unlock(iter->pool); return NULL; } @@ -760,6 +818,9 @@ void *mempool_iter_threadsafe_step(BLI_mempool_threadsafe_iter *ts_iter) if (UNLIKELY(iter->curchunk == NULL)) { if (ret->freeword == FREEWORD) { BLI_asan_poison(ret, iter->pool->esize); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(ret, iter->pool->esize); +# endif mempool_asan_unlock(iter->pool); return NULL; } @@ -774,6 +835,9 @@ void *mempool_iter_threadsafe_step(BLI_mempool_threadsafe_iter *ts_iter) if (ret->freeword == FREEWORD) { BLI_asan_poison(ret, iter->pool->esize); +# ifdef WITH_MEM_VALGRIND + VALGRIND_MAKE_MEM_UNDEFINED(ret, iter->pool->esize); +# endif } else { break;