[Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.

Tue Feb 28 15:06:44 PST 2017

Writing SB API tests for the disassembler is easy, as Jim notes there is an SBFrame::Disassemble() method that maps directly on to this call.

I've written unit tests that create a disassembler -- I do it for the unwind tests.  It's easy when you have an array of bytes to feed directly into the disassembler.  Writing a unit test for StackFrame::Disassemble is a little trickier because the StackFrame has a reference to the thread which has a reference to the process.  And I'm guessing we use the StackFrame's SymbolContext ivar to find the bounds of the function and read the bytes out of the target or process to feed to the disassembler.  That's a lot of classes you'd need to set up to write a unit test for this function.  But SB API would be easy.

The complicated thing to test is UnwindLLDB / RegisterContextLLDB.  They operate on a stopped process and have stack memory, register context, and information about all of the stack frames from the unwind plan importer methods.  It is easy to write an SB API test that exercises these code paths (SBThread::GetNumFrames() will force a simple stack walk for a thread) but you want to write very specific functions / prologues / epilogues / stack content and you're basically writing assembly code to do it correctly, with a C wrapper program around the assembly frame of interest.  I wrote a testsuite in this form for a previous unwinder I wrote, it works great, at least for a single platform (e.g. macos).

Writing a unit test for UnwindLLDB / RegisterContextLLDB would be really great but you need a way to fake up an entire process stopped at a point.  I've had ideas about creating a ProcessMock class which could read heap/stack/thread/register context information from a file.  But it's not sufficient by itself - you need an ObjectFileMock where you can interject symbols and the text of those functions as well.  I think it's an interesting idea that I'd like to pursue, but I won't have the time to work on this any time soon.  Even more exciting would be a ProcessMock that could represent multiple program contexts so you could "step" in a ProcessMock.  And an automated way to gather all the information lldb touches while seeing an example problem and automatically generating the ProcessMock/ObjectFileMock information so it could be reproduced later?  Hmm!

And ProcessMock/ObjectFileMock wouldn't just be limited to unit tests, SB API tests could be written against this repo environment too.

Anyway, I'm getting off topic.  There's no reason why this function can't be tested.  We could discuss whether it's a good idea to rewrite sections of code that have little or no testing because it's always a risk, and the payoff for making those changes must be commensurate (beyond "change the style") with that risk.  At it's core, lldb is a real world tool that thousands of people depend on; breaking it or introducing bugs for little gain beyond aesthetics is a very poor tradeoff.  But once the function has been broken, in addition to fixing it, clearly it's incumbent on any of us to do the right thing and write a test so it doesn't happen again.

> On Feb 28, 2017, at 10:17 AM, Zachary Turner via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> 
> I'm not even sure how to exercise this code path.  Granted, the reason it broke at all is because of no test case, but I feel like someone who understands this code should probably prepare a test case for it.  (I know in the past Jason has said that it was notoriously hard to write test cases for disassembler)  
> 
> On Tue, Feb 28, 2017 at 10:13 AM Jim Ingham <jingham at apple.com> wrote:
> No test case?
> 
> Jim
> 
> > On Feb 28, 2017, at 9:59 AM, Zachary Turner via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> >
> > Author: zturner
> > Date: Tue Feb 28 11:59:59 2017
> > New Revision: 296495
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
> > Log:
> > Fix incorrect logic in StackFrame::Disassemble.
> >
> > This had broken as the result of some previous cleanup.
> >
> > Modified:
> >    lldb/trunk/source/Target/StackFrame.cpp
> >
> > Modified: lldb/trunk/source/Target/StackFrame.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
> > ==============================================================================
> > --- lldb/trunk/source/Target/StackFrame.cpp (original)
> > +++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
> > @@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
> >
> > const char *StackFrame::Disassemble() {
> >   std::lock_guard<std::recursive_mutex> guard(m_mutex);
> > -  if (m_disassembly.Empty())
> > -    return nullptr;
> > -
> > -  ExecutionContext exe_ctx(shared_from_this());
> > -  Target *target = exe_ctx.GetTargetPtr();
> > -  if (target) {
> > -    const char *plugin_name = nullptr;
> > -    const char *flavor = nullptr;
> > -    Disassembler::Disassemble(target->GetDebugger(), target->GetArchitecture(),
> > -                              plugin_name, flavor, exe_ctx, 0, false, 0, 0,
> > -                              m_disassembly);
> > +  if (m_disassembly.Empty()) {
> > +    ExecutionContext exe_ctx(shared_from_this());
> > +    Target *target = exe_ctx.GetTargetPtr();
> > +    if (target) {
> > +      const char *plugin_name = nullptr;
> > +      const char *flavor = nullptr;
> > +      Disassembler::Disassemble(target->GetDebugger(),
> > +                                target->GetArchitecture(), plugin_name, flavor,
> > +                                exe_ctx, 0, false, 0, 0, m_disassembly);
> > +    }
> > +    if (m_disassembly.Empty())
> > +      return nullptr;
> >   }
> > +
> >   return m_disassembly.GetData();
> > }
> >
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits