Re: [Valgrind-developers] Enhancement to tree builder

[Philippe W. <phi...@sk...>]

On Tue, 2013-09-10 at 12:56 +0200, Julian Seward wrote:
> On 09/06/2013 06:11 PM, Florian Krohm wrote:
> > This patch introduces an enhancement in VEX's tree builder.
> 
> Seems plausible.
> 
> My main concern with this is that it doesn't change the
> carefully-balanced and completely-untested (AFAIK) precise-exception
> semantics, as controlled by
The command line arg value -vex-iropt-register-updates=allregs-at-mem-access
is used in 3 tests, but there is no explicit verification of the behaviour.

I believe the 3 other values are used (implicitely):
   cachegrind and callgrind activates by default sp-at-mem-access
   memcheck uses the default unwindregs-at-mem-access
   --vgdb=full activates allregs-at-each-insn

BTW, looking at this, it means that the --help-debug
default is not ok, as the default depends on the tool
(but I do not see how to make the default tool dependent)

Apart of having all values used, I do not think there is
somewhere a test which verifies e.g. that trapping SEGV
and restarting the instruction works with a certain
value and fails with e.g. sp-at-mem-access. 


> I haven't studied the patch + surrounding code enough to know
> the answer to that.  +1 to land if you can convince yourself
> that PX semantics won't be changed.  +2 if you can also figure
> out how to test the behaviour of the --vex-iropt-register-updates
> flag.
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=913876 for
> the kinds of dirty tricks that people do, requiring precise
> exceptions.
> 
> 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.
> struct { UShort low; UShort high; Bool present; } would make
> it 8 bytes on all targets.  re UShort vs Short, is there any
> need to have the offsets signed?
If memory is important for this type, the "present" can I guess be
avoided by switching to semi-open interval semantic and use
low=high to indicate not present.
In other words, a write at 100, length 8 would be represented
by low = 100, high = 108. A not present interval will be 
   e.g. low = 0, high = 0.

Also, the ATmpInfo components might be re-ordered maybe ?

Did not look much at the rest of the patch.

Philippe