|
From: Julian S. <js...@ac...> - 2015-08-21 10:24:50
|
I would like to change the default value for the --partial-loads-ok flag so that it is =yes on all targets. Currently it defaults to =yes on OS X targets only, and =no on all other targets. The flag changes the way Memcheck handles loads that are word-sized or larger, are naturally aligned, and overlap the end of a heap allocated block (typically). Originally, Memcheck would complain about the invalid access (due to overlapping the end of the block). But as compilers and handwritten assembly have become more agressive about vectorization, more and more of these loads show up. They can't cause any faults, so the code is correct, but Memcheck complains nonetheless. Setting the value to =yes causes Memcheck not to complain about the invalid access, but to mark the part of the loaded data from the "inaccessible" area as undefined. So if it really gets used, Memcheck will still complain, but this time, validly. So I propose to change it to =yes for all targets for 3.11. J |
|
From: Philippe W. <phi...@sk...> - 2015-08-21 22:02:27
|
On Fri, 2015-08-21 at 12:24 +0200, Julian Seward wrote:
> I would like to change the default value for the --partial-loads-ok flag
> so that it is =yes on all targets. Currently it defaults to =yes on OS X
> targets only, and =no on all other targets.
>
> The flag changes the way Memcheck handles loads that are word-sized or
> larger, are naturally aligned, and overlap the end of a heap allocated
> block (typically). Originally, Memcheck would complain about the
> invalid access (due to overlapping the end of the block). But as
> compilers and handwritten assembly have become more agressive about
> vectorization, more and more of these loads show up. They can't cause
> any faults, so the code is correct, but Memcheck complains nonetheless.
>
> Setting the value to =yes causes Memcheck not to complain about the
> invalid access, but to mark the part of the loaded data from the
> "inaccessible" area as undefined. So if it really gets used, Memcheck
> will still complain, but this time, validly.
>
> So I propose to change it to =yes for all targets for 3.11.
Looks reasonable to me.
Maybe we could change some other clo defaults ?
I am thinking a.o. to 2 candidates:
* Change --leak-check-heuristics=none to --leak-check-heuristics=all
This avoids false possibly lost in c++ code.
No memory or cpu impact (except neglectible cpu needed for heuristic
during leak search, when encountering a possibly leaked block).
* Change --keep-stacktraces=alloc-then-free to alloc-and-free
This gives more information for writes to freed blocks, showing
also the stacktrace where the block was allocated.
Neglectible cpu impact. Memory impact is one word per client
allocated block.
Otherwise, for 3.11, I have on my todo list the following things:
1. run valgrind under valgrind (memcheck) to search for leaks or other
errors inside valgrind itself.
2. investigate the shell_script exec test that causes gdbserver to be
closed twice (reported by Florian). Nothing serious, but it is not
supposed to happen.
Philippe
|
|
From: Florian K. <fl...@ei...> - 2015-08-22 22:55:53
|
On 22.08.2015 00:02, Philippe Waroquiers wrote: > On Fri, 2015-08-21 at 12:24 +0200, Julian Seward wrote: >> I would like to change the default value for the --partial-loads-ok flag >> so that it is =yes on all targets. .....snip..... >> >> So I propose to change it to =yes for all targets for 3.11. > Looks reasonable to me. Sounds good to me, too. > * Change --leak-check-heuristics=none to --leak-check-heuristics=all > This avoids false possibly lost in c++ code. > No memory or cpu impact (except neglectible cpu needed for heuristic > during leak search, when encountering a possibly leaked block). > Surely a win for the user. +1 > * Change --keep-stacktraces=alloc-then-free to alloc-and-free > This gives more information for writes to freed blocks, showing > also the stacktrace where the block was allocated. > Neglectible cpu impact. Memory impact is one word per client > allocated block. Not sure about this one. Say you have an execution path where memory is allocated (@ point A) then freed (@ point F), then written to (@ point W). In general, there can be execution paths through different allocation sites that all go through the same F and W points. How would knowledge of the specific allocation site be helpful in debugging? Florian |
|
From: John R. <jr...@bi...> - 2015-08-23 03:37:04
|
On 08/22/2015 03:55 PM, Florian Krohm wrote: > On 22.08.2015 00:02, Philippe Waroquiers wrote: >> * Change --keep-stacktraces=alloc-then-free to alloc-and-free >> This gives more information for writes to freed blocks, showing >> also the stacktrace where the block was allocated. >> Neglectible cpu impact. Memory impact is one word per client >> allocated block. > > Not sure about this one. Say you have an execution path where memory is > allocated (@ point A) then freed (@ point F), then written to (@ point > W). In general, there can be execution paths through different > allocation sites that all go through the same F and W points. How would > knowledge of the specific allocation site be helpful in debugging? Depending on the app, knowing the specific point of allocation can provide a Clue as to where (and how, and why) a dangling pointer to the block was kept, even though the block was free()d. That might be "in the library" which together with "Colonel Mustard" and "with the rope" lets the programmer win the game. -- |
|
From: Mark W. <mj...@re...> - 2015-08-24 09:15:07
|
On Sat, 2015-08-22 at 00:02 +0200, Philippe Waroquiers wrote: > On Fri, 2015-08-21 at 12:24 +0200, Julian Seward wrote: > > I would like to change the default value for the --partial-loads-ok flag > > so that it is =yes on all targets. Currently it defaults to =yes on OS X > > targets only, and =no on all other targets. > > > > The flag changes the way Memcheck handles loads that are word-sized or > > larger, are naturally aligned, and overlap the end of a heap allocated > > block (typically). Originally, Memcheck would complain about the > > invalid access (due to overlapping the end of the block). But as > > compilers and handwritten assembly have become more agressive about > > vectorization, more and more of these loads show up. They can't cause > > any faults, so the code is correct, but Memcheck complains nonetheless. > > > > Setting the value to =yes causes Memcheck not to complain about the > > invalid access, but to mark the part of the loaded data from the > > "inaccessible" area as undefined. So if it really gets used, Memcheck > > will still complain, but this time, validly. > > > > So I propose to change it to =yes for all targets for 3.11. > Looks reasonable to me. > > Maybe we could change some other clo defaults ? > > I am thinking a.o. to 2 candidates: > > * Change --leak-check-heuristics=none to --leak-check-heuristics=all > This avoids false possibly lost in c++ code. > No memory or cpu impact (except neglectible cpu needed for heuristic > during leak search, when encountering a possibly leaked block). > > * Change --keep-stacktraces=alloc-then-free to alloc-and-free > This gives more information for writes to freed blocks, showing > also the stacktrace where the block was allocated. > Neglectible cpu impact. Memory impact is one word per client > allocated block. I think all three suggestions are good. If we flip these default in the first beta/rc for 3.11 we can ask people to give feedback on them (and see if there is too much performance impact) and then decide to keep them that way for the final release. > Otherwise, for 3.11, I have on my todo list the following things: > 1. run valgrind under valgrind (memcheck) to search for leaks or other > errors inside valgrind itself. > > 2. investigate the shell_script exec test that causes gdbserver to be > closed twice (reported by Florian). Nothing serious, but it is not > supposed to happen. I have now packaged current svn for fedora rawhide and running it on all arches to look for testsuite issues. See the build.logs for armv7hl, i686 and x86_64 here: http://koji.fedoraproject.org/koji/buildinfo?buildID=678051 (ppc64, ppc64le, aarch64 and s390x are still pending) I did notice we have several vgdb test failures because newer GDB will warn of the remove gdbserver doesn't support the new vfile support (host I/O packets): "warning: remote target does not support file transfer, attempting to access files from local filesystem." https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html Do you think it makes sense (is it easy?) to implement that, or should we just "set sysroot /" or filter out the warning in our testsuite? Cheers, Mark |
|
From: Mark W. <mj...@re...> - 2015-08-25 09:34:50
|
Hi Philippe, On Mon, 2015-08-24 at 11:14 +0200, Mark Wielaard wrote: > I did notice we have several vgdb test failures because newer GDB will > warn of the remove gdbserver doesn't support the new vfile support (host > I/O packets): "warning: remote target does not support file transfer, > attempting to access files from local filesystem." > https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html > Do you think it makes sense (is it easy?) to implement that, or should > we just "set sysroot /" or filter out the warning in our testsuite? So simply filtering out the warning like: Index: gdbserver_tests/filter_stderr =================================================================== --- gdbserver_tests/filter_stderr (revision 15586) +++ gdbserver_tests/filter_stderr (working copy) @@ -10,4 +10,5 @@ -e '/\/path\/to\/gdb/d' \ -e '/and then give GDB the following command/d' \ -e '/target remote |/d' \ - -e '/pid is optional if only one valgrind process is running/d' + -e '/pid is optional if only one valgrind process is running/d' \ + -e '/warning: remote target does not support file transfer, attempting to access files from local filesystem./d' Makes all gdbserver_tests PASS again, except one gdbserver_tests/hgtls. But that seems an unrelated issue. Shall we just go with a filter, or do you want to look into supporting file transfers in vgdb first? Thanks, Mark |
|
From: Philippe W. <phi...@sk...> - 2015-08-25 19:00:30
|
On Tue, 2015-08-25 at 11:34 +0200, Mark Wielaard wrote: > + -e '/warning: remote target does not support file transfer, attempting to access files from local filesystem./d' > > Makes all gdbserver_tests PASS again, except one gdbserver_tests/hgtls. > But that seems an unrelated issue. Shall we just go with a filter, or do > you want to look into supporting file transfers in vgdb first? Hello Mark, Thanks for looking at these failures, and for the filter. Ok to go with the filter, that is largely good enough for the following reasons: 1. implementing file transfer in valgrind gdbserver implies some non minor work (a few hundreds lines of code, I would say). 2. the typical use case of Valgrind gdbserver is to have Valgrind and GDB on the same host, so using gdbserver remote protocol to access files will just be significantly slower, in most cases, and GDB will automatically use the remote file access if V gdbserver would provide it. Of course, without remote file access, it means that for a 'real' remote valgrind (i.e. using vgdb via tcp/ip), that you must have a local copy of the executable and libraries, so that gdb can read them locally. For what concerns the hgtls failure: this is due to a regression in GDB, see bug 'regression in showing __thread so extern variable' https://sourceware.org/bugzilla/show_bug.cgi?id=18564 that has a small reproducer. If someone has a 'gdb bissect setup', this might help to find the regression origin. Philippe |
|
From: Mark W. <mj...@re...> - 2015-08-26 10:28:09
|
On Tue, 2015-08-25 at 21:00 +0200, Philippe Waroquiers wrote: > Ok to go with the filter, that is largely good enough for the following > reasons: > 1. implementing file transfer in valgrind gdbserver implies some > non minor work (a few hundreds lines of code, I would say). > 2. the typical use case of Valgrind gdbserver is to have Valgrind and > GDB on the same host, so using gdbserver remote protocol to access > files will just be significantly slower, in most cases, and GDB > will automatically use the remote file access if V gdbserver would > provide it. > > Of course, without remote file access, it means that for a 'real' remote > valgrind (i.e. using vgdb via tcp/ip), that you must have a local copy > of the executable and libraries, so that gdb can read them locally. OK, I checked in the filter for now (valgrind svn r15591). I opened a bug for the remote files access feature so we can add it for some version after 3.11 if we can find the time to implement it (and do it efficiently): https://bugs.kde.org/show_bug.cgi?id=351792 Thanks, Mark |
|
From: Julian S. <js...@ac...> - 2015-08-27 11:58:12
|
> I am thinking a.o. to 2 candidates: > > * Change --leak-check-heuristics=none to --leak-check-heuristics=all > This avoids false possibly lost in c++ code. > No memory or cpu impact (except neglectible cpu needed for heuristic > during leak search, when encountering a possibly leaked block). > > * Change --keep-stacktraces=alloc-then-free to alloc-and-free > This gives more information for writes to freed blocks, showing > also the stacktrace where the block was allocated. > Neglectible cpu impact. Memory impact is one word per client > allocated block. These both sound good to me. The only concern I have is that they might cause a lot of regtests to fail, so we'd have to update the expected output files too. I would also like to propose the following changes: --smc-check=all-non-file So as to give transparent support for JIT generated code. No perf effect on "normal" (file-backed) code. --dsymutil=yes Since you always need to use this on MacOS, else stack traces are missing or wrong. --read-inline-info=yes My experience of the inlined-unwind functionality introduced in 3.10.0 has been positive, and I always use it. I think it's time to enable it by default. J |
|
From: Ivo R. <ivo...@gm...> - 2015-08-27 12:32:57
|
2015-08-27 13:58 GMT+02:00 Julian Seward <js...@ac...>: > > --read-inline-info=yes > > My experience of the inlined-unwind functionality introduced in 3.10.0 has > been positive, and I always use it. I think it's time to enable it by > default. > +1 for this one. It provides huge help. I. |
|
From: Mark W. <mj...@re...> - 2015-08-31 08:37:45
|
On Thu, 2015-08-27 at 13:58 +0200, Julian Seward wrote: > I would also like to propose the following changes: > > --smc-check=all-non-file > > So as to give transparent support for JIT generated code. No perf effect > on "normal" (file-backed) code. Yes, please. I always forget myself, and if any run time generated code is involved not having it on is really confusing. > --dsymutil=yes > > Since you always need to use this on MacOS, else stack traces are missing > or wrong. No opinion here. But if MacOS users find it useful, lets do it. > --read-inline-info=yes > > My experience of the inlined-unwind functionality introduced in 3.10.0 has > been positive, and I always use it. I think it's time to enable it by > default. Agreed. Although in theory this might cause subtle changes in suppression files (I haven't personally seen this, so it might only be theoretical). Cheers, Mark |
|
From: Philippe W. <phi...@sk...> - 2015-08-31 22:52:54
|
On Mon, 2015-08-31 at 10:37 +0200, Mark Wielaard wrote: > > --read-inline-info=yes > > > > My experience of the inlined-unwind functionality introduced in 3.10.0 has > > been positive, and I always use it. I think it's time to enable it by > > default. > > Agreed. Although in theory this might cause subtle changes in > suppression files (I haven't personally seen this, so it might only be > theoretical). Note that =yes is already the default in 3.10 on linux and android, for memcheck/drd/helgrind. (and is now the default for solaris. Humph, at least the --help is to be updated to add solaris). Ivo, is inline info working properly on solaris ? It is set to =no on Darwin (I think it was not working properly on MacOS). Rhys, any idea/experience with setting it to =yes on Darwin ? (and testing with big applications ? I think it was failing when reading the debug info). Note that inline info behaviour is tested explicitely by some memcheck/tests (e.g. inline, inlinfosuppobj, inlinfo, inltemplate). For most other tools, read inline info is not that useful as it is (mostly/only) useful for tools that are showing symbolic stacktraces during run or when reporting errors. Philippe |
|
From: Ivo R. <ivo...@gm...> - 2015-08-31 22:59:52
|
2015-09-01 0:53 GMT+02:00 Philippe Waroquiers <phi...@sk... >: > On Mon, 2015-08-31 at 10:37 +0200, Mark Wielaard wrote: > > > --read-inline-info=yes > > > > > > My experience of the inlined-unwind functionality introduced in 3.10.0 > has > > > been positive, and I always use it. I think it's time to enable it by > > > default. > > > > Agreed. Although in theory this might cause subtle changes in > > suppression files (I haven't personally seen this, so it might only be > > theoretical). > Note that =yes is already the default in 3.10 on linux and android, > for memcheck/drd/helgrind. > > (and is now the default for solaris. Humph, at least the --help > is to be updated to add solaris). > Ivo, is inline info working properly on Solaris? > --read-inline-info=yes works like a charm on Solaris. All in all, it reads stuff produced by gcc toolchain (with the help of Solaris link editor, though). What part of "--help" do you have in mind? I. |
|
From: Ivo R. <ivo...@gm...> - 2015-09-01 10:53:17
|
2015-09-01 0:59 GMT+02:00 Ivo Raisr <ivo...@gm...>:
>
>
> 2015-09-01 0:53 GMT+02:00 Philippe Waroquiers <
> phi...@sk...>:
>
>> On Mon, 2015-08-31 at 10:37 +0200, Mark Wielaard wrote:
>> > > --read-inline-info=yes
>> > >
>> > > My experience of the inlined-unwind functionality introduced in
>> 3.10.0 has
>> > > been positive, and I always use it. I think it's time to enable it by
>> > > default.
>> >
>> > Agreed. Although in theory this might cause subtle changes in
>> > suppression files (I haven't personally seen this, so it might only be
>> > theoretical).
>> Note that =yes is already the default in 3.10 on linux and android,
>> for memcheck/drd/helgrind.
>>
>> (and is now the default for solaris. Humph, at least the --help
>> is to be updated to add solaris).
>>
>>
Does the following patch look ok to you?
Index: coregrind/m_main.c
===================================================================
--- coregrind/m_main.c (revision 15605)
+++ coregrind/m_main.c (working copy)
@@ -166,7 +166,7 @@
" code except that from file-backed
mappings\n"
" --read-inline-info=yes|no read debug info about inlined function
calls\n"
" and use it to do better stack traces.
[yes]\n"
-" on Linux/Android for
Memcheck/Helgrind/DRD\n"
+" on Linux/Android/Solaris for
Memcheck/Helgrind/DRD\n"
" only. [no] for all other tools and
platforms.\n"
" --read-var-info=yes|no read debug info on stack and global
variables\n"
" and use it to print better error messages
in\n"
Index: docs/xml/manual-core.xml
===================================================================
--- docs/xml/manual-core.xml (revision 15605)
+++ docs/xml/manual-core.xml (working copy)
@@ -1771,8 +1771,8 @@
startup and makes it use more memory (typically for each inlined
piece of code, 6 words and space for the function name), but it
results in more descriptive stacktraces. For the 3.10.0
- release, this functionality is enabled by default only for Linux
- and Android targets and only for the tools Memcheck, Helgrind
+ release, this functionality is enabled by default only for Linux,
+ Android and Solaris targets and only for the tools Memcheck, Helgrind
and DRD. Here is an example of some stacktraces with
<option>--read-inline-info=no</option>:
</para>
Index: none/tests/cmdline1.stdout.exp
===================================================================
--- none/tests/cmdline1.stdout.exp (revision 15605)
+++ none/tests/cmdline1.stdout.exp (working copy)
@@ -80,7 +80,7 @@
code except that from file-backed mappings
--read-inline-info=yes|no read debug info about inlined function calls
and use it to do better stack traces. [yes]
- on Linux/Android for Memcheck/Helgrind/DRD
+ on Linux/Android/Solaris for
Memcheck/Helgrind/DRD
only. [no] for all other tools and
platforms.
--read-var-info=yes|no read debug info on stack and global variables
and use it to print better error messages in
Index: none/tests/cmdline2.stdout.exp
===================================================================
--- none/tests/cmdline2.stdout.exp (revision 15605)
+++ none/tests/cmdline2.stdout.exp (working copy)
@@ -80,7 +80,7 @@
code except that from file-backed mappings
--read-inline-info=yes|no read debug info about inlined function calls
and use it to do better stack traces. [yes]
- on Linux/Android for Memcheck/Helgrind/DRD
+ on Linux/Android/Solaris for
Memcheck/Helgrind/DRD
only. [no] for all other tools and
platforms.
--read-var-info=yes|no read debug info on stack and global variables
and use it to print better error messages in
Kind regards,
I.
|
|
From: Julian S. <js...@ac...> - 2015-09-01 11:43:34
|
Looks fine to me. "make land" :-) J On 01/09/15 12:53, Ivo Raisr wrote: > 2015-09-01 0:59 GMT+02:00 Ivo Raisr <ivo...@gm...>: > >> >> >> 2015-09-01 0:53 GMT+02:00 Philippe Waroquiers < >> phi...@sk...>: >> >>> On Mon, 2015-08-31 at 10:37 +0200, Mark Wielaard wrote: >>>>> --read-inline-info=yes >>>>> >>>>> My experience of the inlined-unwind functionality introduced in >>> 3.10.0 has >>>>> been positive, and I always use it. I think it's time to enable it by >>>>> default. >>>> >>>> Agreed. Although in theory this might cause subtle changes in >>>> suppression files (I haven't personally seen this, so it might only be >>>> theoretical). >>> Note that =yes is already the default in 3.10 on linux and android, >>> for memcheck/drd/helgrind. >>> >>> (and is now the default for solaris. Humph, at least the --help >>> is to be updated to add solaris). >>> >>> > Does the following patch look ok to you? > > Index: coregrind/m_main.c > =================================================================== > --- coregrind/m_main.c (revision 15605) > +++ coregrind/m_main.c (working copy) > @@ -166,7 +166,7 @@ > " code except that from file-backed > mappings\n" > " --read-inline-info=yes|no read debug info about inlined function > calls\n" > " and use it to do better stack traces. > [yes]\n" > -" on Linux/Android for > Memcheck/Helgrind/DRD\n" > +" on Linux/Android/Solaris for > Memcheck/Helgrind/DRD\n" > " only. [no] for all other tools and > platforms.\n" > " --read-var-info=yes|no read debug info on stack and global > variables\n" > " and use it to print better error messages > in\n" > Index: docs/xml/manual-core.xml > =================================================================== > --- docs/xml/manual-core.xml (revision 15605) > +++ docs/xml/manual-core.xml (working copy) > @@ -1771,8 +1771,8 @@ > startup and makes it use more memory (typically for each inlined > piece of code, 6 words and space for the function name), but it > results in more descriptive stacktraces. For the 3.10.0 > - release, this functionality is enabled by default only for Linux > - and Android targets and only for the tools Memcheck, Helgrind > + release, this functionality is enabled by default only for Linux, > + Android and Solaris targets and only for the tools Memcheck, Helgrind > and DRD. Here is an example of some stacktraces with > <option>--read-inline-info=no</option>: > </para> > Index: none/tests/cmdline1.stdout.exp > =================================================================== > --- none/tests/cmdline1.stdout.exp (revision 15605) > +++ none/tests/cmdline1.stdout.exp (working copy) > @@ -80,7 +80,7 @@ > code except that from file-backed mappings > --read-inline-info=yes|no read debug info about inlined function calls > and use it to do better stack traces. [yes] > - on Linux/Android for Memcheck/Helgrind/DRD > + on Linux/Android/Solaris for > Memcheck/Helgrind/DRD > only. [no] for all other tools and > platforms. > --read-var-info=yes|no read debug info on stack and global variables > and use it to print better error messages in > Index: none/tests/cmdline2.stdout.exp > =================================================================== > --- none/tests/cmdline2.stdout.exp (revision 15605) > +++ none/tests/cmdline2.stdout.exp (working copy) > @@ -80,7 +80,7 @@ > code except that from file-backed mappings > --read-inline-info=yes|no read debug info about inlined function calls > and use it to do better stack traces. [yes] > - on Linux/Android for Memcheck/Helgrind/DRD > + on Linux/Android/Solaris for > Memcheck/Helgrind/DRD > only. [no] for all other tools and > platforms. > --read-var-info=yes|no read debug info on stack and global variables > and use it to print better error messages in > > > Kind regards, > I. > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |