https://bugs.kde.org/show_bug.cgi?id=404843
--- Comment #6 from Andreas Arnez <[email protected]> --- (In reply to Julian Seward from comment #5) > Created attachment 119229 [details] > Proposed fix > > For s390x, adds unwinding of f0..f7, so as to be able the handle the case > where GPRs are saved in caller-saved fp registers. Thanks! This certainly helps a lot. In my limited testing the unwinding seems to have worked consistently with this patch. Note that I've only tried GCC 8.0.1. A few comments: > [...] > --- a/coregrind/m_debuginfo/debuginfo.c > +++ b/coregrind/m_debuginfo/debuginfo.c > @@ -3199,6 +3199,15 @@ Addr ML_(get_CFA) ( Addr ip, Addr sp, Addr fp, > [...] > uregs.sp = sp; > uregs.fp = fp; > + /* JRS FIXME 3 Apr 2019: surely we can do better for f0..f7 */ > + uregs.f0 = 0; > + uregs.f1 = 0; I think we can skip the FPR initialization here. IIUC, we'd only need it if the CFA depended on an FPR. But this would be a very unusual compiler decision, because an FPR can't be used for addressing. I'd say we can rule that out. > [...] > @@ -3259,6 +3268,8 @@ void VG_(ppUnwindInfo) (Addr from, Addr to) > For arm, the unwound registers are: R7 R11 R12 R13 R14 R15. > > For arm64, the unwound registers are: X29(FP) X30(LR) SP PC. > + > + For s390, the unwound registers are: R11(FP) R14(LR) R15(SP) F0 F2 F4 … This doesn't match the current implementation, right? In fact it seems that F0-F7 are unwound, although F0, F2, and F4 are probably sufficient (I hope). Theoretically we could even avoid unwinding the FPRs at all; we just need them for the initial frame. Since all of F0-F7 are volatile, the caller can't really have saved anything there. By the way, the PC doesn't need to be unwound either, since unwound frames have PC == R14. > [...] > --- a/coregrind/m_debuginfo/storage.c > +++ b/coregrind/m_debuginfo/storage.c > @@ -138,7 +138,31 @@ void ML_(ppDiCfSI) ( const XArray* /* of CfiExpr */ e… > [...] > + VG_(printf)("oldF6"); \ > + } else \ > + if (_how == CFIR_S390X_F7) { \ > + VG_(printf)("oldF7"); \ > + } else{ \ Missing space. > [...] > --- a/coregrind/m_libcassert.c > +++ b/coregrind/m_libcassert.c > @@ -176,6 +176,15 @@ > [...] > (srP)->misc.S390X.r_fp = fp; \ > (srP)->misc.S390X.r_lr = lr; \ > + /* ANDREAS 3 Apr 2019 FIXME r_f0..r_f7 */ \ > + (srP)->misc.S390X.r_f0 = 0;/*FIXME*/ \ > + (srP)->misc.S390X.r_f1 = 0;/*FIXME*/ \ Let me look into this, I'll provide a suitable inline assembly. > [...] > --- a/coregrind/m_machine.c > +++ b/coregrind/m_machine.c > @@ -118,6 +118,23 @@ void VG_(get_UnwindStartRegs) ( /*OUT*/UnwindStartReg… > [...] > regs->misc.S390X.r_lr > = VG_(threads)[tid].arch.vex.guest_LR; > + /* ANDREAS 3 Apr 2019 FIXME r_f0..r_f7: is this correct? */ > + regs->misc.S390X.r_f0 > + = VG_(threads)[tid].arch.vex.guest_v0.w64[0]; Looks correct to me. The FPRs overlap with the lower-addressed halves of the respective vector registers. > [...] > --- a/coregrind/m_signals.c > +++ b/coregrind/m_signals.c > @@ -526,6 +526,15 @@ typedef struct SigQueue { > (srP)->r_sp = (ULong)((uc)->uc_mcontext.regs.gprs[15]); \ > (srP)->misc.S390X.r_fp = (uc)->uc_mcontext.regs.gprs[11]; \ > (srP)->misc.S390X.r_lr = (uc)->uc_mcontext.regs.gprs[14]; \ > + /* ANDREAS 3 Apr 2019 FIXME r_f0..r_f7: is this correct? */ \ > + (srP)->misc.S390X.r_f0 = (uc)->uc_mcontext.fpregs.fprs[0]; \ Looks OK. -- You are receiving this mail because: You are watching all bug changes.
