Bug 300102 - memcheck tester
Summary: memcheck tester
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-15 23:54 UTC by Florian Krohm
Modified: 2012-08-28 19:18 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
tarball (100.00 KB, application/x-tar)
2012-05-15 23:55 UTC, Florian Krohm
Details
tarball2 (100.00 KB, application/x-tar)
2012-05-16 01:50 UTC, Florian Krohm
Details
tarball3 (110.00 KB, application/x-tar)
2012-05-23 03:27 UTC, Florian Krohm
Details
patch to valgrind (1.90 KB, patch)
2012-08-12 04:12 UTC, Florian Krohm
Details
VEX patch (4.76 KB, patch)
2012-08-12 04:15 UTC, Florian Krohm
Details
tarball with vbit tester and vex/valgrind patches (160.00 KB, application/x-tar)
2012-08-19 04:01 UTC, Florian Krohm
Details
updated version of vbit-tester (160.00 KB, application/x-tar)
2012-08-23 03:33 UTC, Florian Krohm
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Krohm 2012-05-15 23:54:02 UTC
A script to generate testcases to make sure that propagation of undefined'ness works as expected. The script will generate an assemberl program that will be compiled, linked and memchecked in various ways. The README file has a summary at the beginning.
Current status is: incomplete. s390 is supported to some extend: floating point ops, bitwise ops, addition should work.  See TODO section in README for what is missing. I started with a port to x86-64 (x86_64py) but I'm not familiar with that architecture. So this is currently segfaulting. 
To get started: 
unpack tarball
cd memcheck-test
./main.py 
./Iop_Add32.py --debug


Reproducible: Always
Comment 1 Florian Krohm 2012-05-15 23:55:11 UTC
Created attachment 71127 [details]
tarball
Comment 2 Florian Krohm 2012-05-16 01:50:12 UTC
Created attachment 71129 [details]
tarball2

2 pints later.....
Several fixes for x86_64 port. With those changes 4 IROps are now supported on that arch:
Add32, And32, Or32, Xor32.
You should see something like this:

florian@snowflake:/tmp/memcheck-test> ./Iop_And32.py 
Iop_And32 op1: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31  
Iop_And32 op2: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31  
Iop_And32	--> OK
Comment 3 Florian Krohm 2012-05-23 03:27:57 UTC
Created attachment 71306 [details]
tarball3

This version 
- adds support for IRops that sign-extend or truncate.
- adds README.internals that has info about the GCC invocations for the various test runs.
- adds command line flag  --run  which will test all opcodes (i.e. will invoke the generated Iop_whatever.py scripts)
- supports 72 IRops on s390x
Comment 4 Julian Seward 2012-07-05 08:02:48 UTC
What's the status on this?  Is there a way it can be committed into the tree so that
it doesn't get forgotten about / bitrotted?
Comment 5 Florian Krohm 2012-07-05 09:59:51 UTC
It's in reasonably good shape for s390 but not completely done. Other platforms are not supported. There is some work left to do for s390. I won't get to it until I'm back in the US (late next week).
For some reason I cannot log on to the s390 community box from here...
It's not important to be committed for 3.8 as it's only a QA tool.

WRT other platforms: I started with an x86-64 port but as I'm not familiar with that platform that takes a lot of time.. Could need some help there.
Comment 6 Florian Krohm 2012-08-12 04:11:17 UTC
Here is a different approach to the memcheck tester that does not
need any retargeting. It uses VALGRIND_SET_VBITS and VALGRIND_GET_VBIT
that were recently brought to my attention :) plus some new machinery
to inject IR into the program flow. Best explained with an example
(which is fully functional on s390 with the attached patches below):

#include <stdio.h>
#include "memcheck.h"
#include "libvex.h"

// Insert a client request that will initialize VEX for IR injection
#define VALGRIND_VEX_INIT_FOR_IRI(iricb) \
        VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__VEX_INIT_FOR_IRI, iricb, \
                                        0,0,0,0)

