[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=376257

Philippe Waroquiers  changed:

   What|Removed |Added

 Status|RESOLVED|CLOSED

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=376257

Philippe Waroquiers  changed:

   What|Removed |Added

 Resolution|--- |FIXED
 Status|CONFIRMED   |RESOLVED

--- Comment #8 from Philippe Waroquiers  ---
(In reply to Julian Seward from comment #7)
> Philippe, thanks for having the patience (and finding a way!) to redo this
> without changing Vex.  This looks good to me.  Just a few minor comments.

Thanks for the review and comments. I have handled all of them (details below),
and commited the result as 619fb35df7b3fba514da7298c8b428d1ec490f93


> 
> +   /* Take into account the first_ip_delta and first_sp_delta. */
> startRegs.r_pc += (Long)(Word)first_ip_delta;
> +   startRegs.r_sp += (Long)first_sp_delta;
> 
> You might as well remove the (Word) cast for the r_pc line, since it's
> redundant.
(Word) removed.

> 
> 
> +if (fixupSP_needed) {
> +   hName = "evh__mem_help_cwrite_4_fixupSP";
> +   hAddr = __mem_help_cwrite_4_fixupSP;
> +} else {
> +   hName = "evh__mem_help_cwrite_4";
> +   hAddr = __mem_help_cwrite_4;
> +}
> 
> Please add a short comment somewhere, explaining the difference
> between (eg) evh__mem_help_cwrite_4_fixupSP and evh__mem_help_cwrite_4.
I have added short comments in the above 'if', and in the functions
evh__mem_help_cwrite_4_fixupSP and evh__mem_help_cwrite_8_fixupSP

> 
> 
> +   the SP needed to unwind need to be fixed UP.
> 
> Did you mean "UP" and not "up"?
Typo, changed to up.

> 
> 
> +static Bool check_cached_rcec_ok (Thr* thr, Addr previous_frame0)
> +{
> 
> Is this just for debugging, or is it used in "normal" runs?  If it
> is for normal runs, is it safe -- meaning, can it cause the run to
> fail if some of the heuristics it uses are not quite right?
> 
> If it is just for debugging (which I am hoping), please add a comment to say
> that.
I have added a comment indicating that this is for debuggging only (activated
by
--hg-sanity-flags.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=376257

--- Comment #7 from Julian Seward  ---
Philippe, thanks for having the patience (and finding a way!) to redo this
without changing Vex.  This looks good to me.  Just a few minor comments.

+   /* Take into account the first_ip_delta and first_sp_delta. */
startRegs.r_pc += (Long)(Word)first_ip_delta;
+   startRegs.r_sp += (Long)first_sp_delta;

You might as well remove the (Word) cast for the r_pc line, since it's
redundant.


+if (fixupSP_needed) {
+   hName = "evh__mem_help_cwrite_4_fixupSP";
+   hAddr = __mem_help_cwrite_4_fixupSP;
+} else {
+   hName = "evh__mem_help_cwrite_4";
+   hAddr = __mem_help_cwrite_4;
+}

Please add a short comment somewhere, explaining the difference
between (eg) evh__mem_help_cwrite_4_fixupSP and evh__mem_help_cwrite_4.


+   the SP needed to unwind need to be fixed UP.

Did you mean "UP" and not "up"?


+static Bool check_cached_rcec_ok (Thr* thr, Addr previous_frame0)
+{

Is this just for debugging, or is it used in "normal" runs?  If it
is for normal runs, is it safe -- meaning, can it cause the run to
fail if some of the heuristics it uses are not quite right?

If it is just for debugging (which I am hoping), please add a comment to say
that.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=376257

--- Comment #6 from Philippe Waroquiers  ---
Ivo, thanks for looking at the patch.
I will fix this weird character (just a typo).

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=376257

--- Comment #5 from Ivo Raisr  ---
Nice work, Philippe!
I have just one question. In helgrind/hg_main.c, the last hunk,
there is a message containing a weird character: "re-àsetting it to 0".
Is this intended?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=376257

Philippe Waroquiers  changed:

   What|Removed |Added

 Attachment #103936|0   |1
is obsolete||

--- Comment #4 from Philippe Waroquiers  ---
Created attachment 108646
  --> https://bugs.kde.org/attachment.cgi?id=108646=edit
Implement a cached stack in helgrind V2

This version does not need any modification in VEX.

The code now detects that a SP fixup is needed by a simple heuristic in the
helgrind instrument function. The first version was doing a (somewhat similar)
heuristic based on a fixupSP property implemented in VEX, not needed anymore.

The previous version used a 'call property' added in VEX, to allow following
the 'chased call'. This version does not need this property anymore. Instead,
chasing is disabled when the cached stack is enabled.

Finally, a new option has been added, to activate (or not) this cached stack.
It is defaulted to yes on linux/amd64+x86. See user manual diff for more
details.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=376257

--- Comment #3 from Ivo Raisr  ---
Hi Philippe, that's ok and thank you for letting us know!

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=376257

Philippe Waroquiers  changed:

   What|Removed |Added

 CC||philippe.waroquiers@skynet.
   ||be

--- Comment #2 from Philippe Waroquiers  ---
(In reply to Ivo Raisr from comment #1)
> Hello Philippe, how are you doing with this patch? Have you been able to
> polish it more?
Hi Ivo, 
I see you are working hard to make a lot of things advance for the 3.13
release; Thanks for this hard work (a.o. the tilegx cleanup).
I am sorry but I had very low free time to work on Valgrind, so I was
not able to advance much on this. It will thus not make it for 3.13.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 376257] helgrind history full speed up using a cached stack

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=376257

Ivo Raisr  changed:

   What|Removed |Added

 CC||iv...@ivosh.net
 Status|UNCONFIRMED |CONFIRMED
 Ever confirmed|0   |1

--- Comment #1 from Ivo Raisr  ---
Hello Philippe, how are you doing with this patch? Have you been able to polish
it more?

-- 
You are receiving this mail because:
You are watching all bug changes.