Re: [Valgrind-developers] Enhancement to tree builder

[Florian K. <fl...@ei...>]

On 09/10/2013 12:56 PM, Julian Seward wrote:
> 
> My main concern with this is that it doesn't change the
> carefully-balanced and completely-untested (AFAIK) precise-exception
> semantics, as controlled by
> 
> --vex-iropt-register-updates=sp-at-mem-access
>                             |unwindregs-at-mem-access [default]
>                             |allregs-at-mem-access
>                             |allregs-at-each-insn
> 
> In particular, need to verify that (1) the patch does not remove any
> PUTs, loads or stores, and (2) it does not reorder any PUTs with
> respect to loads or stores, and (3) it doesn not reorder any stores
> with respect to loads or other stores.

The patch does none of those things.

What is changed is the logic to decide whether or not an expression
containing one or more GET[I]s and preceding a PUT statement can be
substituted into a statement following the PUT.
Doing such a substitution eliminates a WrTmp statement but otherwise
does not remove or reorder any statement.

> 
> IOW .. imagine we chop up the post-treebuild IRStmts into groups,
> in which each group contains a single Store, a single Put, or
> arbitrary Loads, Gets and arithmetic, but no Stores or Puts.
> (ignoring IRDirty and other complexities).  Then, the question is:
> does the patch change the sequence?

I cannot imagine how it could.

> 
> +2 if you can also figure
> out how to test the behaviour of the --vex-iropt-register-updates
> flag.

We need some way to feed an insn sequence into VEX, run it through the
pipeline with various options and observe the output.
https://bugs.kde.org/show_bug.cgi?id=272405  has some thoughts on that.
See the run-insn.c attachment there. But one would also need a much more
flexible testing harness for that. vg-regtest is not flexible enough.
The BZ discusses that also to some extent.

> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=913876 for
> the kinds of dirty tricks that people do, requiring precise
> exceptions.

Yikes

> 
> re comments on the patch itself:
> 
> +typedef
> +   struct {
> +      Bool present;
> +      Int  low;
> +      Int  high;
> +   }
> +   Interval;
> 
> That has to be at least 12 bytes, or 16 on a 64 bit target.

Did you test your hypothesis ? :)
That struct uses 12 bytes on ILP32 and LP64.

> struct { UShort low; UShort high; Bool present; } would make
> it 8 bytes on all targets. 

.. would make it 6 bytes on all targets.

> re UShort vs Short, is there any
> need to have the offsets signed?

I use an "Int" because that is what is used for "offset into guest
state" and I was attempting to be consistent. Cf. libvex_ir.h -
definition of the Get and Put variants and many other places elsewhere.
IMNSHO those offsets should all be unsigned entities. And, yes, a
"short" would be more than enough.

> 
> +         /* Assume all guest state */
> 
> A bit cryptic, but from reading the lines after, you mean something
> like "Assume all guest state is written" (or some such).

Yes, I'll change that.

    Florian