int main(void)
{
  int opnd1, v_opnd1;   // input operand and its V-bits
  int result, v_result; // result and its V-bits

  IRICB iricb;

  iricb.op     = Iop_Not32;
  iricb.result = (HWord)&result;
  iricb.opnd1  = (HWord)&opnd1;
  iricb.opnd2  = 0;               // unused
  iricb.opnd3  = 0;               // unused
  iricb.opnd4  = 0;               // unused

  VALGRIND_VEX_INIT_FOR_IRI(&iricb);

  // Let's test bit #8 and set it to 'undefined'
  v_opnd1 = 1 << 8;

  VALGRIND_SET_VBITS(&opnd1, &v_opnd1, sizeof opnd1);

  // Create the IR equivalent of result = ~opnd1;
  VALGRIND_VEX_INJECT_IR();

  VALGRIND_GET_VBITS(&result, &v_result, sizeof result);

  if (v_result != v_opnd1)
     printf("wrong result\n");

  return 0;
}

In this program we want to test whether a single undefined input bit
propagates correctly through Iop_Not32. 
First, we arrange for a structure IRICB which has all the necessary
bits so we can build some IR later that will be tested.
VALGRIND_VEX_INIT_FOR_IRI is a new client request which will push this
IRICB structure to VEX which in turn will make a copy of it for future
reference.
Then we set the V-bits of the input operand and invoke VALGRIND_VEX_INJECT_IR.
That macro inserts a new special opcode into the insn stream which the
decoder will detect and then build the IR based on the information passed
in earlier via the IRICB. In this case the decoder would build:

store(mkU64(iricb.result),
      unop(Iop_Not32, load(Ity_I32, mkU64(iricb.opnd1))));

and inject it into the IRSB.
After that we get the vbits of the result and test them for validity.

The valgrind and VEX changes are in the patches below. 
What is missing is the test harness that iterates over all the IROps
and their input bits. I'm working on that.
Comment 7 Florian Krohm 2012-08-12 04:12:47 UTC
Created attachment 73106 [details]
patch to valgrind

This patch defines the new client request and a new special opcode. 
It's only done for s390 but can be trivially done for other platforms
as well.
Comment 8 Florian Krohm 2012-08-12 04:15:15 UTC
Created attachment 73107 [details]
VEX patch

These are the VEX changes. Pretty straight-forward. Note, that all the
beef is currently in guest_s390_toIR.c which is the wrong place. The
reason it's there is because all the convenience functions to build IR: load, store,
unop, binop, etc... are not available in ir_defs.c (we should change that). But clearly, the
VEX changes are generic. Only the width of an address and the endianess
are needed.
Comment 9 Florian Krohm 2012-08-19 04:01:26 UTC
Created attachment 73285 [details]
tarball with vbit tester and vex/valgrind patches

Here is an updated version of the V-bit tester.
The attached tarball contains:

- a patch for VEX
  - new file ir_inject.c to do all the work
    This file is overly verbose; I'll trim it a bit...
  - decoder changes for special opcode on s390x and amd64

- a patch for valgrind
  - arrange for new client request VG_USERREQ__VEX_INIT_FOR_IRI
  - arrange for new special opcodes for s390x and amd64
  - a patch for mc_translate to handle the following IROps
    - Iop_Sar8  -- generated in the x86 frontend
    - Iop_CmpwNEZ32  -- generarted in ir_opt
    - Iop_8HLto16

- a tarball vbit-test.tar with the test program
  - this will extract into a directory vbit-test
  - Makefile is hand-crafted for now and needs to be modefied to fix
    the include path if you want to try it. Will eventually get autotool'ised

The vbit tester supports the common IROps. There are two known issues
(1) on s390x  Iop_Shr8/16 and Iop_Sar8/16 are busted
    Those work fine on amd64; so it's a bug in s390 codegen
(2) on amd64  Iop_128HIto64 is not working properly
    This works on s390. Need to investigate.
Everything else looks good.

There is not much documentation for the vbit tester but the code should be easy to grasp. 
Running all tests takes a few seconds. So this thing could and should be integrated into make regtest. A new subdirectory underneath memcheck/tests makes most sense to me.
Comment 10 Florian Krohm 2012-08-20 16:53:20 UTC
The tester now also works on ppc. There are a couple of hickups there:

