From 9e318bb17fa15538f13d290557e5ee6dd6151b0d Mon Sep 17 00:00:00 2001 From: Julian Seward Date: Thu, 11 Mar 2010 13:43:18 +0000 Subject: [PATCH] If a race error is detected, check to see whether the raced-on address is inside a heap block, and if so, print the allocation point of the heap block. It's stupid not to do this considering that the implementation already keeps track of all mallocs and frees. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@11089 --- helgrind/hg_errors.c | 86 +++++++++++++++++++++++++++++++------------- helgrind/hg_errors.h | 8 +++++ helgrind/hg_main.c | 30 ++++++++++++++++ 3 files changed, 100 insertions(+), 24 deletions(-) diff --git a/helgrind/hg_errors.c b/helgrind/hg_errors.c index 981f4bf9ec..a4d0bdbae5 100644 --- a/helgrind/hg_errors.c +++ b/helgrind/hg_errors.c @@ -178,8 +178,15 @@ typedef Int szB; Bool isWrite; Thread* thr; + /* descr1/2 provide a description of stack/global locs */ XArray* descr1; /* XArray* of HChar */ XArray* descr2; /* XArray* of HChar */ + /* halloc/haddr/hszB describe the addr if it is a heap block. */ + ExeContext* hctxt; + Addr haddr; + SizeT hszB; + /* h1_* and h2_* provide some description of a previously + observed access with which we are conflicting. */ Thread* h1_ct; /* non-NULL means h1 info present */ ExeContext* h1_ct_mbsegstartEC; ExeContext* h1_ct_mbsegendEC; @@ -263,33 +270,47 @@ UInt HG_(update_extra) ( Error* err ) if (0) VG_(printf)("HG_(update_extra): " "%d conflicting-event queries\n", xxx); + + tl_assert(!xe->XE.Race.hctxt); tl_assert(!xe->XE.Race.descr1); tl_assert(!xe->XE.Race.descr2); - xe->XE.Race.descr1 - = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr1", - HG_(free), sizeof(HChar) ); - xe->XE.Race.descr2 - = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr2", - HG_(free), sizeof(HChar) ); - - (void) VG_(get_data_description)( xe->XE.Race.descr1, - xe->XE.Race.descr2, - xe->XE.Race.data_addr ); - - /* If there's nothing in descr1/2, free it. Why is it safe to - to VG_(indexXA) at zero here? Because - VG_(get_data_description) guarantees to zero terminate - descr1/2 regardless of the outcome of the call. So there's - always at least one element in each XA after the call. - */ - if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr1, 0 ))) { - VG_(deleteXA)( xe->XE.Race.descr1 ); - xe->XE.Race.descr1 = NULL; - } - if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr2, 0 ))) { - VG_(deleteXA)( xe->XE.Race.descr2 ); - xe->XE.Race.descr2 = NULL; + /* First, see if it's in any heap block. Unfortunately this + means a linear search through all allocated heap blocks. */ + HG_(mm_find_containing_block)( + &xe->XE.Race.hctxt, &xe->XE.Race.haddr, &xe->XE.Race.hszB, + xe->XE.Race.data_addr + ); + + if (!xe->XE.Race.hctxt) { + /* It's not in any heap block. See if we can map it to a + stack or global symbol. */ + + xe->XE.Race.descr1 + = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr1", + HG_(free), sizeof(HChar) ); + xe->XE.Race.descr2 + = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr2", + HG_(free), sizeof(HChar) ); + + (void) VG_(get_data_description)( xe->XE.Race.descr1, + xe->XE.Race.descr2, + xe->XE.Race.data_addr ); + + /* If there's nothing in descr1/2, free it. Why is it safe to + to VG_(indexXA) at zero here? Because + VG_(get_data_description) guarantees to zero terminate + descr1/2 regardless of the outcome of the call. So there's + always at least one element in each XA after the call. + */ + if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr1, 0 ))) { + VG_(deleteXA)( xe->XE.Race.descr1 ); + xe->XE.Race.descr1 = NULL; + } + if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr2, 0 ))) { + VG_(deleteXA)( xe->XE.Race.descr2 ); + xe->XE.Race.descr2 = NULL; + } } /* And poke around in the conflicting-event map, to see if we @@ -993,6 +1014,23 @@ void HG_(pp_Error) ( Error* err ) } + /* If we have a description of the address in terms of a heap + block, show it. */ + if (xe->XE.Race.hctxt) { + SizeT delta = err_ga - xe->XE.Race.haddr; + if (xml) { + emit(" Address %#lx is %ld bytes inside a block " + "of size %ld alloc'd\n", err_ga, delta, + xe->XE.Race.hszB); + VG_(pp_ExeContext)( xe->XE.Race.hctxt ); + } else { + emit(" Address %#lx is %ld bytes inside a block " + "of size %ld alloc'd\n", err_ga, delta, + xe->XE.Race.hszB); + VG_(pp_ExeContext)( xe->XE.Race.hctxt ); + } + } + /* If we have a better description of the address, show it. Note that in XML mode, it will already by nicely wrapped up in tags, either or , so we can just emit diff --git a/helgrind/hg_errors.h b/helgrind/hg_errors.h index ee49491853..359482579d 100644 --- a/helgrind/hg_errors.h +++ b/helgrind/hg_errors.h @@ -67,6 +67,14 @@ extern ULong HG_(stats__LockN_to_P_get_map_size) ( void ); extern ULong HG_(stats__string_table_queries); extern ULong HG_(stats__string_table_get_map_size) ( void ); +/* For error creation: map 'data_addr' to a malloc'd chunk, if any. + Slow linear search. This is an abuse of the normal file structure + since this is exported by hg_main.c, not hg_errors.c. Oh Well. */ +void HG_(mm_find_containing_block)( /*OUT*/ExeContext** where, + /*OUT*/Addr* payload, + /*OUT*/SizeT* szB, + Addr data_addr ); + #endif /* ! __HG_ERRORS_H */ /*--------------------------------------------------------------------*/ diff --git a/helgrind/hg_main.c b/helgrind/hg_main.c index 19dd1f968e..2f4e13b280 100644 --- a/helgrind/hg_main.c +++ b/helgrind/hg_main.c @@ -3868,6 +3868,36 @@ static SizeT hg_cli_malloc_usable_size ( ThreadId tid, void* p ) } +/* For error creation: map 'data_addr' to a malloc'd chunk, if any. + Slow linear search. */ + +static inline Bool addr_is_in_MM_Chunk( MallocMeta* mm, Addr a ) +{ + if (a < mm->payload) return False; + if (a >= mm->payload + mm->szB) return False; + return True; +} + +void HG_(mm_find_containing_block)( /*OUT*/ExeContext** where, + /*OUT*/Addr* payload, + /*OUT*/SizeT* szB, + Addr data_addr ) +{ + MallocMeta* mm; + /* Well, this totally sucks. But without using an interval tree or + some such, it's hard to see how to do better. */ + VG_(HT_ResetIter)(hg_mallocmeta_table); + while ( (mm = VG_(HT_Next)(hg_mallocmeta_table)) ) { + if (UNLIKELY(addr_is_in_MM_Chunk(mm, data_addr))) { + *where = mm->where; + *payload = mm->payload; + *szB = mm->szB; + return; + } + } +} + + /*--------------------------------------------------------------*/ /*--- Instrumentation ---*/ /*--------------------------------------------------------------*/ -- 2.47.2