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

Todd Fiala tfiala at google.com
Fri Jul 25 12:38:23 PDT 2014 

Ok - thanks for all the details!

I think we'll take a swipe at it and mark it as Linux x86_64 only.  If it
happens to also work on MacOSX x86_64, I'll enable it there as well.

And if it just ends up not being practical to get it going at all, we will
just de-prioritize it.


On Fri, Jul 25, 2014 at 12:34 PM, Jason Molenda <jason at molenda.com> wrote:

> I think we have three problems with writing these tests:
>
> (1) A driver main.c file is needed which does all the process
> startup/shutdown and calls a test function with no arguments.  We want this
> to be in C so the compiler can generate the appropriate assembly for each
> platform.
>
> (2) The assembly will have some template things specific to an OS.  For
> instance, the symbol name needs to be prefixed with an underscore on a Mac
> ("_main:") unlike Linux ("main:") for external linkage visibility.  If it's
> just internal linkage, I think you can get away no-underscore on Mac.
>  Normally in Mac assembly you want to have a section directive and set
> alignment before your functions -- but it seems to work without those?
>  Anyway, it's clear that different systems will have slight variances on
> the template of the assembly that they need.
>
> (3) The assembly may have ABI specific details even for the same
> processor.  x86_64 is not a difficult one because Linux/BSD/Mac all use
> SysV ABI as a base with little deviance.  I have no idea what the windows
> ABI would look like. i386 is slightly trickier because Mac requires a
> 16-byte stack frame whereas the other unixes use 4-byte.  And things get
> even more different if we start to talk about ARMv7 or AArch64.  The main
> concern would be the C file to assembly file interface layer and a func
> call with no arguments is probably going to work for all ABIs.
>
>
> Given the stability of the assembly unwinders, I don't feel super
> motivated to add these tests, but it'd be a good addition if someone wants
> to do it.  The easiest would be to make a "Linux x86_64" specific test".  I
> think with a little fiddling it may be possible to make a "x86_64" specific
> test, probably passing the assembly through the C preprocessor to handle OS
> differences.
>
>
>
> On Jul 25, 2014, at 9:36 AM, Todd Fiala <tfiala at google.com> wrote:
>
> > 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
> >
>
>


-- 
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/4bed1f5a/attachment.html>

More information about the lldb-commits mailing list