<div dir="ltr">Ok - thanks for all the details!<div><br></div><div>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.<div>
<br></div><div>And if it just ends up not being practical to get it going at all, we will just de-prioritize it. </div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 25, 2014 at 12:34 PM, Jason Molenda <span dir="ltr"><<a href="mailto:jason@molenda.com" target="_blank">jason@molenda.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I think we have three problems with writing these tests:<br>
<br>
(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.<br>
<br>
(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.<br>
<br>
(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.<br>
<br>
<br>
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.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On Jul 25, 2014, at 9:36 AM, Todd Fiala <<a href="mailto:tfiala@google.com">tfiala@google.com</a>> wrote:<br>
<br>
> 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 ?<br>
><br>
><br>
> On Fri, Jul 25, 2014 at 9:04 AM, <<a href="mailto:jingham@apple.com">jingham@apple.com</a>> wrote:<br>
> 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.<br>
><br>
> 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...<br>
><br>
> Jim<br>
><br>
><br>
> > On Jul 25, 2014, at 7:11 AM, Todd Fiala <<a href="mailto:tfiala@google.com">tfiala@google.com</a>> wrote:<br>
> ><br>
> > > If you were to make them only run on one OS it would be pretty easy to do it with hand written assembly.<br>
> ><br>
> > 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?<br>
> ><br>
> > -Todd<br>
> ><br>
> ><br>
> > On Fri, Jul 25, 2014 at 3:03 AM, Jason Molenda <<a href="mailto:jason@molenda.com">jason@molenda.com</a>> wrote:<br>
> > 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.<br>
> ><br>
> > On Jul 24, 2014, at 10:26 PM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
> ><br>
> > > Can we add Tong's test case as an LLDB test?<br>
> > ><br>
> > ><br>
> > > On Thu, Jul 24, 2014 at 6:24 PM, Todd Fiala <<a href="mailto:tfiala@google.com">tfiala@google.com</a>> wrote:<br>
> > > Tested and committed:<br>
> > ><br>
> > > svn commit<br>
> > > Sending source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp<br>
> > > Transmitting file data .<br>
> > > Committed revision 213914.<br>
> > ><br>
> > ><br>
> > ><br>
> > > On Thu, Jul 24, 2014 at 5:35 PM, Todd Fiala <<a href="mailto:tfiala@google.com">tfiala@google.com</a>> wrote:<br>
> > > :-)<br>
> > ><br>
> > ><br>
> > > On Thu, Jul 24, 2014 at 5:34 PM, Tong Shen <<a href="mailto:endlessroad@google.com">endlessroad@google.com</a>> wrote:<br>
> > > Thanks Todd!<br>
> > ><br>
> > ><br>
> > > On Thu, Jul 24, 2014 at 5:33 PM, Todd Fiala <<a href="mailto:tfiala@google.com">tfiala@google.com</a>> wrote:<br>
> > > Hey Tong - I'll add that and do all the test runs. I'll get it checked in unless something breaks.<br>
> > ><br>
> > > -Todd<br>
> > ><br>
> > ><br>
> > > On Thu, Jul 24, 2014 at 5:32 PM, Tong Shen <<a href="mailto:endlessroad@google.com">endlessroad@google.com</a>> wrote:<br>
> > > Hi Jason,<br>
> > ><br>
> > > I don't have any strong preference here, so let's do it the way you see fit :-)<br>
> > > Your patch works fine, though there's a right bracket missing at the end of line:<br>
> > > regloc.SetAtCFAPlusOffset (-(stack_offset + row->GetCFAOffset());<br>
> > ><br>
> > > Thank you for looking into this!<br>
> > ><br>
> > ><br>
> > > On Thu, Jul 24, 2014 at 5:16 PM, Jason Molenda <<a href="mailto:jmolenda@apple.com">jmolenda@apple.com</a>> wrote:<br>
> > > 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.<br>
> > ><br>
> > > 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.<br>
> > ><br>
> > > 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.<br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > > On Jul 24, 2014, at 3:33 PM, Tong Shen <<a href="mailto:endlessroad@google.com">endlessroad@google.com</a>> wrote:<br>
> > > ><br>
> > > > Hi jmolenda, lldb-commits,<br>
> > > ><br>
> > > > While hacking around x86 assembly profiler, I found a problem about non-volatile register information.<br>
> > > ><br>
> > > > At UnwindAssembly-x86.cpp line 630 (ViewVC link), stack_offset is calculated but not used.<br>
> > > > I think it should be used in line 636:<br>
> > > > regloc.SetAtCFAPlusOffset (-row->GetCFAOffset() + stack_offset);<br>
> > > > instead of what's there now:<br>
> > > > regloc.SetAtCFAPlusOffset (-row->GetCFAOffset());<br>
> > > ><br>
> > > > Also, in line 417 of the same file, when calculating stack_offset, why is rbp_offset set to abs(offset)?<br>
> > > ><br>
> > > > For testing, I wrote an assembly (test.S in attachment) to test if lldb can recover non-volatile registers.<br>
> > > > You can put a breakpoint after where asm_frame() overrides %rbx, then do "f 1" & "register read" to see if %rbx is correctly restored.<br>
> > > > In my test, unmodified lldb gives wrong %rbx.<br>
> > > > Attached patch solved this problem, and make lldb recover those registers correctly.<br>
> > > ><br>
> > > > Am I missing anything here? Is the original behavior intentional? Please correct me if I'm wrong :-)<br>
> > > ><br>
> > > > Thank you.<br>
> > > ><br>
> > > > --<br>
> > > > Best Regards, Tong Shen<br>
> > > > <test.S><1.patch><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > Best Regards, Tong Shen<br>
> > ><br>
> > > _______________________________________________<br>
> > > lldb-commits mailing list<br>
> > > <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > Todd Fiala | Software Engineer | <a href="mailto:tfiala@google.com">tfiala@google.com</a> | <a href="tel:650-943-3180" value="+16509433180">650-943-3180</a><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > Best Regards, Tong Shen<br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > Todd Fiala | Software Engineer | <a href="mailto:tfiala@google.com">tfiala@google.com</a> | <a href="tel:650-943-3180" value="+16509433180">650-943-3180</a><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > Todd Fiala | Software Engineer | <a href="mailto:tfiala@google.com">tfiala@google.com</a> | <a href="tel:650-943-3180" value="+16509433180">650-943-3180</a><br>
> > ><br>
> > ><br>
> > > _______________________________________________<br>
> > > lldb-commits mailing list<br>
> > > <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
> > ><br>
> > ><br>
> > > _______________________________________________<br>
> > > lldb-commits mailing list<br>
> > > <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > --<br>
> > Todd Fiala | Software Engineer | <a href="mailto:tfiala@google.com">tfiala@google.com</a> | <a href="tel:650-943-3180" value="+16509433180">650-943-3180</a><br>
> ><br>
> > _______________________________________________<br>
> > lldb-commits mailing list<br>
> > <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Todd Fiala | Software Engineer | <a href="mailto:tfiala@google.com">tfiala@google.com</a> | <a href="tel:650-943-3180" value="+16509433180">650-943-3180</a><br>
><br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><table cellspacing="0" cellpadding="0" style="color:rgb(136,136,136);font-family:'Times New Roman'"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small">
<td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Todd Fiala |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td>
<td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a> |</td>
<td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"><font color="#1155cc"> <a>650-943-3180</a></font></td></tr></tbody></table><br></div>
</div>