|
From: Rob <fao...@ya...> - 2012-02-10 16:02:00
|
> Adding the error nr in the Valgrind output would help, but I am not > sure the improvement in gdbserver usage is worth always printing an > error nr (and adding an option --with-error-nr=yes/no looks an overkill > to me). Unless an error nr would be generally useful ? See e.g. how > the 'loss record nr' is now used in 3.8.0SVN to give more details (e.g. > the list of blocks leaked) about a leak. > > In any case, it looks more logical (and might be sufficient) to have > --vgdb-error compared to the nr of shown errors rather than to the > nr of found (and possibly suppressed/possibly not shown errors). > > Can you try the patch below and see if that is working and > sufficient for you ? Thanks for the patch. I have manually applied it to 3.7.0 (not svn) and it is a big improvement. The number seems to be offset by 1 from what I would expect though, eg. --vgdb-error=5 stops after detecting 6 errors. One reason for printing the error number in the output would be to avoid having to manually count them if there are many. Personally I think it would be nicer to always have the errors numbered to help navigating large amounts of output . thanks, Rob |
|
From: Philippe W. <phi...@sk...> - 2012-02-11 00:42:09
|
On Fri, 2012-02-10 at 16:01 +0000, Rob wrote: > > > Thanks for the patch. I have manually applied it to 3.7.0 (not svn) and it is a big improvement. > The number seems to be offset by 1 from what I would expect though, eg. --vgdb-error=5 stops after detecting 6 errors. Thanks for the feedback. I found the reason for the off by one you have seen. I will dig deeper in the difference between nr of errors found, and nr of errors shown. A better patch will follow. > One reason for printing the error number in the output would be to avoid > having to manually count them if there are many. Personally I think it > would be nicer to always have the errors numbered to help navigating large amounts of output . I understand this : it is effectively not very easy for the user to count the error nrs. However, printing an error nr is changing the behaviour for all tools reporting errors. => changing this implies some advice/feedback from others Julian, Bart : an opinion ? Philippe |
|
From: Julian S. <js...@ac...> - 2012-02-14 10:41:13
|
On Saturday, February 11, 2012, Philippe Waroquiers wrote: > On Fri, 2012-02-10 at 16:01 +0000, Rob wrote: > > Thanks for the patch. I have manually applied it to 3.7.0 (not svn) and > > it is a big improvement. > > > > The number seems to be offset by 1 from what I would expect though, eg. > > --vgdb-error=5 stops after detecting 6 errors. > > Thanks for the feedback. I found the reason for the off by one you have > seen. I will dig deeper in the difference between nr of errors found, and > nr of errors shown. A better patch will follow. > > > One reason for printing the error number in the output would be to avoid > > having to manually count them if there are many. Personally I think it > > would be nicer to always have the errors numbered to help navigating > > large amounts of output . > > I understand this : it is effectively not very easy for the user to count > the error nrs. However, printing an error nr is changing the behaviour for > all tools reporting errors. => changing this implies some advice/feedback > from others > Julian, Bart : an opinion ? One thing that might be relevant is that errors already have a 32 bit value that identifies them uniquely. "struct _Error :: unique" You can see them in the XML output, eg ./vg-in-place --xml=yes --xml-fd=1 memcheck/tests/errs1 I would prefer to use them, rather than add yet another kind of error-counter mechanism. But the problem is now to show the user what --vgdb-error value is required for each error. The simple thing to do is to print a line Use --vgdb-error=<unique> to make the GDB server stop at this error Problem is I don't really want to add printing of such lines by default. Is it possible that we can make printing of them conditional on some other command line option that must be present in order to use the gdbserver? J |
|
From: Rob <fao...@ya...> - 2012-02-14 13:54:16
|
> One thing that might be relevant is that errors already have a > 32 bit value that identifies them uniquely. "struct _Error :: unique" > You can see them in the XML output, eg > > ./vg-in-place --xml=yes --xml-fd=1 memcheck/tests/errs1 > > I would prefer to use them, rather than add yet another kind of > error-counter mechanism. But the problem is now to show the user > what --vgdb-error value is required for each error. The simple > thing to do is to print a line > > Use --vgdb-error=<unique> to make the GDB server stop at this error > > Problem is I don't really want to add printing of such lines by > default. Is it possible that we can make printing of them conditional > on some other command line option that must be present in order to > use the gdbserver? > > J Yes, allowing the <unique/> numbers in place of a count would be good. It wouldn't necessarily have to print the full usage method above as this could be documented in the manual. How about appending the numbers at the end of each error line, either by default or with an option? ==14600== Syscall param write(buf) points to uninitialised byte(s) (uid=0x2f9) In my usage scenario I wouldn't normally be using the gdb server, only for specific reruns. I would therefore have the above output turned on all the time, just in case I need it later, assuming it weren't too verbose. thanks, Rob |
|
From: Julian S. <js...@ac...> - 2012-02-14 23:00:03
|
Hmm, this doesn't sound like it's going to be simple to fix in a clean way. For the moment, can we do the incremental fix of taking Philippe's patch (with the off-by-one fixed) ? That's a very simple patch and uncontroversial patch. (Maybe should also backport it for 3.7.1 ?) J On Tuesday, February 14, 2012, Rob wrote: > > One thing that might be relevant is that errors already have a > > 32 bit value that identifies them uniquely. "struct _Error :: unique" > > You can see them in the XML output, eg > > > > ./vg-in-place --xml=yes --xml-fd=1 memcheck/tests/errs1 > > > > I would prefer to use them, rather than add yet another kind of > > error-counter mechanism. But the problem is now to show the user > > what --vgdb-error value is required for each error. The simple > > thing to do is to print a line > > > > Use --vgdb-error=<unique> to make the GDB server stop at this error > > > > Problem is I don't really want to add printing of such lines by > > default. Is it possible that we can make printing of them conditional > > on some other command line option that must be present in order to > > use the gdbserver? > > > > J > > Yes, allowing the <unique/> numbers in place of a count would be good. It > wouldn't necessarily have to print the full usage method above as this > could be documented in the manual. How about appending the numbers at the > end of each error line, either by default or with an option? > > ==14600== Syscall param write(buf) points to uninitialised byte(s) > (uid=0x2f9) > > In my usage scenario I wouldn't normally be using the gdb server, only for > specific reruns. I would therefore have the above output turned on all > the time, just in case I need it later, assuming it weren't too verbose. > > thanks, > Rob > > --------------------------------------------------------------------------- > --- Keep Your Developer Skills Current with LearnDevNow! > The most comprehensive online learning library for Microsoft developers > is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, > Metro Style Apps, more. Free future releases when you subscribe now! > http://p.sf.net/sfu/learndevnow-d2d > _______________________________________________ > Valgrind-users mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-users |
|
From: Philippe W. <phi...@sk...> - 2012-02-15 00:05:20
|
On Tue, 2012-02-14 at 23:52 +0100, Julian Seward wrote: > Hmm, this doesn't sound like it's going to be simple to fix in > a clean way. > > For the moment, can we do the incremental fix of taking Philippe's > patch (with the off-by-one fixed) ? That's a very simple patch > and uncontroversial patch. (Maybe should also backport it for > 3.7.1 ?) I will prepare a patch (and effectively, this looks a good candidate for 3.7.1 backporting). Assuming the idea --vgdb-error-list=<filename> invoke gdbserver for each error described in filename is deemed a good idea, it is for sure not for 3.7.x. Philippe |
|
From: Philippe W. <phi...@sk...> - 2012-02-14 23:25:45
|
On Tue, 2012-02-14 at 13:54 +0000, Rob wrote:
> > One thing that might be relevant is that errors already have a
> > 32 bit value that identifies them uniquely. "struct _Error :: unique"
> > You can see them in the XML output, eg
> >
> > ./vg-in-place --xml=yes --xml-fd=1 memcheck/tests/errs1
> >
> > I would prefer to use them, rather than add yet another kind of
> > error-counter mechanism. But the problem is now to show the user
There is in fact already a mechanism which counts the number of printed
errors, used a.o. to tell the user
"More than %d errors detected. Subsequent errors\n"
"will still be recorded, but in less "
"detail than before.\n"
The idea behind the current --vgdb-error was to use this counting
mechanism.
The manual for --vgdb-error=<number> says:
"Tools that report errors will wait for "number" errors to be reported
before freezing the program and waiting for you to connect with GDB.
It follows that a value of zero will cause the gdbserver to be started
before your program is executed. This is typically used to insert
GDB breakpoints before execution, and also works with tools that
do not report errors, such as Massif."
I have a preference for this "number" errors to be reported, but if
the above is still not convincing, fine for me to use the unique concept
instead.
> > what --vgdb-error value is required for each error. The simple
> > thing to do is to print a line
> >
> > Use --vgdb-error=<unique> to make the GDB server stop at this error
> >
> > Problem is I don't really want to add printing of such lines by
> > default. Is it possible that we can make printing of them conditional
> > on some other command line option that must be present in order to
> > use the gdbserver?
Difficult to find a mandatory command line option as --vgdb=yes is the
default.
So, a new option would be needed (e.g.
--print-unique-error-nr=no|yes print error nr for each error
or maybe
--print-vgdb-error=no|yes print error nr to use for --vgdb-error
> Yes, allowing the <unique/> numbers in place of a count would be good.
> It wouldn't necessarily have to print the full usage method above as this could be documented in the manual.
> How about appending the numbers at the end of each error line, either by default or with an option?
>
> ==14600== Syscall param write(buf) points to uninitialised byte(s) (uid=0x2f9)
To make the link with --vgdb-error, maybe
==14600== Syscall param write(buf) points to uninitialised byte(s) (--vgdb-error=761)
(I would in any case not use hexadecimal for this, so as to match the way integers
options are read)
Numbering the errors (either with n_errs_shown or with unique) will for
sure help.
However, with multi-threaded applications, the order and numbering of
errors might not be easily reproduced from one run to another.
At work, a user is doing a nasty trick to survive in such a case: he
writes a suppression file for all errors preceeding the one he is
interested in. Not very easy but can be made better
by having a new command line option:
--vgdb-error-list=<filename> invoke gdbserver for each error described in filename
The big advantage of this schema is that it is not sensitive
to scheduling/numbering/...
With this, gdbserver would be invoked either when the error nr is >= --vgdb-error
or when it matches an error described in --vgdb-error-list.
The --vgdb-error-list=<filename> will use the same format as a suppression file.
Philippe
|
|
From: Rob <fao...@ya...> - 2012-02-14 23:59:01
|
> Hmm, this doesn't sound like it's going to be simple to fix in > a clean way. > > For the moment, can we do the incremental fix of taking Philippe's > patch (with the off-by-one fixed) ? That's a very simple patch > and uncontroversial patch. (Maybe should also backport it for > 3.7.1 ?) Sounds good to me. This is still a big improvement. > At work, a user is doing a nasty trick to survive in such a case: he > writes a suppression file for all errors preceeding the one he is > interested in. Not very easy but can be made better > by having a new command line option: > --vgdb-error-list=<filename> invoke gdbserver for each error described in filename > > The big advantage of this schema is that it is not sensitive > to scheduling/numbering/... > > With this, gdbserver would be invoked either when the error nr is>= --vgdb-error > or when it matches an error described in --vgdb-error-list. > The --vgdb-error-list=<filename> will use the same format as a suppression file. > Even better! I think this is a good combination of options and should suit most eventualities. thanks, Rob |
|
From: Philippe W. <phi...@sk...> - 2012-02-15 22:38:08
|
On Tue, 2012-02-14 at 23:58 +0000, Rob wrote: > > Hmm, this doesn't sound like it's going to be simple to fix in > > a clean way. > > > > For the moment, can we do the incremental fix of taking Philippe's > > patch (with the off-by-one fixed) ? That's a very simple patch > > and uncontroversial patch. (Maybe should also backport it for > > 3.7.1 ?) > > Sounds good to me. This is still a big improvement. fixed in revision 12388. > > With this, gdbserver would be invoked either when the error nr is>= --vgdb-error > > or when it matches an error described in --vgdb-error-list. > > The --vgdb-error-list=<filename> will use the same format as a suppression file. > > > > Even better! I think this is a good combination of options and should > suit most eventualities. Not doing this for the moment (waiting for more feedback/more demands). Let's see if the easy patch is good enough for the moment ... Thanks Philippe |