|
From: Evan L. <sa2...@cy...> - 2007-08-07 16:24:26
|
I have a problem with some C++ code when using a long double type. A long double is reported as 16 bytes on this system, but Valgrind reports an error if I set a char pointer to the start of the long double, and then try to read 16 bytes from it. The problem seems to be straightforward, and is that Valgrind is smart enough to know that a long double is actually 10 bytes, rather than 16 bytes, and it's reporting the top bytes as uninitialised (see http://osdir.com/ml/debugging.valgrind/2005-08/msg00376.html). Question: is there a smart way to get rid of these error messages? I have a lot of templated code that deals with arbitrary floating-point types, and it relies on 'sizeof' to tell it how big the type is. 'sizeof' tells me that a long double is 16 bytes, so I copy around and manipulate 16-byte quantities. I can get rid of the error messages by assuming that long doubles are actually 80 bits, and not 128 bits, but I'd rather not hard-wire this assumption into the code. Initialising an area of memory and then copying the long doubles into it doesn't work: Valgrind still reports an error, because the copy in of the long double still sets the top 6 bytes to uninitialised data. Any ideas? How does Valgrind know that a long double is 80 bits anyway? Thanks - Evan |
|
From: Nicholas N. <nj...@cs...> - 2007-08-07 22:07:23
|
On Tue, 7 Aug 2007, Evan Lavelle wrote: > I have a problem with some C++ code when using a long double type. A > long double is reported as 16 bytes on this system, but Valgrind reports > an error if I set a char pointer to the start of the long double, and > then try to read 16 bytes from it. > > The problem seems to be straightforward, and is that Valgrind is smart > enough to know that a long double is actually 10 bytes, rather than 16 > bytes, and it's reporting the top bytes as uninitialised (see > http://osdir.com/ml/debugging.valgrind/2005-08/msg00376.html). > > Question: is there a smart way to get rid of these error messages? I > have a lot of templated code that deals with arbitrary floating-point > types, and it relies on 'sizeof' to tell it how big the type is. > 'sizeof' tells me that a long double is 16 bytes, so I copy around and > manipulate 16-byte quantities. I can get rid of the error messages by > assuming that long doubles are actually 80 bits, and not 128 bits, but > I'd rather not hard-wire this assumption into the code. > > Initialising an area of memory and then copying the long doubles into it > doesn't work: Valgrind still reports an error, because the copy in of > the long double still sets the top 6 bytes to uninitialised data. > > Any ideas? How does Valgrind know that a long double is 80 bits anyway? Valgrind works entirely at the binary level, and it knows that x86 FP registers are 80 bits. The errors at that website are all like this: =15339== Syscall param write(buf) points to uninitialised byte(s) ==15339== at 0x503B03: __write_nocancel (in /lib/libc-2.3.5.so) ==15339== by 0x4A6378: new_do_write (in /lib/libc-2.3.5.so) ==15339== by 0x4A6474: _IO_do_write@@GLIBC_2.1 (in /lib/libc-2.3.5.so) ==15339== by 0x4A5C6A: _IO_file_close_it@@GLIBC_2.1 (in /lib/libc-2.3.5.so) ==15339== by 0x49CC55: fclose@@GLIBC_2.1 (in /lib/libc-2.3.5.so) ==15339== by 0x804C952: test_complex_long_double_file (test_complex_source.c:433) ==15339== by 0x8063828: main (test.c:205) So it seems you're passing these partially undefined 16 byte values to the write() system call. That lookss like a genuine bug. Just copying around undefined data won't cause Valgrind to complain, it only complains when you use undefined data in ways that may affect execution, and passing the data to a system call like write() is one such case. Nick |
|
From: John R.
|
Evan Lavelle wrote:
> uint8_t *cptr = (uint8_t *)&src;
> // **BODGE** fixes valgrind error messages: is there a better
> // way to do this?
> // if(Dbits == 128)
> // Dbits = 80;
> for(int i = 0; i < Dbits; i += 8) {
> mask = *cptr++; // <--- VALGRIND REPORTS ERROR HERE
> mask <<= i;
> *dst |= mask;
> }
Is that a true copy+paste of the source code, with no overriding of
operators, and an accurate transcription of the error location?
Does compilation deliver a closed subroutine or inlined code?
Does changing the compiler optimization level between -O2 and -O0
affect the location that memcheck reports?
Memcheck should not complain for the fetch:
mask = *cptr++;
because a fetch does not "use" the bits yet; but memcheck does remember
the status of the fetched bits (initialized or uninitialized.)
No system call [often a write()] and no conditional branch depends on
the bits yet.
Memcheck should not complain for the OR-to-memory:
*dst |= mask;
but instead "merely" update the accounting information about which bits
at *dst are now uninitialized:
A bit of *dst which previously was 1, keeps that initialized value.
( 1 OR x ) is 1.
A bit of *dst which previously was 0, but which lines up with an
uninitialized bit of mask, becomes uninitialized. ( 0 OR x ) is x.
A bit of *dst which previously was uninitialized, but which lines up
with an initialized 1 bit of mask, becomes 1. ( uninit OR 1 ) is 1.
A bit of *dst which previously was uninitialized, and which lines up
with an initialized 0 bit of mask, or with an uninitialized bit of
mask, remains uninitialized. Both ( uninit OR 0 ) and ( uninit OR uninit )
are uninit.
The uninitialized bits still do not affect any conditional branch,
nor any system call, so memcheck still should not complain yet.
> This code works, and doesn't depend on an uninitialised value, but
> Valgrind reports an error when 'T' is a long double.
The workaround for such cases is always the same:
memset(&src, 0, sizeof(src));
which obviously must be done before any existing use of src.
Unfortunately this may mean propagating the memset all the way
back to the allocation of src, which may be far away from the text
of TVLBigInt::mp_set_real().
--
|
|
From: Evan L. <sa2...@cy...> - 2007-08-09 08:36:48
|
Thanks for the explanation; that's a lot clearer.
John Reiser wrote:
> Evan Lavelle wrote:
>> uint8_t *cptr = (uint8_t *)&src;
>
>> // **BODGE** fixes valgrind error messages: is there a better
>> // way to do this?
>> // if(Dbits == 128)
>> // Dbits = 80;
>> for(int i = 0; i < Dbits; i += 8) {
>> mask = *cptr++; // <--- VALGRIND REPORTS ERROR HERE
>> mask <<= i;
>> *dst |= mask;
>> }
>
> Is that a true copy+paste of the source code, with no overriding of
> operators, and an accurate transcription of the error location?
What I hadn't shown was that 'mask' and 'dst' are of a class type (for
storing/handling big integers). "mask = *cptr++" invokes a constructor,
which takes the "*cptr" data. This is eventually compared against zero,
so that I can call an optimised routine for setting memory to 0, if
necessary. There are also errors reported at the next line ("mask <<=
i"). This is because the class left-shift operator does a comparison
operation, and both these comparisons lead to a "conditional jump or
move" error report for the uninitialised bytes. Apologies for not making
this clearer last time; this is the start of the error trail, not the
end. The start is more interesting, because it makes it clear that a
long double causes the problem.
So, in one sense, valgrind is perfectly correct in reporting an error.
However, there's no actual error here; the end result is simply that a
long double gets exactly the same value as another long double,
including the uninitialised bytes. It doesn't actually matter what the
result of the "conditional jump or move" is - they're just different
ways to move around the undefined bytes.
Of course, I can't expect Valgrind to know that there is no higher-level
semantic error here. However, if it was possible to tell Valgrind that a
long double was 16 bytes, as reported by sizeof, then the problem goes away.
> The workaround for such cases is always the same:
> memset(&src, 0, sizeof(src));
> which obviously must be done before any existing use of src.
I had assumed that this wouldn't work, because 'src' is always a
compiler-generated long double, and my reasoning was that writing a long
double into initialised memory would mark the top bytes as
uninitialised. However, you're right; test code below. This code is also
a good short summary of the problem. It assumes that the size of a long
double is 'sizeof(long double)', and allocates this many bytes, and
initialises the first BYTES_TO_INIT bytes in the array. It then loads a
long double into the array, and carries out some ops on the top byte of
the array. If BYTES_TO_INIT is set to sizeof(long double), valgrind
doesn't complain; if it's set to anything less, you get 7 errors.
Thanks -
Evan
================================================
// compile as C-99: ie. 'gcc -std=c99 -o test', etc
#include <stdio.h>
#include <stdint.h>
#include <string.h>
#define LDSIZE sizeof(long double)
// ** valgrind reports errors only if this is less than LDSIZE **
#define BYTES_TO_INIT (LDSIZE-1) // [0, LDSIZE]
int main(void) {
int i;
uint8_t storage[LDSIZE] __attribute__ ((aligned (32)));
memset(storage, 0xaa, BYTES_TO_INIT);
// display initialised storage
printf("%d-byte array (%d bytes initialised):\n", LDSIZE,
BYTES_TO_INIT);
for(i=0; i<LDSIZE; ++i)
printf("%02x ", storage[i]);
printf("\n");
// load a long double into 'storage'
long double *ldptr = (long double *)storage;
*ldptr = 1.0L;
// display over-written storage
printf("%d-byte array, containing 1.0L:\n", LDSIZE);
for(i=0; i<LDSIZE; ++i)
printf("%02x ", storage[i]);
printf("\n");
if((storage[LDSIZE-1] & 0xaa) == 0xaa) // ops for valgrind
printf("top byte is 0xaa\n");
else
printf("top byte is %02x\n", storage[LDSIZE-1]);
}
|
|
From: Evan L. <sa2...@cy...> - 2007-08-08 09:13:25
|
Nicholas Nethercote wrote:
> So it seems you're passing these partially undefined 16 byte values to
> the write() system call. That lookss like a genuine bug. Just copying
> around undefined data won't cause Valgrind to complain, it only
> complains when you use undefined data in ways that may affect execution,
> and passing the data to a system call like write() is one such case.
I didn't look at the 2005 case in detail but, for the sake of argument,
let's assume that the OP had an array of long doubles that he wanted to
write to a file. Any call to 'write' is going to involve using 'sizeof'
somewhere. He might, for example, have calculated the length of the
array as "num_elements * sizeof(long double)". If gcc reports
sizeof(long double) as 16, and you have 10 elements, you're going to
call 'write' for 160 bytes, which has lots of holes in it. Sure, this
could potentially be an error, but almost certainly isn't.
My case is actually very similar. I need to store long doubles somewhere
in memory, and retrieve them at a later time. The code looks like this:
================================================================
template <typename T>
int TVLBigInt::mp_set_real(BigInt *dst, T src)
{
uint8_t *cptr = (uint8_t *)&src;
...<cut: set Dbits to the number of bits in the templated type>
// **BODGE** fixes valgrind error messages: is there a better
// way to do this?
// if(Dbits == 128)
// Dbits = 80;
for(int i = 0; i < Dbits; i += 8) {
mask = *cptr++; // <--- VALGRIND REPORTS ERROR HERE
mask <<= i;
*dst |= mask;
}
================================================================
This code works, and doesn't depend on an uninitialised value, but
Valgrind reports an error when 'T' is a long double. I don't care that
'mask' will have uninitialised bytes in it; 'mask' will only ever be
used as part of a long double. The code still works if I uncomment the
block that starts with **BODGE**, and Valgrind then doesn't report an error.
But, my point is, Valgrind is being too smart. It 'knows' that a long
double is 80 bits, but I can only rely on what the compiler tells me,
which is that a long double is 128 bits. I don't (semantically) use the
uninitialised bytes, because I only ever operate on long doubles using
C++ operations; I just occasionally need to store and retrieve them from
an unusual place. For this, I have to use sizeof, because that's what
sizeof is for. However, Valgrind gives me hundreds of errors.
Ideally, I think that I need to tell Valgrind to use C's size definition
here, rather than its built-in knowledge. I could potentially find a
system header somewhere that could tell me that a long double is
actually 80 bits, but this wouldn't help: C would still store long
doubles in 16 bytes of memory, and I couldn't correctly operate on
arrays of long doubles. In other words, I still need sizeof (actually, I
never operate on arrays, but you get the idea).
Any ideas?
Thanks -
Evan
|
|
From: Nicholas N. <nj...@cs...> - 2007-08-08 22:16:52
|
On Wed, 8 Aug 2007, Evan Lavelle wrote:
> But, my point is, Valgrind is being too smart. It 'knows' that a long
> double is 80 bits, but I can only rely on what the compiler tells me,
> which is that a long double is 128 bits. I don't (semantically) use the
> uninitialised bytes, because I only ever operate on long doubles using
> C++ operations; I just occasionally need to store and retrieve them from
> an unusual place. For this, I have to use sizeof, because that's what
> sizeof is for. However, Valgrind gives me hundreds of errors.
>
> Ideally, I think that I need to tell Valgrind to use C's size definition
> here, rather than its built-in knowledge. I could potentially find a
> system header somewhere that could tell me that a long double is
> actually 80 bits, but this wouldn't help: C would still store long
> doubles in 16 bytes of memory, and I couldn't correctly operate on
> arrays of long doubles. In other words, I still need sizeof (actually, I
> never operate on arrays, but you get the idea).
Valgrind works at the binary level, and knows very little about C. It uses
an assumption that sending undefined bytes to write() is bad, and that is
very nearly always the case. Perhaps in your case it's not bad (although
I'm still not convinced -- you're writing junk bytes to a file) in which
case you have to work around the assumption, by using memset like John
suggested, or one of the client requests that change the status of memory.
> for(int i = 0; i < Dbits; i += 8) {
> mask = *cptr++; // <--- VALGRIND REPORTS ERROR HERE
> mask <<= i;
> *dst |= mask;
> }
Does it really complain on that line? As John said, Valgrind shouldn't
complain on that line, and the example errors you gave previously all
involved calls to write().
Nick
|
|
From: Evan L. <sa2...@cy...> - 2007-08-09 08:46:17
|
Nicholas Nethercote wrote:
> Valgrind works at the binary level, and knows very little about C. It
> uses an assumption that sending undefined bytes to write() is bad, and
> that is very nearly always the case. Perhaps in your case it's not bad
> (although I'm still not convinced -- you're writing junk bytes to a
> file) in which case you have to work around the assumption, by using
> memset like John suggested, or one of the client requests that change
> the status of memory.
>
>> for(int i = 0; i < Dbits; i += 8) {
>> mask = *cptr++; // <--- VALGRIND REPORTS ERROR HERE
>> mask <<= i;
>> *dst |= mask;
>> }
>
> Does it really complain on that line? As John said, Valgrind shouldn't
> complain on that line, and the example errors you gave previously all
> involved calls to write().
The original link I posted was an old message from 2005 - I only posted
it because it looked like the same problem; I don't actually call
'write' anywhere.
But, assuming that calling write() on the top bytes does cause a
problem, surely this is a normal operation anyway? If you're writing
long doubles to a file, then you'll generally want to write an array of
them, and read back that array at some later time. In this case, you
would calculate the size of the array as "num_elements * sizeof(long
double)", and write out the whole lot, or do something equivalent. This
means that you write out the undefined bytes along with everything else.
The only practical fix is to manually compress the array on the way out,
and expand it on the way back, using special knowledge of the real size
of a long double.
I have no idea if this is possible (and I suspect it isn't), but
wouldn't telling valgrind that a long double is actually sizeof(long
double) bytes be a better fix?
Evan
|
|
From: Nicholas N. <nj...@cs...> - 2007-08-09 11:48:58
|
On Thu, 9 Aug 2007, Evan Lavelle wrote: > I have no idea if this is possible (and I suspect it isn't), but > wouldn't telling valgrind that a long double is actually sizeof(long > double) bytes be a better fix? As I said, Valgrind works at the binary level and knows very little about C. It works with registers, memory locations, instructions, etc. It doesn't work with C-level variables, types, etc. So there is no way to tell it this information. Nick |