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

[Zachary Turner]

Can we add Tong's test case as an LLDB test?


On Thu, Jul 24, 2014 at 6:24 PM, Todd Fiala <tfiala at google.com> wrote:

> 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
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140724/708ca6cc/attachment.html>