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

[Jason Molenda via lldb-commits]

It makes me uncomfortable to have these whole-project-ranging change sets going into the source, one example being const char * -> StringRef) for such small benefit.  Yes there were a few methods where we were doing string parsing and tokenizing which became much smaller and easier to understand when switched to StringRef.  But then the additional step of changing the entire codebase over to use StringRef was very hard for me to understand -- it's months of time, it's introducing bugs (some of which we've caught!  Some of which I'm sure we haven't), and it leads to no benefit except that we're using a different model for passing strings around.  I think there was discussion about how it might make the command line parser in lldb faster or more efficient, but that's a non-goal; if someone can show me the time for lldb to parse "lldb --attach-name a.out" takes a human detectable amount of time I'd be really interested to see it.

When I am working in a method I'll often update the method to current best practices in the process of my necessary changes.  I think this is a good use of time and it allows me to roll in the testing of my overall updates and the fixes I'm doing at the same time, both by testsuite and by manual testing.

You mentioned that this kind of mechanical churn of the code is something everyone else in the LLVM/LLDB community supports.  I don't know about the LLVM community, but I strongly argue that this is the wrong way to develop a stable, reliable product.  I know I'm on the conservative side here, but these kinds of changes add no value to the product and add plenty of risk as they're being done while everyone is half paying attention to them going in.  Maybe if there was a vastly more extensive testsuite we could make large scale changes without fear of breaking the debugger, but we don't have that, no one has had the time to add one yet, and it's definitely something that we need to be putting energy into.  We don't need to be putting energy into switching to a different string representation across the board simply because it shows benefits in a handful of methods.


As for the testsuite not being stable -- it's the debugger that isn't stable, not the testsuite.  Jim mentioned that the multi process debugger test case is showing a SectionLoadList race condition that we need to fix.  It also turned up the UnwindPlan locking bug that I fixed last week.  I'm working on another problem right now where the C++ formatters fail ~15% of the time in the testsuite.  The testsuite isn't the bug; lldb has the bug, the testsuite is showing us that it is happening.




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