Tested and committed: svn commit Sending source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp Transmitting file data . Committed revision 213914.
On Thu, Jul 24, 2014 at 5:35 PM, Todd Fiala <tfi...@google.com> wrote: > :-) > > > On Thu, Jul 24, 2014 at 5:34 PM, Tong Shen <endlessr...@google.com> wrote: > >> Thanks Todd! >> >> >> On Thu, Jul 24, 2014 at 5:33 PM, Todd Fiala <tfi...@google.com> wrote: >> >>> Hey Tong - I'll add that and do all the test runs. I'll get it checked >>> in unless something breaks. >>> >>> -Todd >>> >>> >>> On Thu, Jul 24, 2014 at 5:32 PM, Tong Shen <endlessr...@google.com> >>> wrote: >>> >>>> Hi Jason, >>>> >>>> I don't have any strong preference here, so let's do it the way you see >>>> fit :-) >>>> Your patch works fine, though there's a right bracket missing at the >>>> end of line: >>>> regloc.SetAtCFAPlusOffset (-(stack_offset + row->GetCFAOffset()); >>>> >>>> Thank you for looking into this! >>>> >>>> >>>> On Thu, Jul 24, 2014 at 5:16 PM, Jason Molenda <jmole...@apple.com> >>>> wrote: >>>> >>>>> Good catch Tong. I don't think clang generates this instruction >>>>> pattern (it normally pushq's all the registers in the prologue that it >>>>> wants to preserve) so we've never hit this bug. >>>>> >>>>> I don't have a strong preference about >>>>> mov_reg_to_local_stack_frame_p() returning a positive value in rbp_offset >>>>> except that we have other sections returning an offset from the CFA e.g. >>>>> UnwindPlan::Row::GetCFAOffset() which return a positive offset value that >>>>> must be subtracted from the CFA to get the correct address. >>>>> >>>>> If you would prefer to change mov_reg_to_local_stack_frame_p() to >>>>> return a negative value, I'm not going to argue against it. But maybe a >>>>> slight change to your patch like this? It's complicated enough code that >>>>> I >>>>> added a couple of comments while I was at it. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> > On Jul 24, 2014, at 3:33 PM, Tong Shen <endlessr...@google.com> >>>>> wrote: >>>>> > >>>>> > Hi jmolenda, lldb-commits, >>>>> > >>>>> > While hacking around x86 assembly profiler, I found a problem about >>>>> non-volatile register information. >>>>> > >>>>> > At UnwindAssembly-x86.cpp line 630 (ViewVC link), stack_offset is >>>>> calculated but not used. >>>>> > I think it should be used in line 636: >>>>> > regloc.SetAtCFAPlusOffset (-row->GetCFAOffset() + >>>>> stack_offset); >>>>> > instead of what's there now: >>>>> > regloc.SetAtCFAPlusOffset (-row->GetCFAOffset()); >>>>> > >>>>> > Also, in line 417 of the same file, when calculating stack_offset, >>>>> why is rbp_offset set to abs(offset)? >>>>> > >>>>> > For testing, I wrote an assembly (test.S in attachment) to test if >>>>> lldb can recover non-volatile registers. >>>>> > You can put a breakpoint after where asm_frame() overrides %rbx, >>>>> then do "f 1" & "register read" to see if %rbx is correctly restored. >>>>> > In my test, unmodified lldb gives wrong %rbx. >>>>> > Attached patch solved this problem, and make lldb recover those >>>>> registers correctly. >>>>> > >>>>> > Am I missing anything here? Is the original behavior intentional? >>>>> Please correct me if I'm wrong :-) >>>>> > >>>>> > Thank you. >>>>> > >>>>> > -- >>>>> > Best Regards, Tong Shen >>>>> > <test.S><1.patch> >>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Best Regards, Tong Shen >>>> >>>> _______________________________________________ >>>> lldb-commits mailing list >>>> lldb-commits@cs.uiuc.edu >>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >>>> >>>> >>> >>> >>> -- >>> Todd Fiala | Software Engineer | tfi...@google.com | 650-943-3180 >>> >> >> >> >> -- >> Best Regards, Tong Shen >> > > > > -- > Todd Fiala | Software Engineer | tfi...@google.com | 650-943-3180 > -- Todd Fiala | Software Engineer | tfi...@google.com | 650-943-3180
_______________________________________________ lldb-commits mailing list lldb-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits