Created attachment 103936 [details] VEX and valgrind/helgrind modifications for helgrind cached stack Attached patch implements the optimisation discussed at FOSDEM2017. See slides at https://fosdem.org/2017/schedule/event/valgrind_optimizations/attachments/slides/1820/export/events/attachments/valgrind_optimizations/slides/1820/fosdem2017_optim_xt_hg.odp for some background information. For sure, patch needs some more work before it can be committed.
Hello Philippe, how are you doing with this patch? Have you been able to polish it more?
(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.
Hi Philippe, that's ok and thank you for letting us know!
Created attachment 108646 [details] 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.
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?
Ivo, thanks for looking at the patch. I will fix this weird character (just a typo).
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.
(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.