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

[jingham at apple.com]

The function you write in the .s file is going to encode an ABI, so if you try to call it from a .c file, the compiler compiling that .c file has to follow that ABI.  So you could get into OS dependencies at that point.

But if you make a wrapper in the .s file that takes no arguments and returns nothing, then have that call whatever test code you are going to write in the .s file, you're probably OK.  It is possible to have two ABI's decide that they want to call no-arguments/no return functions differently on the same architecture, but that seems much less likely...

Jim


> On Jul 25, 2014, at 7:11 AM, Todd Fiala <tfiala at google.com> wrote:
> 
> >  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
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits