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

[Zachary Turner via lldb-commits]

On Tue, Feb 28, 2017 at 3:49 PM Jim Ingham <jingham at apple.com> wrote:

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

It seems like this is going to end up in a circular argument.  Testing
those changes happens automatically in LLVM, because there is good test
coverage.  So it's difficult to impossible to change logic in code without
testing the results, because everywhere is tested.  As a result, there is a
widespread mentality that "if the tests pass, I didn't break anything".
That is, in fact, the main benefit of having tests.

Especially when changing a function like StackFrame::Disassemble, you would
think that something, somewhere is testing disassembly right?  I mean we
can't expect everyone to have the entire code coverage of LLDB mapped out
in their heads, so it's reasonable to make assumptions, especially about
such core functionality as "is disassembly completely broken?"

We should not be adopting an exclusionary policy of gating changes to the
code based on how much code coverage there is or how much expertise you
have in a particular area.  We should instead be finding ways to make it
easier to contribute.



>
> >
> > 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.
>
Right, my point is just that tests are so all encompassing that if one
fails it often gives you no clues about the reason for the failure.
Because everything is a full scale integration test.  On the other hand,
that's the only type of test that it's practical to write.  We have some
gtests now, but it's still often very hard to write them because the system
is not modular enough.  Since A depends on B depends on C depends on D ad
infinitum, you can't even create an instance of A to test it without
spinning up 100 other things.

There should be a system in place that allows one to write a test that
tests *almost nothing* other than the specific functionality you are trying
to test.  You can still have the full scale tests, but you need a first
line of defense, and integration tests should not be it.  Anyway, this is
starting to sound like a discussion we've had many times before.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170301/19513a3b/attachment.html>