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

[Todd Fiala]

Ok - maybe we write it up as ABI-agnostic as we can, and if we find
combinations that fail the test because of ABI-isms that creep in, then we
disable on those combinations at that time ?


On Fri, Jul 25, 2014 at 9:04 AM, <jingham at apple.com> wrote:

> 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
>
>


-- 
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/6acc3557/attachment.html>