|
From: Jeremy F. <je...@go...> - 2002-10-19 00:56:19
|
New patches for today, fresh from http://www.goop.org/~jeremy/valgrind/: 14-hg-tid HELGRIND: This fixes a bug in Helgrind in which all memory access by syscalls was being treated as if it were happening in thread 1. This is because the eraser_mem_read/write functions were using get_current_tid_1_if_root() to get the current tid. Unfortunately, during syscalls there is no current thread, so it was getting 1_if_root. This patch fixes this by using what thread ID information we're given, and only using get_current_tid() if we're recording a memory access performed by code (rather than by a syscall). 15-hg-datasym HELGRIND: In conjuction with patch 13-data-syms, print symbolic information for addresses in error messages (if possible). 16-ld-nodelete Add -Wl,-z,nodelete,-z,initfirst to link line for libpthread.so, because HJ says so. Also add soname. J |
|
From: Nicholas N. <nj...@ca...> - 2002-10-21 09:02:15
|
On 18 Oct 2002, Jeremy Fitzhardinge wrote: > 14-hg-tid > HELGRIND: This fixes a bug in Helgrind in which all memory access by > syscalls was being treated as if it were happening in thread 1. This is > because the eraser_mem_read/write functions were using > get_current_tid_1_if_root() to get the current tid. Unfortunately, > during syscalls there is no current thread, so it was getting 1_if_root. > This patch fixes this by using what thread ID information we're given, > and only using get_current_tid() if we're recording a memory access > performed by code (rather than by a syscall). Well done for spotting this. I suspected/hoped that there would be a single bug that accounted for a lot of the garbage output. Accept my apologies for my wretched coding in the first place :) N |
|
From: Jeremy F. <je...@go...> - 2002-10-21 15:53:10
|
On Mon, 2002-10-21 at 02:02, Nicholas Nethercote wrote:
On 18 Oct 2002, Jeremy Fitzhardinge wrote:
> 14-hg-tid
> HELGRIND: This fixes a bug in Helgrind in which all memory access by
> syscalls was being treated as if it were happening in thread 1. This is
> because the eraser_mem_read/write functions were using
> get_current_tid_1_if_root() to get the current tid. Unfortunately,
> during syscalls there is no current thread, so it was getting 1_if_root.
> This patch fixes this by using what thread ID information we're given,
> and only using get_current_tid() if we're recording a memory access
> performed by code (rather than by a syscall).
Well done for spotting this. I suspected/hoped that there would be a
single bug that accounted for a lot of the garbage output. Accept my
apologies for my wretched coding in the first place :)
It was pretty subtle, but easy to fix once found. I'm still wondering
what get_current_tid_1_if_root() is for. It seems that
get_current_tid() can either return a tid, or "no tid" in the case where
there's nothing loaded into the base block. But I don't really
understand the rationale for returning what seems like a random tid if
there's no correct answer. I was thinking of getting rid of it
altogether, but it is used in a couple of other places.
J
|
|
From: Nicholas N. <nj...@ca...> - 2002-10-21 19:57:14
|
On 21 Oct 2002, Jeremy Fitzhardinge wrote: > It was pretty subtle, but easy to fix once found. I'm still wondering > what get_current_tid_1_if_root() is for. It seems that > get_current_tid() can either return a tid, or "no tid" in the case where > there's nothing loaded into the base block. But I don't really > understand the rationale for returning what seems like a random tid if > there's no correct answer. I was thinking of getting rid of it > altogether, but it is used in a couple of other places. As I remember it, the root thread is #1, and if there's no tid in the base block then it must be the root thread. This could be wrong. N |
|
From: Jeremy F. <je...@go...> - 2002-10-21 21:05:07
|
On Mon, 2002-10-21 at 12:57, Nicholas Nethercote wrote: > As I remember it, the root thread is #1, and if there's no tid in the base > block then it must be the root thread. This could be wrong. Yes, I don't think that assumption is good. If there's no thread in the baseBlock, then there's no current (virtual machine) thread running at all. J |
|
From: Nicholas N. <nj...@ca...> - 2002-10-23 12:55:36
|
On 21 Oct 2002, Jeremy Fitzhardinge wrote: > > As I remember it, the root thread is #1, and if there's no tid in the base > > block then it must be the root thread. This could be wrong. > > Yes, I don't think that assumption is good. If there's no thread in the > baseBlock, then there's no current (virtual machine) thread running at > all. So then what do you do in that case? N |
|
From: Jeremy F. <je...@go...> - 2002-10-23 15:45:40
|
On Wed, 2002-10-23 at 05:55, Nicholas Nethercote wrote:
On 21 Oct 2002, Jeremy Fitzhardinge wrote:
> > As I remember it, the root thread is #1, and if there's no tid in the base
> > block then it must be the root thread. This could be wrong.
>
> Yes, I don't think that assumption is good. If there's no thread in the
> baseBlock, then there's no current (virtual machine) thread running at
> all.
So then what do you do in that case?
A hack. I removed VG_(get_current_tid_1_if_root)() and replaced it with
VG_(get_current_or_recent_tid)(). It returns either the current tid, or
if there is none, the most recently current tid. This works for the
cases where we're acting on behalf of the most recently running thread,
such as when we're doing things in syscalls. A more correct solution
might be to always pass around a ThreadState, but that would require
changing the interface of almost all the tracking functions.
J
|