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

[Jim Ingham via lldb-commits]

> On Feb 28, 2017, at 3:36 PM, Zachary Turner <zturner at google.com> wrote:
> 
> That patch was not really about early returns, it was about using StringRef.  So my comment about the aesthetics was referring to the patch in general.  The early return was more of a drive by, since I was already touching the code.  In that respect I was just following the well documented and widely accepted LLVM policy.

I do not think that it is widely accepted LLVM policy to go changing logic in code you don't really understand without testing the results of that change.  It is much better to leave a little bit of code looking not quite like you would like it to be, than to add a bug to it.

> 
> Regarding testing, I honestly don't see a reasonable path forward regarding increasing test coverage until we have a better testing infrastructure in place that is less platform-dependent, less flaky, and makes it much much easier to write small, targeted tests.

That seems to me to be a cop-out.  Every time we've actually chased down a "flakey" test on OS X we've found the origin in some race condition in lldb - i.e. the test suite was not flakey, it was working quite correctly in showing somewhere that lldb was flakey.  There seem to still be instances of this - the multi debugger test case - another one of the "flakey" ones - turns out to be a race condition in accessing the section lists in the debugger.  I doubt you are going to get something that actually runs the debugger up to some point and then does testing that is significantly simpler than the current test suite.  And I really doubt that the effort of doing that would end up having more benefit than adding tests to the current suite. 

And come on, these test are really not that hard to write.  For instance, here's the totality of an inline test:

 > cat TestDumpDynamic.py
from __future__ import absolute_import

from lldbsuite.test import lldbinline

lldbinline.MakeInlineTest(
    __file__, globals(), [
        lldbinline.expectedFailureAll(
            oslist=["windows"], bugnumber="llvm.org/pr24663")])

 cat main.cpp
//===-- main.cpp ------------------------------------------------*- C++ -*-===//
//
//                     The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//

class Base {
public:
  Base () = default;
  virtual int func() { return 1; }
  virtual ~Base() = default;
};

class Derived : public Base {
private:
  int m_derived_data;
public:
  Derived () : Base(), m_derived_data(0x0fedbeef) {}
  virtual ~Derived() = default;
  virtual int func() { return m_derived_data; }
};

int main (int argc, char const *argv[])
{
  Base *base = new Derived();
    return 0; //% stream = lldb.SBStream()
    //% base = self.frame().FindVariable("base")
    //% base.SetPreferDynamicValue(lldb.eDynamicDontRunTarget)
    //% base.GetDescription(stream)
    //% if self.TraceOn(): print(stream.GetData())
    //% self.assertTrue(stream.GetData().startswith("(Derived *"))
}

And for smaller components that don't rely on having parts of the debugger up and cooperating I thought you were happy with your gtests.  Is that not true, is there something wrong with the gtests that I've missed?  I have only written a few, but they seemed fine to me.

Jim


> 
> On Tue, Feb 28, 2017 at 3:27 PM Jim Ingham <jingham at apple.com> wrote:
> 
> > On Feb 28, 2017, at 3:14 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda <jmolenda at apple.com> wrote:
> > 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.
> >
> > Just for the record, I disagree with this assertion that there is little gain beyond aesthetics (as does I think almost everyone else in the LLVM/LLDB community).
> 
> 
> No doubt, early returns makes it easier to reason about complicated code.  But you added an early return to a function that had maybe 10 lines of code in it and was trivial to read either way.  There was pretty much zero chance somebody working on the code before this change would introduce a bug that they wouldn't because of the clarity provided by the early return.  But in doing so you DID add a bug.  In this case it seems clear that for the sake of very little more than aesthetics, you introduced a bug.  That seems to me a very poor tradeoff.
> 
> BTW, somebody at Apple tried to get the llvm version of gcov working on the lldb testsuite to see what kind of coverage we actually had.  It didn't work right off the bat for reasons that weren't clear, and whoever did the initial effort lost the window of time they had to work on this.  But that would be a useful exercise; then you could know whether the code you were touching was already well tested.  Then we could gate any of these sorts of formal manipulations on there being adequate test coverage of the affected area in advance of that work.
> 
> 
> Jim
>