Re: [Valgrind-developers] {add.sub}.w constraints (was: vex: r2227 - trunk/priv)

[Stephen M. <sm...@al...>]

>>>>> "JS" == Julian Seward <js...@ac...> writes:

JS> Stephen,

SMcC> Seeing this patch go by reminds me that I'd noticed a similar
SMcC> seemingly incorrect set of constrains for the versions of the
SMcC> add.w and sub.w instructions that take another register and an
SMcC> optional shift rather than an immediate. I'm still working on an
SMcC> appropriate test suite addition (it's slow going since I only
SMcC> have software emulators).

SMcC> If no-one besides me has run into it yet, I was guessing that my
SMcC> change wasn't important enough to rush into 3.7, but I'm
SMcC> appending it below in case anyone wants to take a look; it will
SMcC> be in Bugzilla in the future.

JS> Um, I wasn't aware this patch existed.

That's to be expected, since this is the first time I've mentioned
it.

JS> What bugzilla number tracks this?

I just created bug #285219 for it.

JS> My (permanent) concern is adding support for instructions without
JS> test cases, since it's exceedingly difficult to track down subtle
JS> errors without test cases.  See
JS> https://bugs.kde.org/show_bug.cgi?id=245925#c3 for a sorry tale.

JS> So I've taken to fixing any such case that turns up in the wild,
JS> or has a regtest to exercise it.

JS> Anyway .. if you have a patch + test case then I will be very
JS> happy to push that into trunk, + into 3_7_BRANCH if anybody
JS> reports it as a failure in practice.

That all seems like a perfectly reasonable policy. Given this, and
since I tend to run into issues with VEX when using it in non-standard
ways, my usual approach has been to sit on patches until I have time
to create a good regression test suite patch for them. That has been
slow going here because this is the first ARM issue I've run into, so
I'm running the ARM test suite for the first time, and finding that it
has a lot of failures (some of which may be caused by running under a
bleeding-edge QEMU rather than on real hardware).

In Bugzilla, I've included details showing that such instructions
appear statically in a recent Ubuntu libc package, and giving a small
test case program to reproduce the failure. From the fact that it
appears in libc, I would guess that GCC can generate this instruction,
though I wasn't immediately able to see how to trigger that in my test
case.

Ideally it looks like this instruction should go in
none/tests/arm/v6intThumb.c, but I'm reluctant to patch that file when
the current version isn't passing on my emulator. (Which is a problem
of its own, though whether in Valgrind or the emulator I don't yet
know.)

 -- Stephen