|
From: Eyal L. <ey...@ey...> - 2003-03-14 08:19:33
|
My standard run takes about 15h, and in this period a single server deals with a few hundred clients. I need to know which valgrind error report belongs to which client, and the way to do it is to timestamp the reports. I added a line saying ==nnnn== Time: dddddd-hh:mm:ss to the start of the report and I find it very usefull. It was a worthy exercies as I found that I cannot simply access the c library (I am still to learn how to do this) but since I did have access to time() I displayed the rest using a simple function. Is there a standard was to request such timestamping? I would not mind learning how to properly add it to the package, with a proper --time-stap=yes option. Right now I find the most important bit (after correctness) to be the speedup of the main scheduler, most of the above 15 hours pass with the CPU at 98-100% idle. Is anyone working on this part currently? -- Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Nicholas N. <nj...@ca...> - 2003-03-14 08:37:32
|
On Fri, 14 Mar 2003, Eyal Lebedinsky wrote:
> My standard run takes about 15h, and in this period a single server
> deals with a few hundred clients. I need to know which valgrind
> error report belongs to which client, and the way to do it is to
> timestamp the reports.
>
> I added a line saying
>
> ==nnnn== Time: dddddd-hh:mm:ss
> Is there a standard was to request such timestamping? I would
> not mind learning how to properly add it to the package, with
> a proper --time-stap=yes option.
Just choose an existing option and copy the way it's done. For example,
search for "--gdb-attach" and "VG_(clo_GDB_attach)" in the source ("clo"
is short for "command line option).
> Right now I find the most important bit (after correctness) to
> be the speedup of the main scheduler, most of the above 15
> hours pass with the CPU at 98-100% idle. Is anyone working
> on this part currently?
Not really. Valgrind's threads implementation is not very fast, because
most programs don't really need it to be. And as simple as it is, it's
still one of the most complicated and problematic parts of Valgrind. So I
wouldn't hold your breath for improvements any time soon, sorry.
N
|
|
From: Eyal L. <ey...@ey...> - 2003-03-14 10:34:08
Attachments:
vg-1.9.4-e1.patch
|
Nicholas Nethercote wrote:
>
> On Fri, 14 Mar 2003, Eyal Lebedinsky wrote:
> > I added a line saying
> >
> > ==nnnn== Time: dddddd-hh:mm:ss
>
> > Is there a standard was to request such timestamping? I would
> > not mind learning how to properly add it to the package, with
> > a proper --time-stap=yes option.
>
> Just choose an existing option and copy the way it's done. For example,
> search for "--gdb-attach" and "VG_(clo_GDB_attach)" in the source ("clo"
> is short for "command line option).
OK, here is a humble patch that adds a new option
--time-stamp=[no|yes]
which yields the report below. I find that localtime() fails to account
for timezone when used in valgrind, if anyone can fix it please do so.
$ date ; valgrind --time-stamp=yes ls
Fri Mar 14 21:31:01 EST 2003
==18272== Memcheck, a.k.a. Valgrind, a memory error detector for
x86-linux.
==18272== Copyright (C) 2002, and GNU GPL'd, by Julian Seward.
==18272== Using valgrind-1.9.4, a program instrumentation system for
x86-linux.
==18272== Copyright (C) 2000-2002, and GNU GPL'd, by Julian Seward.
==18272== Estimated CPU clock rate is 1204 MHz
==18272== For more details, rerun with: -v
==18272==
==18272== Time: 2003/03/14 10:31:01
==18272== pthread_mutex_destroy: mutex is still in use
==18272== at 0x40352E70: pthread_error (vg_libpthread.c:288)
==18272== by 0x40353D3F: __pthread_mutex_destroy
(vg_libpthread.c:998)
==18272== by 0x402CE1CE: closedir (in /lib/libc-2.2.5.so)
==18272== by 0x804A90D: (within /bin/ls)
03.ndx ds1.bat odds setit.bat test-all.bat
build examples okreps ssacssv.err testall
build.bat ids.db prepids ssacssv.log testall.bat
chkunpdf ids.dbg prepids.bat ssarun.log testit.bat
chkunpdf.awk idscosv.err rds stopit.bat tests
chkunpdf.bat idscosv.log rds.awk stress.bat testx03g.lst
cleanit.bat idssrsv.err rds.bat stress1.bat testx03p.lst
data idssrsv.log rule.db systems x
distro makefile rule.ndx t x.c
ds1 makefile.tpl setit t.bat xmit
==18272==
==18272== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 1)
==18272== malloc/free: in use at exit: 13833 bytes in 80 blocks.
==18272== malloc/free: 90 allocs, 10 frees, 24577 bytes allocated.
==18272== For a detailed leak analysis, rerun with: --leak-check=yes
==18272== For counts of detected errors, rerun with: -v
--
Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Julian S. <js...@ac...> - 2003-03-16 11:43:01
|
Eyal
It looks like a good patch, and a useful thing too. I would merge
it in, except there is one critical problem which needs fixing first.
The patch uses a glibc function, time(), to get the time. We have
a policy that valgrind itself does not use any glibc functions or
headers. This is to avoid nightmare bugs where V is executing glibc
code on the real CPU at the same time that the client program is
executing glibc code on the simulated CPU. Such mixing has proved
a problem in the past, and recently Nick spent 2 days chasing another
bug of this kind (not his bug, I point out).
This is why we have vg_mylibc.c to supply replacements for libc services
we actually need.
You may do a system call to get time info (look in vg_mylibc.c
for lots of examples). The best solution would be:
- Remove this
+#include <time.h> /* time, localtime, timezone */
+#include <sys/time.h> /* gettimeofday */
- Remove the call to time()
- Write your own vg_time(), somehow, and put it in vg_mylibc.c.
Note that there is no particular reason why vg_time should have
the same interface as glibc's time(); feel free to change it as
you feel useful. (Perhaps rename it if you change the meaning).
- Use that instead.
I hope this can be made to work because it looks to me like a
useful patch.
J
On Friday 14 March 2003 10:33 am, Eyal Lebedinsky wrote:
> Nicholas Nethercote wrote:
> > On Fri, 14 Mar 2003, Eyal Lebedinsky wrote:
> > > I added a line saying
> > >
> > > ==nnnn== Time: dddddd-hh:mm:ss
> > >
> > > Is there a standard was to request such timestamping? I would
> > > not mind learning how to properly add it to the package, with
> > > a proper --time-stap=yes option.
> >
> > Just choose an existing option and copy the way it's done. For example,
> > search for "--gdb-attach" and "VG_(clo_GDB_attach)" in the source ("clo"
> > is short for "command line option).
>
> OK, here is a humble patch that adds a new option
> --time-stamp=[no|yes]
> which yields the report below. I find that localtime() fails to account
> for timezone when used in valgrind, if anyone can fix it please do so.
>
> $ date ; valgrind --time-stamp=yes ls
> Fri Mar 14 21:31:01 EST 2003
> ==18272== Memcheck, a.k.a. Valgrind, a memory error detector for
> x86-linux.
> ==18272== Copyright (C) 2002, and GNU GPL'd, by Julian Seward.
> ==18272== Using valgrind-1.9.4, a program instrumentation system for
> x86-linux.
> ==18272== Copyright (C) 2000-2002, and GNU GPL'd, by Julian Seward.
> ==18272== Estimated CPU clock rate is 1204 MHz
> ==18272== For more details, rerun with: -v
> ==18272==
> ==18272== Time: 2003/03/14 10:31:01
> ==18272== pthread_mutex_destroy: mutex is still in use
> ==18272== at 0x40352E70: pthread_error (vg_libpthread.c:288)
> ==18272== by 0x40353D3F: __pthread_mutex_destroy
> (vg_libpthread.c:998)
> ==18272== by 0x402CE1CE: closedir (in /lib/libc-2.2.5.so)
> ==18272== by 0x804A90D: (within /bin/ls)
> 03.ndx ds1.bat odds setit.bat test-all.bat
> build examples okreps ssacssv.err testall
> build.bat ids.db prepids ssacssv.log testall.bat
> chkunpdf ids.dbg prepids.bat ssarun.log testit.bat
> chkunpdf.awk idscosv.err rds stopit.bat tests
> chkunpdf.bat idscosv.log rds.awk stress.bat testx03g.lst
> cleanit.bat idssrsv.err rds.bat stress1.bat testx03p.lst
> data idssrsv.log rule.db systems x
> distro makefile rule.ndx t x.c
> ds1 makefile.tpl setit t.bat xmit
> ==18272==
> ==18272== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 1)
> ==18272== malloc/free: in use at exit: 13833 bytes in 80 blocks.
> ==18272== malloc/free: 90 allocs, 10 frees, 24577 bytes allocated.
> ==18272== For a detailed leak analysis, rerun with: --leak-check=yes
> ==18272== For counts of detected errors, rerun with: -v
|
|
From: Eyal L. <ey...@ey...> - 2003-03-16 23:40:04
|
Julian Seward wrote: > > Eyal > > It looks like a good patch, and a useful thing too. I would merge > it in, except there is one critical problem which needs fixing first. > > The patch uses a glibc function, time(), to get the time. We have And that goes for localtime() too, right? I will track down a private implementation then. But, it should be OK to issue glibc calls at initialisation time. I can then grab timezone offset and use it locally at runtime. -- Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Julian S. <js...@ac...> - 2003-03-16 23:49:58
|
On Sunday 16 March 2003 11:39 pm, Eyal Lebedinsky wrote: > Julian Seward wrote: > > Eyal > > > > It looks like a good patch, and a useful thing too. I would merge > > it in, except there is one critical problem which needs fixing first. > > > > The patch uses a glibc function, time(), to get the time. We have > > And that goes for localtime() too, right? I will track down a private > implementation then. Get hold of the glibc sources and copy relevant bits from there. > But, it should be OK to issue glibc calls at initialisation time. No ... it's not ok to issue glibc calls at any time. You get bad interactions with the dynamic linker. J |
|
From: Eyal L. <ey...@ey...> - 2003-03-17 13:29:27
Attachments:
vg-1.9.4-e4.patch
|
Julian Seward wrote: > > Eyal > > It looks like a good patch, and a useful thing too. I would merge > it in, except there is one critical problem which needs fixing first. > > The patch uses a glibc function, time(), to get the time. We have > a policy that valgrind itself does not use any glibc functions or > headers. Is that so? I see a direct call to gettimeofday() in vg_libpthread.c #include <sys/time.h> /* gettimeofday */ and the include of <pthread.h> also brings in a few time related libc headers. So, I could not define the usual types and had to create new types (vg_time_t, struct vg_tm) and the related functions. Here is a second attempt at these options. The attached patch includes my toying with adjusting the sleep periods and switching to microsec periods (my 17h run finishes in under 7h now), I will send in a clean patch if the time-stamp part is acceptable. The relevant hunks are these (mostly the last one) coregrind/vg_errcontext.c print the timestamp coregrind/vg_include.h clo and my_libc declarations coregrind/vg_main.c clo handling coregrind/vg_mylibc.c VG_(time) and VG_(localtime) I can improve the performance of this implementation (stolen from the linux kernel, the handiest source at the time) and I should stress that this gives UTC, no attempt was done to implement the whole timezone business. I plan to extend the syntax of the option to allow --time-stamp=[+|-]mm:mm to set your local time offset manually, this should be good for most. Please check the code carefully, some of it is monkey see monkey do... -- Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Eyal L. <ey...@ey...> - 2003-03-18 11:16:23
Attachments:
vg-1.9.4-e6.patch
|
Julian Seward wrote: > > Eyal > > It looks like a good patch, and a useful thing too. I would merge > it in, except there is one critical problem which needs fixing first. As promised, here is a clean patch for the new --time-stamp option. I rewrote the logic so it is much simpler. Two general purpose functions were added VG_(time) as the standard time(), but uses type 'vg_time_t' VG_(localtime) as the standard localtime(), but uses type 'vg_tm'. Also, it always sets tm_isdst to zero and timezone is set through clo. I hope it complies with the current programming model and style (please let me know where it fails to do so). BTW, where is the CVS tree (is it public?). -- Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Nicholas N. <nj...@ca...> - 2003-03-18 11:34:04
|
On Tue, 18 Mar 2003, Eyal Lebedinsky wrote: > I hope it complies with the current programming model and style (please > let me know where it fails to do so). From a quick glance, style looks ok. One thing: you use the "long" type for VG_(clo_time_zone)... use the Valgrind synonyms (eg. Long, although I think UInt would be ok here?) We should probably #define the built types "int", etc, to some rubbish so that any use of them is detected... > BTW, where is the CVS tree (is it public?). Publicly readable, on Sourceforge: sourceforge.net/cvs/?group_id=46268 N |
|
From: Eyal L. <ey...@ey...> - 2003-03-18 12:23:20
|
Nicholas Nethercote wrote: > > On Tue, 18 Mar 2003, Eyal Lebedinsky wrote: > > > I hope it complies with the current programming model and style (please > > let me know where it fails to do so). > > >From a quick glance, style looks ok. One thing: you use the "long" type > for VG_(clo_time_zone)... use the Valgrind synonyms (eg. Long, although I > think UInt would be ok here?) > > We should probably #define the built types "int", etc, to some rubbish so > that any use of them is detected... I am not clear on why we want to #define the 'int' types (you mean inside struct vg_tm, right?). The added functions do use new types. Maybe, if it looks close enough, you can adjust it and check it in, and I will then read it and figure what it is that you wanted. -- Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Nicholas N. <nj...@ca...> - 2003-03-18 12:29:37
|
On Tue, 18 Mar 2003, Eyal Lebedinsky wrote: > > We should probably #define the built types "int", etc, to some rubbish so > > that any use of them is detected... > > I am not clear on why we want to #define the 'int' types (you mean > inside struct vg_tm, right?). Sorry, I wasn't clear. We want to avoid using "int", "char", "unsigned int", etc, anywhere in the core or in skins, and instead use "Int", "Char", "UInt", etc. (This isn't really critical, I think, more for consistency.) One way to ensure this would be to put something like this in include/vg_skin.h: #define int no_such_type_as_int_use_Int_instead #define char no_such_type_as_char_use_Char_instead Then any accidental use of "int", "char", etc. would cause a compile error. > The added functions do use new types. Mostly, yes; but VG_(clo_time_zone) is a "long". I think it could/should be a "UInt" (32 bits is enough, right?) N |
|
From: Eyal L. <ey...@ey...> - 2003-03-18 12:37:43
Attachments:
vg-1.9.4-e7.patch
|
Nicholas Nethercote wrote:
>
> On Tue, 18 Mar 2003, Eyal Lebedinsky wrote:
>
> > I hope it complies with the current programming model and style (please
> > let me know where it fails to do so).
>
> >From a quick glance, style looks ok. One thing: you use the "long" type
> for VG_(clo_time_zone)... use the Valgrind synonyms (eg. Long, although I
> think UInt would be ok here?)
No, the timezone can be negative, so Int is probably what we want. I can
do this change [attached].
And when hiding types, how do I do this:
VG_(message)(Vg_UserMsg, "Time: %d/%02d/%02d %02d:%02d:%02d",
tm.tm_year+1900, tm.tm_mon+1, tm.tm_mday,
tm.tm_hour, tm.tm_min, tm.tm_sec );
'%d' is good for 'int', what is the correct one for 'Int'? If I
use '%ld' then I violated the type hiding. In C this is a problem
and for this reason I prefer to use basic types wherever I can, and
typedef only where actually necessary.
--
Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Nicholas N. <nj...@ca...> - 2003-03-18 12:42:05
|
> And when hiding types, how do I do this: > VG_(message)(Vg_UserMsg, "Time: %d/%02d/%02d %02d:%02d:%02d", > tm.tm_year+1900, tm.tm_mon+1, tm.tm_mday, > tm.tm_hour, tm.tm_min, tm.tm_sec ); > > '%d' is good for 'int', what is the correct one for 'Int'? If I > use '%ld' then I violated the type hiding. In C this is a problem > and for this reason I prefer to use basic types wherever I can, and > typedef only where actually necessary. We use '%d' everywhere. I guess the use of Int instead of int isn't so important, more a convention maybe (although Julian may disagree). N |
|
From: Jeremy F. <je...@go...> - 2003-03-18 19:01:26
|
On Tue, 2003-03-18 at 04:41, Nicholas Nethercote wrote:
> We use '%d' everywhere. I guess the use of Int instead of int isn't so
> important, more a convention maybe (although Julian may disagree).
It took me a while to work out that "Long" is not the same as "long".
Its a bit of a trap.
J
|
|
From: Jeremy F. <je...@go...> - 2003-03-18 19:02:30
|
On Tue, 2003-03-18 at 04:37, Eyal Lebedinsky wrote:
> '%d' is good for 'int', what is the correct one for 'Int'? If I
> use '%ld' then I violated the type hiding. In C this is a problem
> and for this reason I prefer to use basic types wherever I can, and
> typedef only where actually necessary.
No. VG_(message) is a Valgrind-internal function, so it is defined in
terms of Int, Char, etc, including the interpretation of %d. %ld is for
Long, which is 64 bits.
J
|
|
From: Eyal L. <ey...@ey...> - 2003-03-18 22:31:49
Attachments:
vg-1.9.4-e8.patch
|
> Jeremy Fitzhardinge wrote: > > On Tue, 2003-03-18 at 04:37, Eyal Lebedinsky wrote: > > > '%d' is good for 'int', what is the correct one for 'Int'? If I > > use '%ld' then I violated the type hiding. In C this is a problem > > and for this reason I prefer to use basic types wherever I can, and > > typedef only where actually necessary. > > > No. VG_(message) is a Valgrind-internal function, so it is defined in > terms of Int, Char, etc, including the interpretation of %d. %ld is > for Long, which is 64 bits. OK, this is good. However, I still am unclear on a few idioms in vg. e.g., when adding to vg_mylibs.c I see some functions are defines as Int my_connect (... while others use Int VG_(write_socket)(... Another confusing point: I see identical functions defined in different contexts, like VG_(do_syscall2) in general and my_do_syscall2 in vg_mylibsc.c. Is there a need for vg_mylibc.c to be independent? Attached in a patch that uses the abstracted types exclusively. -- Eyal Lebedinsky (ey...@ey...) <http://samba.org/eyal/> |
|
From: Nicholas N. <nj...@ca...> - 2003-03-18 22:46:53
|
On Wed, 19 Mar 2003, Eyal Lebedinsky wrote: > OK, this is good. However, I still am unclear on a few idioms in vg. > > e.g., when adding to vg_mylibs.c I see some functions are defines as > Int my_connect (... > while others use > Int VG_(write_socket)(... Are you referring to the use of the VG_ prefix? If so, we use it on any non-static (ie. globally visible) symbols. Since Valgrind shares the same namespace as the client program, at least from the point of view of the dynamic linker, Valgrind and skins shouldn't define any global symbols without VG_ or SK_ or similar prefixes, to minimise the chances of name clashes. It's not necessary for static (ie. local) symbols. (Note that VG_(foo) is just short for vgPlain_foo, that SK_(foo) is just short for vgSkin_foo, etc.) > Another confusing point: I see identical functions defined in different > contexts, like VG_(do_syscall2) in general and my_do_syscall2 in > vg_mylibsc.c. Is there a need for vg_mylibc.c to be independent? I don't see VG_(do_syscall2)() anywhere. But vg_mylibc.c:vg_do_syscall2() is very similar to vg_libpthread.c:my_do_syscall2(). Perhaps the latter could be removed. But they are slightly different, which may be important. Not sure. N |
|
From: Jeremy F. <je...@go...> - 2003-03-18 23:28:14
|
On Tue, 2003-03-18 at 14:31, Eyal Lebedinsky wrote:
> Another confusing point: I see identical functions defined in
> different
> contexts, like VG_(do_syscall2) in general and my_do_syscall2 in
> vg_mylibsc.c. Is there a need for vg_mylibc.c to be independent?
Syscalls are special, because they require values to be placed in
particular registers. They're static or inline, mainly so they can get
the registers they need. There's a bug in that valgrind is compiled
without -fpic despite being a shared object; the problem is that -fpic
makes it hard for the syscall functions to get the registers they need.
I poked at it for a while, but didn't come up with a nice enough
solution to show off.
Obviously the problem is solvable, because glibc does it.
J
|