The insn selector supports Add16 but not Left16. So memcheck does not work
Likewise for Sub16.
The insn selector considers Iop_Shr8/16 and Iop_Shr8/16 but will assert later
upon encountering them.
The following insns are not handled in insn selection which was a bit of
a surprise 
- Iop_8HLto16    memcheck may generate it
- Iop_16HLto32   likewise
Comment 11 Florian Krohm 2012-08-20 17:00:46 UTC
amd64 frontend may generate Iop_Mul8, but its insn selector does not handle it.

The issues on s390 that were mentioned in comment #9  habe been fixed. Those were indeed bugs in s390  insn selection / code generation.
Comment 12 Florian Krohm 2012-08-23 03:33:13 UTC
Created attachment 73408 [details]
updated version of vbit-tester

Another update. The attached tarball contains
- a patch for VEX with these changes
  - new file ir_inject.c
  - libvex_ir.h: sentinel Iop_LAST added to IROp type so we can conveniently
    iterate over all IROps
  - libvex.h: typedef for IRICB (IR Injection Control Block)
  - guest_amd64/ppc64/s390x_ir.c: recognition of new special opcode that
    triggers IR injection
- a patch for valgrind with these changes
  - configury and Makefile changes to build memcheck/tests/vbit-test and have it run as 
   part of make regtest
  - definition of new client request  VG_USERREQ__VEX_INIT_FOR_IRI
  - definition of VALGRIND_VEX_INJECT_IR to trigger IR injection
- a patch containing the vbit tester
  - the tester resides in memcheck/tests/vbit-test which seems most
    logical

The tester covers all IROps that are common across architectures, i.e.
from Iop_Add8 to Iop_RoundF64toF32. With the exception of Iop_CmpORDxxx.
Those are left as an exercise for the ppc maintainers.
Also included is support for the DFP ops.
The following architectures are currently supported: amd64, ppc64, s390x.
I can wire up x86, mips, and arm if somebody wants to provide me access.
I don't have plans to support vector insns, as I'm not familiar with those.

The problem on amd64, reported in comment #9, has been resolved. It was
a bug in the vbit tester. So... no worries :)

I noticed: The ppc insn selector cannot make up his mind. It considers
Iop_CmpLT64S, Iop_CmpLE32S, and Iop_CmpLE64S but when they do show up,
it asserts. This did not surface earlier because the previous version of the tester did
not handle IROps with Boolean results.

Julian, there are a couple of formalities:
(1) Do you want copyright notices in the files that make up the vbit
    tester?  Currently, there are none.  It's about 3600 LOC.
(2) The vbit tester has a copy of typeOfPrimop without copyright. If that's
    not OK, I can split that function into its own file and fix it.
Comment 13 Florian Krohm 2012-08-24 18:49:06 UTC
I added support for mips32. The few IROps it supports run through without errors.
The insn selector gives the impression to support 64-bit ops (as evidenced by e.g. case Iop_Add64: )
but that is clearly not the case. All such insn assert later on.
Perhaps this is work in progress or just copied and pasted ...
Comment 14 Julian Seward 2012-08-25 08:36:13 UTC
I just had a look through the the latest patch.  It has turned into
quite a magnum opus!  I am not claiming to understand it.  2 simple
questions tho:

* For the type IRICB, you have

  +      HWord result;    // address of the result
  +      HWord opnd1;     // address of 1st operand
  +      HWord opnd2;     // address of 2nd operand
  +      HWord opnd3;     // address of 3rd operand
  +      HWord opnd4;     // address of 4th operand

  Are these intended to indicate the address, from the guest code
  point of view, where the operands and result are?  Should they point
  to 256-bit blocks of memory?

* Maybe good to add, in libvex_ir.h, at the top of type IROp, some
  explanation about how the tester needs to be changed when new ops
  are added.  Or perhaps it will just fail to compile or fail to run
  in that situation, hence making it obvious that something was
  forgotten?
