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

[Todd Fiala]

>  If you were to make them only run on one OS it would be pretty easy to
do it with hand written assembly.

Yeah I was planning on having that get added as a follow-up pass.  We'll
just have it qualified to only run locally and only when the host
architecture is what we need (which I think is x86_64 in this case).  I
think with the assembly it will be OS agnostic but architecture specific as
long as we get the build rules right?

-Todd


On Fri, Jul 25, 2014 at 3:03 AM, Jason Molenda <jason at molenda.com> wrote:

> It's a little tricky to write unwinder tests - they have to be written in
> assembly to set up conditions correctly and that makes them
> OS/ABI-specific.  A combination of a C driver program (with a main()
> function) and a .s file that main() calls might be possible to do for
> Mac/Linux/*BSD but I'm not sure about that.  If you were to make them only
> run on one OS it would be pretty easy to do it with hand written assembly.
>
> On Jul 24, 2014, at 10:26 PM, Zachary Turner <zturner at google.com> wrote:
>
> > 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
> >
> >
> > _______________________________________________
> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140725/9f29084e/attachment.html>