|
From: <sv...@va...> - 2006-03-10 13:42:17
|
Author: sewardj
Date: 2006-03-10 13:41:58 +0000 (Fri, 10 Mar 2006)
New Revision: 5736
Log:
Add a new kind of memory-painting primitive, which is: 'make_defined'.
For each byte in the range, if the byte is addressible, make it be
initialised, but if it isn't addressible, leave it alone. So it's
like a version of make_readable which doesn't alter addressibility.
Modified:
trunk/memcheck/mc_main.c
trunk/memcheck/memcheck.h
Modified: trunk/memcheck/mc_main.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- trunk/memcheck/mc_main.c 2006-03-09 19:08:20 UTC (rev 5735)
+++ trunk/memcheck/mc_main.c 2006-03-10 13:41:58 UTC (rev 5736)
@@ -741,7 +741,23 @@
set_address_range_perms ( a, len, VGM_BIT_VALID, VGM_BIT_VALID );
}
=20
+/* For each byte in [a,a+len), if the byte is addressable, make it be
+ defined, but if it isn't addressible, leave it alone. In other
+ words a version of mc_make_readable that doesn't mess with
+ addressibility. Low-performance implementation. */
+static void mc_make_defined ( Addr a, SizeT len )
+{
+ SizeT i;
+ UWord abit, vbyte;
+ DEBUG("mc_make_defined(%p, %llu)\n", a, (ULong)len);
+ for (i =3D 0; i < len; i++) {
+ get_abit_and_vbyte( &abit, &vbyte, a+i );
+ if (EXPECTED_TAKEN(abit =3D=3D VGM_BIT_VALID))
+ set_vbyte(a+i, VGM_BYTE_VALID);
+ }
+}
=20
+
/* --- Block-copy permissions (needed for implementing realloc() and
sys_mremap). --- */
=20
@@ -2519,6 +2535,11 @@
*ret =3D -1;
break;
=20
+ case VG_USERREQ__MAKE_DEFINED: /* make defined */
+ mc_make_defined ( arg[1], arg[2] );
+ *ret =3D -1;
+ break;
+
case VG_USERREQ__CREATE_BLOCK: /* describe a block */
if (arg[1] !=3D 0 && arg[2] !=3D 0) {
i =3D alloc_client_block();
Modified: trunk/memcheck/memcheck.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- trunk/memcheck/memcheck.h 2006-03-09 19:08:20 UTC (rev 5735)
+++ trunk/memcheck/memcheck.h 2006-03-10 13:41:58 UTC (rev 5736)
@@ -70,6 +70,10 @@
=20
#include "valgrind.h"
=20
+/* !! ABIWARNING !! ABIWARNING !! ABIWARNING !! ABIWARNING !!=20
+ This enum comprises an ABI exported by Valgrind to programs
+ which use client requests. DO NOT CHANGE THE ORDER OF THESE
+ ENTRIES, NOT DELETE ANY -- add new ones at the end. */
typedef
enum {=20
VG_USERREQ__MAKE_NOACCESS =3D VG_USERREQ_TOOL_BASE('M','C'),
@@ -86,6 +90,8 @@
=20
VG_USERREQ__CREATE_BLOCK,
=20
+ VG_USERREQ__MAKE_DEFINED,
+
/* This is just for memcheck's internal use - don't use it */
_VG_USERREQ__MEMCHECK_RECORD_OVERLAP_ERROR=20
=3D VG_USERREQ_TOOL_BASE('M','C') + 256
@@ -125,6 +131,18 @@
_qzz_res; \
}))
=20
+/* Similar to mark memory at VALGRIND_MAKE_READABLE except that
+ addressibility is not altered: bytes which are addressible are
+ marked as defined, but those which are not addressible are
+ left unchanged. */
+#define VALGRIND_MAKE_DEFINED(_qzz_addr,_qzz_len) \
+ (__extension__({unsigned int _qzz_res; \
+ VALGRIND_DO_CLIENT_REQUEST(_qzz_res, 0 /* default return */, \
+ VG_USERREQ__MAKE_DEFINED, \
+ _qzz_addr, _qzz_len, 0, 0, 0); \
+ _qzz_res; \
+ }))
+
/* Create a block-description handle. The description is an ascii
string which is included in any messages pertaining to addresses
within the specified memory range. Has no other effect on the
|
|
From: Ashley P. <as...@qu...> - 2006-03-10 14:06:50
|
On Fri, 2006-03-10 at 13:42 +0000, sv...@va... wrote:
> Author: sewardj
> Date: 2006-03-10 13:41:58 +0000 (Fri, 10 Mar 2006)
> New Revision: 5736
>
> Log:
> Add a new kind of memory-painting primitive, which is: 'make_defined'.
> For each byte in the range, if the byte is addressible, make it be
> initialised, but if it isn't addressible, leave it alone. So it's
> like a version of make_readable which doesn't alter addressibility.
Should I be changing my client requests to call this new macro? I
assume it throws a warning to the user if you try and make something
readable which isn't addressable?
When I was looking at this before I was thinking of doing something like
the following but settled with MAKE_READABLE in the end.
#define VALGRIND_MAKE_DEFINED(base,len) do { \
if ( ! VALGRIND_CHECK_WRITEABLE(base,len) ) \
VALGRIND_MAKE_READABLE(base,len); \
} while (0)
> +/* !! ABIWARNING !! ABIWARNING !! ABIWARNING !! ABIWARNING !!
> + This enum comprises an ABI exported by Valgrind to programs
> + which use client requests. DO NOT CHANGE THE ORDER OF THESE
> + ENTRIES, NOT DELETE ANY -- add new ones at the end. */
You spotted that too then ;)
Ashley,
|
|
From: Julian S. <js...@ac...> - 2006-03-10 14:19:53
|
> Should I be changing my client requests to call this new macro?
I believe so, but I wouldn't go hacking furiously until all this
has settled down a bit. The motivation is shown below. No, the
macro doesn't give any warnings; it doesn't need to - see below.
> > +/* !! ABIWARNING !! ABIWARNING !! ABIWARNING !! ABIWARNING !!
> > + This enum comprises an ABI exported by Valgrind to programs
> > + which use client requests. DO NOT CHANGE THE ORDER OF THESE
> > + ENTRIES, NOT DELETE ANY -- add new ones at the end. */
>
> You spotted that too then ;)
If folks want to compile client requests into their code and
then ship it, I guess we'd better make efforts to remain backward
compatible :)
If you see any more places where there should be a similar warning
please let me know.
J
---------
At the moment we have 3 kinds of memory-painting macros:
make_{noaccess,writable,readable}. I've been using them when
playing with mpi wrappers, and I noticed there's something
they can't express. Consider this schematic for data arriving
from off-node:
buf = malloc(11); // should be 12, but we made a mistake
// check that the space is big enough
check_writable(buf, 12); // gets a complaint
// prod mpi to actually get the data into buf (do MPI_Recv)
// 12 bytes arrives
// mark it as defined
make_readable(buf, 12)
// ...buf[11] ... // !!! we don't get an addressing error
The problem is that make_readable doesn't just change the definedness
markings (V bits), it also messes with addressibility (the A bits).
What seems to be needed is a variant of make_readable which has the
meaning "if this byte was addressible, mark it initialised, but if
not just leave it alone". (dually make_writable). This would be
useful I imagine in other situations where we're trying to describe
how the state of memory is changing. (custom allocators?)
Interesting we never noticed the need for such a thing before.
|
|
From: Ashley P. <as...@qu...> - 2006-03-10 14:52:56
|
On Fri, 2006-03-10 at 14:19 +0000, Julian Seward wrote:
> > You spotted that too then ;)
>
> If folks want to compile client requests into their code and
> then ship it, I guess we'd better make efforts to remain backward
> compatible :)
>
> If you see any more places where there should be a similar warning
> please let me know.
The one things that springs to mind is to put a SVN hook in which puts a
warning in the commit email you get when you change and of the installed
header files, similar to the way it warns you that printf commits are
potentially dangerous.
> At the moment we have 3 kinds of memory-painting macros:
> make_{noaccess,writable,readable}. I've been using them when
> playing with mpi wrappers, and I noticed there's something
> they can't express. Consider this schematic for data arriving
> from off-node:
>
> buf = malloc(11); // should be 12, but we made a mistake
>
> // check that the space is big enough
> check_writable(buf, 12); // gets a complaint
Yes of course, i use check_writeable here as well.
> // prod mpi to actually get the data into buf (do MPI_Recv)
> // 12 bytes arrives
>
> // mark it as defined
> make_readable(buf, 12)
>
> // ...buf[11] ... // !!! we don't get an addressing error
No which is why I wanted to make the make_readable conditional on the
writeable check succeeding. In the end I didn't because it would mean
maintaining state between the two calls and be quite intrusive. It's
not actually quite as bad as you might think though as the users will
have already seen the previous error.
> The problem is that make_readable doesn't just change the definedness
> markings (V bits), it also messes with addressibility (the A bits).
>
> What seems to be needed is a variant of make_readable which has the
> meaning "if this byte was addressible, mark it initialised, but if
> not just leave it alone". (dually make_writable).
> This would be
> useful I imagine in other situations where we're trying to describe
> how the state of memory is changing. (custom allocators?)
I've still got that bug I was hunting before xmas with the custom
allocator not reporting overruns properly by the way, I noticed you
re-enabled [g|s]et_vbits this week so I'll try to take another look
soon. Can I make a request for a GET_ABITS function as well please if
it's not to difficult.
I was thinking about sending the VBITS over the network with the data
earlier, this would involve a check_writable() function which only
returned true/false and didn't warn the user.
In fact come to think of it what does SET_VBITS do if you call in on
un-addressable memory?
Ashley,
|