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

[Tong Shen]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140724/a0fbca3b/attachment.html>