GMP 5.1.1: Valgrind reports incorrect read in __gmpn_copyd (called from __gmpz_mul_2exp)

[Alexander Kruppa]

2013/2/22  <bodrato at mail.dm.unipi.it>:
> Ciao Alexander,
>
> Il Gio, 21 Febbraio 2013 4:30 pm, Alexander Kruppa ha scritto:
>
>> case, it would be a bug in Valgrind, imho - notwithstanding that
>> --partial-loads-ok=yes is a last resort hack.
>
> Valgrind documentation reads: "code that behaves in this way is in
> violation of the the ISO C/C++ standards, and should be considered
> broken."
> But the code in GMP that needs this option is not a piece of portable C
> code, but an assembly optimised portion of code that is used only on some
> CPUs where it is known that it's safe! I'd not consider it broken.

The --partial-loads-ok=yes option effectively disables some checks at
array boundaries, where they are needed most, so using that option is
a last-resort hack, even if the language specification does not
explicitly declare partially-valid loads as giving undefined program
behaviour.


>> At any rate, if this error is by design for efficiency reasons, I
>> think it would be nice to have a configure option to make GMP choose
>> code that strictly adheres to correct memory access. Memory checkers
>
> If you do not care about efficiency, GMP compiled with --disable-assembly
> should adhere to ISO C standards (if it doesn't, that's a bug!).
>
> Removing the files listed by $(grep -rl fastsse mpn/x86_64), before
> ./configure, can be another hack.

How do you assume that I do not care for efficiency? Aiming for a
false dichotomy?


>> a shame to reduce their utility with deliberate false positives.
>
> Does "deliberate" means "intentional"? As the corresponding Italian word
> does?
> I can assure you that the goal of that code is not "to reduce" usefulness
> of memory checkers by triggering "false positives" :-D

Yes, "deliberate" means a decision made after some consideration, and
thus implies intentional. The out-of-bounds access is most certainly
deliberate, with the reasons behind the decision as explained on the
Debugging page of gmplib.org and in Torbjörn's mail. The false
positives are a necessary result of the decision, but that does not
imply that having them was the reason for making the decision. Your
argument is a non sequitur.


> A possible compromise can be a set of valgrind-friendly custom allocation
> functions.
> I attach a draft proposal I wrote looking at Valgrind documentation. It
> [...]
> I hope this can help.
>
> Regards,
> m

It marks bytes as addressable even though they do not correspond to
allocated memory - as far as I can tell, it would have the same effect
as --partial-loads-ok=yes (it that option were working correctly).

If the code wants to access memory as if it were allocated up to a
16-byte-aligned boundry, why not allocate enough up to a
16-byte-aligned boundry? I.e., instead of telling the memory checker
to ignore those invalid accesses, actually making them valid? Would it
suffice to allocate mpz_t's with even number of allocated limbs on
SSE-enabled x86_64, and, if the same problem exists on 32-bit bit
machines, with number of allocated limbs rounded up to a multiple of
4? This would, of course, likewise hide erroneous accesses past the
well-defined data, i.e., past what _mp_size allows, but at least those
reads would hit valid addresses, and it would not assume any
particular memory checker that needs to be fooled.

On a related note: Since _mp_alloc can be larger than _mp_size (and
usually is), valgrind will usually not even notice reads past
ABS(_mp_size) limbs. This could be checked very accurately, however,
with valgrind client requests that set the limbs _mp_d[ABS(_mp_size)
... _mp_alloc-1] to undefined values on any mpz_t output operand. Mere
read or write accesses past ABS(mp_size) would not trigger a warning,
so long as they are in addressable memory, but if any value read from
such a location is used for a condition or in a system call, it would
rightly complain. This idea is a feature request, tho, and if there's
any interest, should be discussed separately.

Alex

(re-sent to list this time)