[Lldb-commits] Possible UnwindAssembly-x86 Problem & Fix

[Todd Fiala]

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 <tfiala at google.com> wrote:

> :-)
>
>
> On Thu, Jul 24, 2014 at 5:34 PM, Tong Shen <endlessroad at google.com> wrote:
>
>> Thanks Todd!
>>
>>
>> On Thu, Jul 24, 2014 at 5:33 PM, Todd Fiala <tfiala at 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 <endlessroad at 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 <jmolenda at 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 <endlessroad at 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 at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>>
>>>>
>>>
>>>
>>> --
>>> Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>>>
>>
>>
>>
>> --
>> Best Regards, Tong Shen
>>
>
>
>
> --
> Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>



-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140724/f5fd84ae/attachment.html>