Comment 15 Florian Krohm 2012-08-25 14:23:45 UTC
(In reply to comment #14)
> I just had a look through the the latest patch. It has turned into quite a magnum opus! 

True. There are more kinds of undefinedness propagation than one would anticipate. Right now, there are 14 of them just for the basic (non architecture-specific) IROps. And for each of those you have to write a little snippet of code that double the validity of a result.
 
> * For the type IRICB, you have
> 
>   +      HWord result;    // address of the result
>   +      HWord opnd1;     // address of 1st operand
>   +      HWord opnd2;     // address of 2nd operand
>   +      HWord opnd3;     // address of 3rd operand
>   +      HWord opnd4;     // address of 4th operand
> 
>   Are these intended to indicate the address, from the guest code
>   point of view, where the operands and result are?

Yes, exactly.

>  Should they point  to 256-bit blocks of memory?

Yes, they could.  I have not tested with 256-bit wide operands as the generic primops do not use those. 

> * Maybe good to add, in libvex_ir.h, at the top of type IROp, some
>   explanation about how the tester needs to be changed when new ops
>   are added.  Or perhaps it will just fail to compile or fail to run
>   in that situation, hence making it obvious that something was
>   forgotten?

I'll add some verbiage. The tester is meant to run as part of 'make runtest' and will fail, if a new opcode has been added. So it won't get unnoticed if it gets out of synch.

I had two questions regarding copyright notices at the end of comment #12. Can you take a quick look?
Comment 16 Julian Seward 2012-08-25 19:42:15 UTC
(In reply to comment #12)

> I noticed: The ppc insn selector cannot make up his mind. It considers
> Iop_CmpLT64S, Iop_CmpLE32S, and Iop_CmpLE64S but when they do show up,
> it asserts. This did not surface earlier because the previous version of the
> tester did not handle IROps with Boolean results.

Strange.  I wonder what is going on there.  I wonder if these are even
used on ppc, since the ppc front end generates Iop_CmpORD* for scalar
comparisons, not any of the "normal" Cmp{LE,LT} etc group.  Perhaps
you can commit a change which comments out the consideration bit for
the unhandled cases, so they jump correctly to the
vex_printf("iselCondCode(ppc): No such tag(%u)\n" bit at the end of
the function, rather than merely asserting.

> (1) Do you want copyright notices in the files that make up the vbit
>     tester?  Currently, there are none.  It's about 3600 LOC.

I don't feel strongly either way, but if you want to, please feel
free.  There are many huge test programs in the code base (eg,
none/tests/{amd64,arm}/*.c) that have no such notice.  But there are
some that do.

> (2) The vbit tester has a copy of typeOfPrimop without copyright. If that's
>     not OK, I can split that function into its own file and fix it.

Fine by me.  Maybe add a comment to it though saying it started out
life as a copy of the one in ir_defs.c though.

For the new non-test file ir_inject.c, pls can you use the format

  Copyright (C) 2012-2012  Florian Krohm <email address>,

the 2012-2012 thing so that auxprogs/change-copyright-year handles it,
and the email address because I believe (IIRC) the GNU guidelines say
one should be present.
Comment 17 Florian Krohm 2012-08-28 16:54:00 UTC
I've added VEX r2490 and valgrind r12906.
guest_arm_toIR.c needs a little bit of implementation work. Should be straight forward.
amd and x86 are not tested (no access to system).
I'll clean up the ppc insn selector in a followup patch.

I addressed the comments re copyright and such.
Comment 18 Florian Krohm 2012-08-28 19:18:18 UTC
(In reply to comment #16)
> (In reply to comment #12)
> 
> > I noticed: The ppc insn selector cannot make up his mind. It considers
> > Iop_CmpLT64S, Iop_CmpLE32S, and Iop_CmpLE64S but when they do show up,
> > it asserts. This did not surface earlier because the previous version of the
> > tester did not handle IROps with Boolean results.
> 
> Strange.  I wonder what is going on there.  I wonder if these are even
> used on ppc, since the ppc front end generates Iop_CmpORD* for scalar
> comparisons, not any of the "normal" Cmp{LE,LT} etc group. 

Right, the ppc frontend does not use those. But what about tools?  They are free to create expressions using these IROps in their instrumentation.  And we don't want to place a burden on tools that requires them to use CmpORD on ppc and the other comparison ops elsewhere. Besides, that would be very ugly indeed.

So what I'm going to do is to leave the ppc insn selector as is because I really think that it should
support these primops.

Related to this: It would be desirable to have a comment in libvex_ir.h  explaining to tool writers which primops they are allowed to use in a tool that is meant to run on multiple platforms. Doing so would also point out where our insn selectors need to be beefed-up.