[valgrind] [Bug 376257] helgrind history full speed up using a cached stack
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
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 = &evh__mem_help_cwrite_4_fixupSP;
> +} else {
> + hName = "evh__mem_help_cwrite_4";
> + hAddr = &evh__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
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 = &evh__mem_help_cwrite_4_fixupSP;
+} else {
+ hName = "evh__mem_help_cwrite_4";
+ hAddr = &evh__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
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
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
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&action=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
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
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
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.
