LLDB Evolution - Final Form

Following up with Kate’s post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I’ve been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

  1. De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is similar to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I’ve identified. This list is by no means complete. For each one, I’ve given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:

  2. Use llvm::Regex instead of lldb::Regex

  3. llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?

  4. Risk: 6

  5. Impact: 3

  6. Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

  7. Use llvm streams instead of lldb::StreamString

  8. Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.

  9. Interoperates nicely with many existing llvm utility classes

  10. Risk: 4

  11. Impact: 5

  12. Difficulty / Effort: 7

  13. Use llvm::Error instead of lldb::Error

  14. llvm::Error is an error class that requires you to check whether it succeeded or it will assert. In a way, it’s similar to a C++ exception, except that it doesn’t come with the performance hit associated with exceptions. It’s extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.

  15. Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.

  16. Risk: 7

  17. Impact: 7

  18. Difficulty / Effort: 8

  19. StringRef instead of const char *, len everywhere

  20. Can do most common string operations in a way that is guaranteed to be safe.

  21. Reduces string manipulation algorithm complexity by an order of magnitude.

  22. Can potentially eliminate tens of thousands of string copies across the codebase.

  23. Simplifies code.

  24. Risk: 3

  25. Impact: 8

  26. Difficulty / Effort: 7

  27. ArrayRef instead of const void *, len everywhere

  28. Same analysis as StringRef

  29. MutableArrayRef instead of void *, len everywhere

  30. Same analysis as StringRef

  31. Delete ConstString, use a modified StringPool that is thread-safe.

  32. StringPool is a non thread-safe version of ConstString.

  33. Strings are internally refcounted so they can be cleaned up when they are no longer used. ConstStrings are a large source of memory in LLDB, so ref-counting and removing stale strings has the potential to be a huge savings.

  34. Risk: 2

  35. Impact: 9

  36. Difficulty / Effort: 6

  37. thread_local instead of lldb::ThreadLocal

  38. This fixes a number of bugs on Windows that cannot be fixed otherwise, as they require compiler support.

  39. Some other compilers may not support this yet?

  40. Risk: 2

  41. Impact: 3

  42. Difficulty: 3

  43. Use llvm::cl for the command line arguments to the primary lldb executable.

  44. Risk: 2

  45. Impact: 3

  46. Difficulty / Effort: 4

  47. Testing - Our testing infrastructure is unstable, and our test coverage is lacking. We should take steps to improve this.

  48. Port as much as possible to lit

  49. Simple tests should be trivial to port to lit today. If nothing else this serves as a proof of concept while increasing the speed and stability of the test suite, since lit is a more stable harness.

  50. Separate testing tools

  51. One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

I don’t take full credit for this idea. I had been toying with a similar idea for some time, but it was further cemented in an offline discussion with a co-worker.

There many small, targeted tools in LLVM (e.g. llc, lli, llvm-objdump, etc) whose purpose are to be chained together to do interesting things. Instead of a command line api as we think of in LLDB where you type commands from an interactive prompt, they have a command line api as you would expect from any tool which is launched from a shell.

I can imagine many potential candidates for lldb tools of this nature. Off the top of my head:

  1. lldb-unwind - A tool for testing the unwinder. Accepts byte code as input and passes it through to the unwinder, outputting a compressed summary of the steps taken while unwinding, which could be pattern matched in lit. The output format is entirely controlled by the tool, and not by the unwinder itself, so it would be stable in the face of changes to the underlying unwinder. Could have various options to enable or disable features of the unwinder in order to force the unwinder into modes that can be tricky to encounter in the wild.

  2. lldb-symbol - A tool for testing symbol resolution. Could have options for testing things like:

  3. Determining if a symbol matches an executable

  4. looking up a symbol by name in the debug info, and mapping it to an address in the process.

  5. Displaying candidate symbols when doing name lookup in a particular scope (e.g. while stopped at a breakpoint).

  6. lldb-breakpoint - A tool for testing breakpoints and stepping. Various options could include:

  7. Set breakpoints and out addresses and/or symbol names where they were resolved to.

  8. Trigger commands, so that when a breakpoint is hit the tool could automatically continue and try to run to another breakpoint, etc.

  9. options to inspect certain useful pieces of state about an inferior, to be matched in lit.

  10. lldb-interpreter - tests the jitter etc. I don’t know much about this, but I don’t see why this couldn’t be tested in a manner similar to how lli is tested.

  11. lldb-platform - tests lldb local and remote platform interfaces.

  12. lldb-cli – lldb interactive command line.

  13. lldb-format - lldb data formatters etc.

  14. Tests NOW, not later.

  15. I know we’ve been over this a million times and it’s not worth going over the arguments again. And I know it’s hard to write tests, often requiring the invention of new SB APIs. Hopefully those issues will be addressed by above a) and b) above and writing tests will be easier. Vedant Kumar ran some analytics on the various codebases and found that LLDB has the lowest test / commit ratio of any LLVM project (He didn’t post numbers for lld, so I’m not sure what it is there).

  16. lldb: 287 of the past 1000 commits

  17. llvm: 511 of the past 1000 commits

  18. clang: 622 of the past 1000 commits

  19. compiler-rt: 543 of the past 1000 commits

This is an alarming statistic, and I would love to see this number closer to 50%.

  1. Code style / development conventions - Aside from just the column limitations and bracing styles, there are other areas where LLDB differs from LLVM on code style. We should continue to adopt more of LLVM’s style where it makes sense. I’ve identified a couple of areas (incomplete list) which I outline below.

  2. Clean up the mess of cyclical dependencies and properly layer the libraries. This is especially important for things like lldb-server that need to link in as little as possible, but regardless it leads to a more robust architecture, faster build and link times, better testability, and is required if we ever want to do a modules build of LLDB

  3. Use CMake instead of Xcode project (CMake supports Frameworks). CMake supports Apple Frameworks, so the main roadblock to getting this working is just someone doing it. Segmenting the build process by platform doesn’t make sense for the upstream, especially when there is a perfectly workable solution. I have no doubt that the resulting Xcode workspace generated automatically by CMake will not be as “nice” as one that is maintained by hand. We face this problem with Visual Studio on Windows as well. The solution that most people have adopted is to continue using the IDE for code editing and debugging, but for actually running the build, use CMake with Ninja. A similar workflow should still be possible with an OSX CMake build, but as I do not work every day on a Mac, all I can say is that it’s possible, I have no idea how impactful it would be on peoples’ workflows.

  4. Variable naming conventions

  5. I don’t expect anyone is too fond of LLDB’s naming conventions, but if we’re committed to joining the LLVM ecosystem, then let’s go all the way.

  6. Use more modern C++ and less C

  7. Old habits die hard, but this isn’t just a matter of style. It leads to safer, more robust, and less fragile code as well.

  8. Shorter functions and classes with more narrowly targeted responsibilities

  9. It’s not uncommon to find functions that are hundreds (and in a few cases even 1,000+) of lines long. We really need to be better about breaking functions and classes down into smaller responsibilities. This helps not just for someone coming in to read the function, but also for testing. Smaller functions are easier to unit test.

  10. Convert T foo(X, Y, Error &error) functions to Expected foo(X, Y) style (Depends on 1.c)

  11. llvm::Expected is based on the llvm::Error class described earlier. It’s used when a function is supposed to return a value, but it could fail. By packaging the error with the return value, it’s impossible to have a situation where you use the return value even in case of an error, and because llvm::Error has mandatory checking, it’s also impossible to have a sitaution where you don’t check the error. So it’s very safe.

Whew. That was a lot. If you made it this far, thanks for reading!

Obviously if we were to embark on all of the above, it would take many months to complete everything. So I’m not proposing anyone stop what they’re doing to work on this. This is just my own personal wishlist

I’ll only comment on the stuff that affects me.

  1. Use llvm streams instead of lldb::StreamString

  2. Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.

  3. Interoperates nicely with many existing llvm utility classes

  4. Risk: 4

  5. Impact: 5

  6. Difficulty / Effort: 7

I don’t like that llvm’s stringstream needs to be babied to make it produce its string. You have to wrap it and then flush it and then read the string. Maybe a subclass could be made that wraps its own string and flushes automatically on read?

  1. Use llvm::Error instead of lldb::Error

  2. llvm::Error is an error class that requires you to check whether it succeeded or it will assert. In a way, it’s similar to a C++ exception, except that it doesn’t come with the performance hit associated with exceptions. It’s extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.

  3. Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.

  4. Risk: 7

  5. Impact: 7

  6. Difficulty / Effort: 8

This would help me a lot if the underlying Clang APIs implemented it. Unfortunately the expression parser mainly just receives diagnostics and has to deal with them.
From the perspective of internal use, I could live with llvm::Error, particularly if it gets us 3f.

  1. ArrayRef instead of const void *, len everywhere

  2. Same analysis as StringRef

  1. MutableArrayRef instead of void *, len everywhere

  2. Same analysis as StringRef

At one point I made a templated mixin class that would make everything that vended GetNum*/Get*AtIndex functions also vend an iterator. Hopefully we can get that implemented everywhere.

  1. Testing - Our testing infrastructure is unstable, and our test coverage is lacking. We should take steps to improve this.

  2. Port as much as possible to lit

  3. Simple tests should be trivial to port to lit today. If nothing else this serves as a proof of concept while increasing the speed and stability of the test suite, since lit is a more stable harness.

  4. Separate testing tools

  5. One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

I don’t take full credit for this idea. I had been toying with a similar idea for some time, but it was further cemented in an offline discussion with a co-worker.

There many small, targeted tools in LLVM (e.g. llc, lli, llvm-objdump, etc) whose purpose are to be chained together to do interesting things. Instead of a command line api as we think of in LLDB where you type commands from an interactive prompt, they have a command line api as you would expect from any tool which is launched from a shell.

I can imagine many potential candidates for lldb tools of this nature. Off the top of my head:

  1. lldb-unwind - A tool for testing the unwinder. Accepts byte code as input and passes it through to the unwinder, outputting a compressed summary of the steps taken while unwinding, which could be pattern matched in lit. The output format is entirely controlled by the tool, and not by the unwinder itself, so it would be stable in the face of changes to the underlying unwinder. Could have various options to enable or disable features of the unwinder in order to force the unwinder into modes that can be tricky to encounter in the wild.

  2. lldb-symbol - A tool for testing symbol resolution. Could have options for testing things like:

  3. Determining if a symbol matches an executable

  4. looking up a symbol by name in the debug info, and mapping it to an address in the process.

  5. Displaying candidate symbols when doing name lookup in a particular scope (e.g. while stopped at a breakpoint).

  6. lldb-breakpoint - A tool for testing breakpoints and stepping. Various options could include:

  7. Set breakpoints and out addresses and/or symbol names where they were resolved to.

  8. Trigger commands, so that when a breakpoint is hit the tool could automatically continue and try to run to another breakpoint, etc.

  9. options to inspect certain useful pieces of state about an inferior, to be matched in lit.

  10. lldb-interpreter - tests the jitter etc. I don’t know much about this, but I don’t see why this couldn’t be tested in a manner similar to how lli is tested.

  11. lldb-platform - tests lldb local and remote platform interfaces.

  12. lldb-cli – lldb interactive command line.

  13. lldb-format - lldb data formatters etc.

I’m thinking of re-implementing the “inline” tester as a C++ tool that could be driven by lit.
As far as anything touching the expression parser, I’d prefer just implementing solid testers inside llvm/clang, rather than something on top of LLDB.

  1. Tests NOW, not later.

  2. I know we’ve been over this a million times and it’s not worth going over the arguments again. And I know it’s hard to write tests, often requiring the invention of new SB APIs. Hopefully those issues will be addressed by above a) and b) above and writing tests will be easier. Vedant Kumar ran some analytics on the various codebases and found that LLDB has the lowest test / commit ratio of any LLVM project (He didn’t post numbers for lld, so I’m not sure what it is there).

  3. lldb: 287 of the past 1000 commits

  4. llvm: 511 of the past 1000 commits

  5. clang: 622 of the past 1000 commits

  6. compiler-rt: 543 of the past 1000 commits

This is an alarming statistic, and I would love to see this number closer to 50%.

I’m definitely a culprit here. Many fixes I make that don’t have tests are for problems we can’t reproduce. In general, LLDB suffers from a lack of “failure tests” which check what happens when something doesn’t work.
IMO a very useful avenue for investigation would be to add various selectable “failure generators” into LLDB that simulate the failure of particular operations or subsystems. Then we could use these as proxies for failures we can’t reproduce as integration tests.

  1. Code style / development conventions - Aside from just the column limitations and bracing styles, there are other areas where LLDB differs from LLVM on code style. We should continue to adopt more of LLVM’s style where it makes sense. I’ve identified a couple of areas (incomplete list) which I outline below.

  2. Clean up the mess of cyclical dependencies and properly layer the libraries. This is especially important for things like lldb-server that need to link in as little as possible, but regardless it leads to a more robust architecture, faster build and link times, better testability, and is required if we ever want to do a modules build of LLDB

  3. Use CMake instead of Xcode project (CMake supports Frameworks). CMake supports Apple Frameworks, so the main roadblock to getting this working is just someone doing it. Segmenting the build process by platform doesn’t make sense for the upstream, especially when there is a perfectly workable solution. I have no doubt that the resulting Xcode workspace generated automatically by CMake will not be as “nice” as one that is maintained by hand. We face this problem with Visual Studio on Windows as well. The solution that most people have adopted is to continue using the IDE for code editing and debugging, but for actually running the build, use CMake with Ninja. A similar workflow should still be possible with an OSX CMake build, but as I do not work every day on a Mac, all I can say is that it’s possible, I have no idea how impactful it would be on peoples’ workflows.

My dev workflow these days is running xcodebuild from vim. I would love to be running ninja.

  1. Convert T foo(X, Y, Error &error) functions to Expected foo(X, Y) style (Depends on 1.c)

  2. llvm::Expected is based on the llvm::Error class described earlier. It’s used when a function is supposed to return a value, but it could fail. By packaging the error with the return value, it’s impossible to have a situation where you use the return value even in case of an error, and because llvm::Error has mandatory checking, it’s also impossible to have a sitaution where you don’t check the error. So it’s very safe.

Marshaling Error objects around is a regular aggravation and makes my code more messy. I welcome with open arms anything that reduces that mess.

You do have to wrap it. But if you call llvm::raw_string_ostream::str(), it will internally flush before it returns the thing. So if you write:

std::string s;
llvm::raw_string_ostream stream(s);
stream << “foo”;
return stream.str();

then the code will be correct. It’s still a bit more annoying then if the string were automatically part of the stream though, so there’s probably a clean way to design a version of it that owns the string.

A few remarks

Following up with Kate’s post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I’ve been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

  1. De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is similar to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I’ve identified. This list is by no means complete. For each one, I’ve given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:

  2. Use llvm::Regex instead of lldb::Regex

  3. llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?

  4. Risk: 6

  5. Impact: 3

  6. Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

  7. Use llvm streams instead of lldb::StreamString

  8. Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.

  9. Interoperates nicely with many existing llvm utility classes

  10. Risk: 4

  11. Impact: 5

  12. Difficulty / Effort: 7

  13. Use llvm::Error instead of lldb::Error

  14. llvm::Error is an error class that requires you to check whether it succeeded or it will assert.

I assume that assertion would be stripped in Release builds?
We have our own lldbassert() macro currently, which assert()s in Debug mode, but in Release mode produces an error message and continues
It would be great if llvm::Error allowed us to plug that in…

  1. In a way, it’s similar to a C++ exception, except that it doesn’t come with the performance hit associated with exceptions. It’s extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.

  2. Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.

  3. Risk: 7

  4. Impact: 7

  5. Difficulty / Effort: 8

  6. StringRef instead of const char *, len everywhere

  7. Can do most common string operations in a way that is guaranteed to be safe.

  8. Reduces string manipulation algorithm complexity by an order of magnitude.

  9. Can potentially eliminate tens of thousands of string copies across the codebase.

  10. Simplifies code.

  11. Risk: 3

  12. Impact: 8

  13. Difficulty / Effort: 7

  14. ArrayRef instead of const void *, len everywhere

  15. Same analysis as StringRef

  16. MutableArrayRef instead of void *, len everywhere

  17. Same analysis as StringRef

I don’t think we have a lot of those - IIRC, it’s mostly in the SB API where SWIG is supposed to map it back to a Python string

  1. Delete ConstString, use a modified StringPool that is thread-safe.

  2. StringPool is a non thread-safe version of ConstString.

  3. Strings are internally refcounted so they can be cleaned up when they are no longer used. ConstStrings are a large source of memory in LLDB, so ref-counting and removing stale strings has the potential to be a huge savings.

  4. Risk: 2

  5. Impact: 9

  6. Difficulty / Effort: 6

  7. thread_local instead of lldb::ThreadLocal

  8. This fixes a number of bugs on Windows that cannot be fixed otherwise, as they require compiler support.

  9. Some other compilers may not support this yet?

  10. Risk: 2

  11. Impact: 3

  12. Difficulty: 3

  13. Use llvm::cl for the command line arguments to the primary lldb executable.

  14. Risk: 2

  15. Impact: 3

  16. Difficulty / Effort: 4

  17. Testing - Our testing infrastructure is unstable, and our test coverage is lacking. We should take steps to improve this.

  18. Port as much as possible to lit

  19. Simple tests should be trivial to port to lit today. If nothing else this serves as a proof of concept while increasing the speed and stability of the test suite, since lit is a more stable harness.

  20. Separate testing tools

  21. One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

I don’t take full credit for this idea. I had been toying with a similar idea for some time, but it was further cemented in an offline discussion with a co-worker.

There many small, targeted tools in LLVM (e.g. llc, lli, llvm-objdump, etc) whose purpose are to be chained together to do interesting things. Instead of a command line api as we think of in LLDB where you type commands from an interactive prompt, they have a command line api as you would expect from any tool which is launched from a shell.

I can imagine many potential candidates for lldb tools of this nature. Off the top of my head:

  1. lldb-unwind - A tool for testing the unwinder. Accepts byte code as input and passes it through to the unwinder, outputting a compressed summary of the steps taken while unwinding, which could be pattern matched in lit. The output format is entirely controlled by the tool, and not by the unwinder itself, so it would be stable in the face of changes to the underlying unwinder. Could have various options to enable or disable features of the unwinder in order to force the unwinder into modes that can be tricky to encounter in the wild.

  2. lldb-symbol - A tool for testing symbol resolution. Could have options for testing things like:

  3. Determining if a symbol matches an executable

  4. looking up a symbol by name in the debug info, and mapping it to an address in the process.

  5. Displaying candidate symbols when doing name lookup in a particular scope (e.g. while stopped at a breakpoint).

  6. lldb-breakpoint - A tool for testing breakpoints and stepping. Various options could include:

  7. Set breakpoints and out addresses and/or symbol names where they were resolved to.

  8. Trigger commands, so that when a breakpoint is hit the tool could automatically continue and try to run to another breakpoint, etc.

  9. options to inspect certain useful pieces of state about an inferior, to be matched in lit.

  10. lldb-interpreter - tests the jitter etc. I don’t know much about this, but I don’t see why this couldn’t be tested in a manner similar to how lli is tested.

  11. lldb-platform - tests lldb local and remote platform interfaces.

  12. lldb-cli – lldb interactive command line.

  13. lldb-format - lldb data formatters etc.

  14. Tests NOW, not later.

  15. I know we’ve been over this a million times and it’s not worth going over the arguments again. And I know it’s hard to write tests, often requiring the invention of new SB APIs. Hopefully those issues will be addressed by above a) and b) above and writing tests will be easier. Vedant Kumar ran some analytics on the various codebases and found that LLDB has the lowest test / commit ratio of any LLVM project (He didn’t post numbers for lld, so I’m not sure what it is there).

  16. lldb: 287 of the past 1000 commits

  17. llvm: 511 of the past 1000 commits

  18. clang: 622 of the past 1000 commits

  19. compiler-rt: 543 of the past 1000 commits

This is an alarming statistic, and I would love to see this number closer to 50%.

I am definitely not innocent in this regard. However, it does happen for a reason.

Sometimes, I am writing code in lldb that is the foundation of something I need to do over on the Swift.org side.

I’ll lay out foundational work/groundwork/plumbing code/… on the llvm.org side of the fence, but that code is a no-op there. The real grunt work happens on the Swift support. It’s just architecturally sound to have non-Swift-specific bits happen on the llvm.org side. When that happens, I have no reasonable way (in the current model) to test the groundwork - it’s just an intricate no-op that doesn’t get activated.

There are tests. They are on a different repo. It’s not great, I’ll admit. But right now, I would have to design an API around those bits even though I don’t need one, or add commands I don’t want “just” for testing. That is polluting a valuable user-facing resource with implementation details. I would gladly jump on a testing infrastructure that lets me write tests for this kind of code without extra API/commands.

  1. Code style / development conventions - Aside from just the column limitations and bracing styles, there are other areas where LLDB differs from LLVM on code style. We should continue to adopt more of LLVM’s style where it makes sense. I’ve identified a couple of areas (incomplete list) which I outline below.

  2. Clean up the mess of cyclical dependencies and properly layer the libraries. This is especially important for things like lldb-server that need to link in as little as possible, but regardless it leads to a more robust architecture, faster build and link times, better testability, and is required if we ever want to do a modules build of LLDB

  3. Use CMake instead of Xcode project (CMake supports Frameworks). CMake supports Apple Frameworks, so the main roadblock to getting this working is just someone doing it. Segmenting the build process by platform doesn’t make sense for the upstream, especially when there is a perfectly workable solution. I have no doubt that the resulting Xcode workspace generated automatically by CMake will not be as “nice” as one that is maintained by hand. We face this problem with Visual Studio on Windows as well. The solution that most people have adopted is to continue using the IDE for code editing and debugging, but for actually running the build, use CMake with Ninja. A similar workflow should still be possible with an OSX CMake build, but as I do not work every day on a Mac, all I can say is that it’s possible, I have no idea how impactful it would be on peoples’ workflows.

I am very much in the minority on this issue, but +100
I have no particular allegiance to CMake over Xcode, or vice versa, but I definitely don’t see the value in our current multiple build system model.

  1. Variable naming conventions

  2. I don’t expect anyone is too fond of LLDB’s naming conventions, but if we’re committed to joining the LLVM ecosystem, then let’s go all the way.

  3. Use more modern C++ and less C

  4. Old habits die hard, but this isn’t just a matter of style. It leads to safer, more robust, and less fragile code as well.

  5. Shorter functions and classes with more narrowly targeted responsibilities

  6. It’s not uncommon to find functions that are hundreds (and in a few cases even 1,000+) of lines long. We really need to be better about breaking functions and classes down into smaller responsibilities. This helps not just for someone coming in to read the function, but also for testing. Smaller functions are easier to unit test.

  7. Convert T foo(X, Y, Error &error) functions to Expected foo(X, Y) style (Depends on 1.c)

  8. llvm::Expected is based on the llvm::Error class described earlier. It’s used when a function is supposed to return a value, but it could fail. By packaging the error with the return value, it’s impossible to have a situation where you use the return value even in case of an error, and because llvm::Error has mandatory checking, it’s also impossible to have a sitaution where you don’t check the error. So it’s very safe.

Whew. That was a lot. If you made it this far, thanks for reading!

Obviously if we were to embark on all of the above, it would take many months to complete everything. So I’m not proposing anyone stop what they’re doing to work on this. This is just my own personal wishlist


lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Thanks,
- Enrico
:envelope_with_arrow: egranata@.com :phone: 27683

Following up with Kate's post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I've been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

  • De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is *similar* to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I've identified. This list is by no means complete. For each one, I've given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:
    • Use llvm::Regex instead of lldb::Regex
      • llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?
      • Risk: 6
      • Impact: 3
      • Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

As long as it supports all of the features, then this is fine. Otherwise we will need to modify the regular expressions to work with the LLVM version or back port any advances regex features we need into the LLVM regex parser.

    • Use llvm streams instead of lldb::StreamString
      • Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.
      • Interoperates nicely with many existing llvm utility classes
      • Risk: 4
      • Impact: 5
      • Difficulty / Effort: 7

I have worked extensively with both C++ streams and with printf. I don't find the type safe streaming operators to be readable and a great way to place things into streams. Part of my objection to this will be quelled if the LLVM streams are not stateful like C++ streams are (add an extra "std::hex" into the stream and suddenly other things down stream start coming out in hex). As long as printf functionality is in the LLVM streams I am ok with it.

    • Use llvm::Error instead of lldb::Error
      • llvm::Error is an error class that *requires* you to check whether it succeeded or it will assert. In a way, it's similar to a C++ exception, except that it doesn't come with the performance hit associated with exceptions. It's extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.
      • Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.
      • Risk: 7
      • Impact: 7
      • Difficulty / Effort: 8

We must be very careful here. Nothing can crash LLDB and adding something that will be known to crash in some cases can only be changed if there is a test that can guarantee that it won't crash. assert() calls in a shared library like LLDB are not OK and shouldn't fire. Even if they do, when asserts are off, then it shouldn't crash LLDB. We have made efforts to only use asserts in LLDB for things that really can't happen, but for things like constructing a StringRef with NULL is one example of why I don't want to use certain classes in LLVM. If we can remove the crash threat, then lets use them. But many people firmly believe that asserts belong in llvm and clang code and I don't agree. Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don't want them.

    • StringRef instead of const char *, len everywhere
      • Can do most common string operations in a way that is guaranteed to be safe.
      • Reduces string manipulation algorithm complexity by an order of magnitude.
      • Can potentially eliminate tens of thousands of string copies across the codebase.
      • Simplifies code.
      • Risk: 3
      • Impact: 8
      • Difficulty / Effort: 7

I don't think StringRef needs to be used everywhere, but it many places it does make sense. For example our public API should not contain any LLVM classes in the API. "const char *" is a fine parameter for public functions. I really hate the fact that constructing llvm::StringRef with NULL causes an assertion. Many people don't know that and don't take that into account when making their changes. I would love to see the assertion taken out to tell you the truth. That would make me feel better about using StringRef in many more places within LLDB as we shouldn't ever crash due to this. I would expect most internal APIs are safe to move to llvm::StringRef, but we will need to make sure none of these require a null terminated string which would cause us to have to call "str.str().c_str()". So internally, yes we can cut over to more use of StringRef. But the public API can't be changed.

    • ArrayRef instead of const void *, len everywhere
      • Same analysis as StringRef

Internally yes, external APIs no.

    • MutableArrayRef instead of void *, len everywhere
      • Same analysis as StringRef

Internally yes, external APIs no.

    • Delete ConstString, use a modified StringPool that is thread-safe.
      • StringPool is a non thread-safe version of ConstString.
      • Strings are internally refcounted so they can be cleaned up when they are no longer used. ConstStrings are a large source of memory in LLDB, so ref-counting and removing stale strings has the potential to be a huge savings.
      • Risk: 2
      • Impact: 9
      • Difficulty / Effort: 6

We can't currently rely on ref counting. We currently hand out "const char *" as return values from many public API calls and these rely on the strings living forever. We many many existing clients and we can't change the public API. So I vote to avoid this. We are already using LLVM string pools under the covers and we also associate the mangled/demangled names in the map as is saves us a ton of cycles when parsing stuff as we never demangle (very expensive) the same name twice. So I don't see the need to change this as it is already backed by LLVM technology under the covers.

    • thread_local instead of lldb::ThreadLocal
      • This fixes a number of bugs on Windows that cannot be fixed otherwise, as they require compiler support.
      • Some other compilers may not support this yet?
      • Risk: 2
      • Impact: 3
      • Difficulty: 3

As long as all compilers support this then this is fine.

    • Use llvm::cl for the command line arguments to the primary lldb executable.
      • Risk: 2
      • Impact: 3
      • Difficulty / Effort: 4

Easy and fine to switch over to. We might need to port some getopt_long functionality into it if it doesn't handle all of the things that getopt_long does. For example arguments and options can be interspersed in getopt_long(). You can also terminate your arguments with "--". It accepts single dashes for long option names if they don't conflict with short options. Many things like this.

  • Testing - Our testing infrastructure is unstable, and our test coverage is lacking. We should take steps to improve this.
    • Port as much as possible to lit
      • Simple tests should be trivial to port to lit today. If nothing else this serves as a proof of concept while increasing the speed and stability of the test suite, since lit is a more stable harness.

As long a tests use the public API to run test. We are not doing text scraping.

    • Separate testing tools
      • One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

We have a public API... Why are we going to go and spend _any_ time on doing anything else? I just don't get it. What a waste of time. We have a public API. Lets use it. Not simple enough for people? Tough. Testing a debugger should be done using the public API except when we are actually trying to test the LLDB command line in pexpect like tests. Those and only those are fine to covert over to using LIT and use text scraping. But 95% of our testing should be done using the API that our IDEs use. Using MI? Please no.

I don’t take full credit for this idea. I had been toying with a similar idea for some time, but it was further cemented in an offline discussion with a co-worker.

There many small, targeted tools in LLVM (e.g. llc, lli, llvm-objdump, etc) whose purpose are to be chained together to do interesting things. Instead of a command line api as we think of in LLDB where you type commands from an interactive prompt, they have a command line api as you would expect from any tool which is launched from a shell.

I can imagine many potential candidates for lldb tools of this nature. Off the top of my head:
  • lldb-unwind - A tool for testing the unwinder. Accepts byte code as input and passes it through to the unwinder, outputting a compressed summary of the steps taken while unwinding, which could be pattern matched in lit. The output format is entirely controlled by the tool, and not by the unwinder itself, so it would be stable in the face of changes to the underlying unwinder. Could have various options to enable or disable features of the unwinder in order to force the unwinder into modes that can be tricky to encounter in the wild.
  • lldb-symbol - A tool for testing symbol resolution. Could have options for testing things like:
    • Determining if a symbol matches an executable
    • looking up a symbol by name in the debug info, and mapping it to an address in the process.
    • Displaying candidate symbols when doing name lookup in a particular scope (e.g. while stopped at a breakpoint).
  • lldb-breakpoint - A tool for testing breakpoints and stepping. Various options could include:
    • Set breakpoints and out addresses and/or symbol names where they were resolved to.
    • Trigger commands, so that when a breakpoint is hit the tool could automatically continue and try to run to another breakpoint, etc.
    • options to inspect certain useful pieces of state about an inferior, to be matched in lit.
  • lldb-interpreter - tests the jitter etc. I don’t know much about this, but I don’t see why this couldn’t be tested in a manner similar to how lli is tested.
  • lldb-platform - tests lldb local and remote platform interfaces.
  • lldb-cli -- lldb interactive command line.
  • lldb-format - lldb data formatters etc.

I agree that testing more functionality it a good idea, but if it is worth testing we should see if we can get it into our public API. If so, then we test it there. If not, then we come up with another solution. Even in the alternate solution, it will be something that will probably create structured data (JSON, Yaml, etc) and then parsed, so I would rather spend the time getting these things into out public APIs, or test them vai our public APIs.

  • Tests NOW, not later.
    • I know we’ve been over this a million times and it’s not worth going over the arguments again. And I know it’s hard to write tests, often requiring the invention of new SB APIs. Hopefully those issues will be addressed by above a) and b) above and writing tests will be easier. Vedant Kumar ran some analytics on the various codebases and found that LLDB has the lowest test / commit ratio of any LLVM project (He didn’t post numbers for lld, so I’m not sure what it is there).
      • lldb: 287 of the past 1000 commits
      • llvm: 511 of the past 1000 commits
      • clang: 622 of the past 1000 commits
      • compiler-rt: 543 of the past 1000 commits
This is an alarming statistic, and I would love to see this number closer to 50%.
  • Code style / development conventions - Aside from just the column limitations and bracing styles, there are other areas where LLDB differs from LLVM on code style. We should continue to adopt more of LLVM's style where it makes sense. I've identified a couple of areas (incomplete list) which I outline below.
    • Clean up the mess of cyclical dependencies and properly layer the libraries. This is especially important for things like lldb-server that need to link in as little as possible, but regardless it leads to a more robust architecture, faster build and link times, better testability, and is required if we ever want to do a modules build of LLDB
    • Use CMake instead of Xcode project (CMake supports Frameworks). CMake supports Apple Frameworks, so the main roadblock to getting this working is just someone doing it. Segmenting the build process by platform doesn't make sense for the upstream, especially when there is a perfectly workable solution. I have no doubt that the resulting Xcode workspace generated automatically by CMake will not be as "nice" as one that is maintained by hand. We face this problem with Visual Studio on Windows as well. The solution that most people have adopted is to continue using the IDE for code editing and debugging, but for actually running the build, use CMake with Ninja. A similar workflow should still be possible with an OSX CMake build, but as I do not work every day on a Mac, all I can say is that it's possible, I have no idea how impactful it would be on peoples' workflows.
    • Variable naming conventions
      • I don’t expect anyone is too fond of LLDB’s naming conventions, but if we’re committed to joining the LLVM ecosystem, then let’s go all the way.

Hopefully we can get LLVM and Clang to adopt naming member variables with a prefix... If so, that is my main remaining concern.

    • Use more modern C++ and less C
      • Old habits die hard, but this isn’t just a matter of style. It leads to safer, more robust, and less fragile code as well.
    • Shorter functions and classes with more narrowly targeted responsibilities
      • It’s not uncommon to find functions that are hundreds (and in a few cases even 1,000+) of lines long. We really need to be better about breaking functions and classes down into smaller responsibilities. This helps not just for someone coming in to read the function, but also for testing. Smaller functions are easier to unit test.
    • Convert T foo(X, Y, Error &error) functions to Expected<T> foo(X, Y) style (Depends on 1.c)
      • llvm::Expected is based on the llvm::Error class described earlier. It’s used when a function is supposed to return a value, but it could fail. By packaging the error with the return value, it’s impossible to have a situation where you use the return value even in case of an error, and because llvm::Error has mandatory checking, it’s also impossible to have a sitaution where you don’t check the error. So it’s very safe.

As crashes if you don't check the errors. One of the big differences between LLDB and LLVM/Clang is that asserts are used liberally all over clang which make the code very crashy when used in production with uncontrolled input like we get in the debugger. We will need to be very careful with any changes to make sure we don't increase crash frequency.

Whew. That was a lot. If you made it this far, thanks for reading!

Obviously if we were to embark on all of the above, it would take many months to complete everything. So I'm not proposing anyone stop what they're doing to work on this. This is just my own personal wishlist

There are many great things in here. As long as we are careful to not increase the crash frequency in LLDB I am all for it. My mains areas of concern are:
- public API can't change in its current form
- no LLVM or clang classes in the public API as arguments or return values.
- don't crash more by introducing assert heavy code into code paths that use input from outside sources

As you said we should be converting over to using LLVM/Clang coding conventions as we proceed as we have already taken the big reformatting step.

Greg

Part of the problem is just that I think we don’t have the tools we need to write effective tests. We have a lot of tests that only work on a particular platform, or with a particular language runtime, or all these different variables that affect the configurations under which the test should pass. In an ideal world we would be able to test the lion’s share of the debugger on any platform. The only thing that’s platform specific is really the loader. But maybe we have a huge block of code that itself is not platform specific, but only gets run as a child of some platform specific branch. Now coverage of that platform-inedpenent branch is limited or non-existant, because it depends on being able to have juuust the right setup to tickle the debugger into running it.

So I just want to re-iterate that I don’t think anyone is to blame, we just don’t have the right tools that we need. I think the separate tools that I mentioned could go a long way to rectifying that, because you can skip past all of the platform specific aspects and test the code that runs behind them.

Of course, you’d still have your full integration tests, but you’d now have much fewer. And when they fail, you’d have a pretty good idea where to look because presumably the failure would be specific to some bit of platform specific functionality.

I remmeber one case where TestUnsigned failed on Windows but TestSigned passed. And when we ultimately tracked down the error, it had absolutely nothing to do with signed-ness. Because there were too many layers in between the test and the functionality being tested.

FWIW, I agree with you about mi.

That said, the public api is problematic. Here you’ve got an API which, once you do something to, is set in stone forever. And yet in order to test something, you have to add it to the API. So now, in order to test something, you have to make a permanent change to a public API that can never be undone.

It’s also a public facing API, when a lot of the stuff we need to be able to look at and inspect is not really suitable for public consumption.

If something is a public API that can never be changed once it’s added, then there should be an extremely strict review process in order to add something to it. It should be VERY HARD to modify (it’s currently not, which is a debate for another day, but anyway…). And yet that’s fundamentally incompatible with having tests be easy to write. When it’s hard to add the thing you need to add in order to write the test, then it’s hard to write the test.

Furthermore, with a system such as the one I proposed, you could even run tests with LLDB_DISABLE_PYTHON. IDK about you, but that would be amazing from my point of view. Even though I spent months making Python 3 work, I would be happy to burn it all with fire and go back to just Python 2 if we had a viable testing strategy.

Following up with Kate's post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I've been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

  • De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is *similar* to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I've identified. This list is by no means complete. For each one, I've given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:
    • Use llvm::Regex instead of lldb::Regex
      • llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?
      • Risk: 6
      • Impact: 3
      • Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

As long as it supports all of the features, then this is fine. Otherwise we will need to modify the regular expressions to work with the LLVM version or back port any advances regex features we need into the LLVM regex parser.

    • Use llvm streams instead of lldb::StreamString
      • Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.
      • Interoperates nicely with many existing llvm utility classes
      • Risk: 4
      • Impact: 5
      • Difficulty / Effort: 7

I have worked extensively with both C++ streams and with printf. I don't find the type safe streaming operators to be readable and a great way to place things into streams. Part of my objection to this will be quelled if the LLVM streams are not stateful like C++ streams are (add an extra "std::hex" into the stream and suddenly other things down stream start coming out in hex). As long as printf functionality is in the LLVM streams I am ok with it.

    • Use llvm::Error instead of lldb::Error
      • llvm::Error is an error class that *requires* you to check whether it succeeded or it will assert. In a way, it's similar to a C++ exception, except that it doesn't come with the performance hit associated with exceptions. It's extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.
      • Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.
      • Risk: 7
      • Impact: 7
      • Difficulty / Effort: 8

We must be very careful here. Nothing can crash LLDB and adding something that will be known to crash in some cases can only be changed if there is a test that can guarantee that it won't crash. assert() calls in a shared library like LLDB are not OK and shouldn't fire. Even if they do, when asserts are off, then it shouldn't crash LLDB. We have made efforts to only use asserts in LLDB for things that really can't happen, but for things like constructing a StringRef with NULL is one example of why I don't want to use certain classes in LLVM. If we can remove the crash threat, then lets use them. But many people firmly believe that asserts belong in llvm and clang code and I don't agree.

I’m surprise by your aversion to assertions, what is your suggested alternative? Are you expecting to check and handle every possible invariants everywhere and recover (or signal an error) properly? That does not seem practical, and it seems to me that it'd lead to just involving UB (or breaking design invariant) without actually noticing it.

Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don't want them.

It is not really supported to use the LLVM C++ headers without assertions and link to a LLVM build with assertions (and vice-versa).

    • StringRef instead of const char *, len everywhere
      • Can do most common string operations in a way that is guaranteed to be safe.
      • Reduces string manipulation algorithm complexity by an order of magnitude.
      • Can potentially eliminate tens of thousands of string copies across the codebase.
      • Simplifies code.
      • Risk: 3
      • Impact: 8
      • Difficulty / Effort: 7

I don't think StringRef needs to be used everywhere, but it many places it does make sense. For example our public API should not contain any LLVM classes in the API. "const char *" is a fine parameter for public functions. I really hate the fact that constructing llvm::StringRef with NULL causes an assertion. Many people don't know that and don't take that into account when making their changes. I would love to see the assertion taken out to tell you the truth. That would make me feel better about using StringRef in many more places within LLDB as we shouldn't ever crash due to this. I would expect most internal APIs are safe to move to llvm::StringRef, but we will need to make sure none of these require a null terminated string which would cause us to have to call "str.str().c_str()". So internally, yes we can cut over to more use of StringRef. But the public API can't be changed.

    • ArrayRef instead of const void *, len everywhere
      • Same analysis as StringRef

Internally yes, external APIs no.

    • MutableArrayRef instead of void *, len everywhere
      • Same analysis as StringRef

Internally yes, external APIs no.

    • Delete ConstString, use a modified StringPool that is thread-safe.
      • StringPool is a non thread-safe version of ConstString.
      • Strings are internally refcounted so they can be cleaned up when they are no longer used. ConstStrings are a large source of memory in LLDB, so ref-counting and removing stale strings has the potential to be a huge savings.
      • Risk: 2
      • Impact: 9
      • Difficulty / Effort: 6

We can't currently rely on ref counting. We currently hand out "const char *" as return values from many public API calls and these rely on the strings living forever. We many many existing clients and we can't change the public API. So I vote to avoid this. We are already using LLVM string pools under the covers and we also associate the mangled/demangled names in the map as is saves us a ton of cycles when parsing stuff as we never demangle (very expensive) the same name twice. So I don't see the need to change this as it is already backed by LLVM technology under the covers.

    • thread_local instead of lldb::ThreadLocal
      • This fixes a number of bugs on Windows that cannot be fixed otherwise, as they require compiler support.
      • Some other compilers may not support this yet?
      • Risk: 2
      • Impact: 3
      • Difficulty: 3

As long as all compilers support this then this is fine.

    • Use llvm::cl for the command line arguments to the primary lldb executable.
      • Risk: 2
      • Impact: 3
      • Difficulty / Effort: 4

Easy and fine to switch over to. We might need to port some getopt_long functionality into it if it doesn't handle all of the things that getopt_long does. For example arguments and options can be interspersed in getopt_long(). You can also terminate your arguments with "--". It accepts single dashes for long option names if they don't conflict with short options. Many things like this.

  • Testing - Our testing infrastructure is unstable, and our test coverage is lacking. We should take steps to improve this.
    • Port as much as possible to lit
      • Simple tests should be trivial to port to lit today. If nothing else this serves as a proof of concept while increasing the speed and stability of the test suite, since lit is a more stable harness.

As long a tests use the public API to run test. We are not doing text scraping.

    • Separate testing tools
      • One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

We have a public API... Why are we going to go and spend _any_ time on doing anything else? I just don't get it. What a waste of time. We have a public API. Lets use it. Not simple enough for people? Tough. Testing a debugger should be done using the public API except when we are actually trying to test the LLDB command line in pexpect like tests. Those and only those are fine to covert over to using LIT and use text scraping. But 95% of our testing should be done using the API that our IDEs use. Using MI? Please no.

I don’t take full credit for this idea. I had been toying with a similar idea for some time, but it was further cemented in an offline discussion with a co-worker.

There many small, targeted tools in LLVM (e.g. llc, lli, llvm-objdump, etc) whose purpose are to be chained together to do interesting things. Instead of a command line api as we think of in LLDB where you type commands from an interactive prompt, they have a command line api as you would expect from any tool which is launched from a shell.

I can imagine many potential candidates for lldb tools of this nature. Off the top of my head:
  • lldb-unwind - A tool for testing the unwinder. Accepts byte code as input and passes it through to the unwinder, outputting a compressed summary of the steps taken while unwinding, which could be pattern matched in lit. The output format is entirely controlled by the tool, and not by the unwinder itself, so it would be stable in the face of changes to the underlying unwinder. Could have various options to enable or disable features of the unwinder in order to force the unwinder into modes that can be tricky to encounter in the wild.
  • lldb-symbol - A tool for testing symbol resolution. Could have options for testing things like:
    • Determining if a symbol matches an executable
    • looking up a symbol by name in the debug info, and mapping it to an address in the process.
    • Displaying candidate symbols when doing name lookup in a particular scope (e.g. while stopped at a breakpoint).
  • lldb-breakpoint - A tool for testing breakpoints and stepping. Various options could include:
    • Set breakpoints and out addresses and/or symbol names where they were resolved to.
    • Trigger commands, so that when a breakpoint is hit the tool could automatically continue and try to run to another breakpoint, etc.
    • options to inspect certain useful pieces of state about an inferior, to be matched in lit.
  • lldb-interpreter - tests the jitter etc. I don’t know much about this, but I don’t see why this couldn’t be tested in a manner similar to how lli is tested.
  • lldb-platform - tests lldb local and remote platform interfaces.
  • lldb-cli -- lldb interactive command line.
  • lldb-format - lldb data formatters etc.

I agree that testing more functionality it a good idea, but if it is worth testing we should see if we can get it into our public API. If so, then we test it there. If not, then we come up with another solution. Even in the alternate solution, it will be something that will probably create structured data (JSON, Yaml, etc) and then parsed, so I would rather spend the time getting these things into out public APIs, or test them vai our public APIs.

  • Tests NOW, not later.
    • I know we’ve been over this a million times and it’s not worth going over the arguments again. And I know it’s hard to write tests, often requiring the invention of new SB APIs. Hopefully those issues will be addressed by above a) and b) above and writing tests will be easier. Vedant Kumar ran some analytics on the various codebases and found that LLDB has the lowest test / commit ratio of any LLVM project (He didn’t post numbers for lld, so I’m not sure what it is there).
      • lldb: 287 of the past 1000 commits
      • llvm: 511 of the past 1000 commits
      • clang: 622 of the past 1000 commits
      • compiler-rt: 543 of the past 1000 commits
This is an alarming statistic, and I would love to see this number closer to 50%.
  • Code style / development conventions - Aside from just the column limitations and bracing styles, there are other areas where LLDB differs from LLVM on code style. We should continue to adopt more of LLVM's style where it makes sense. I've identified a couple of areas (incomplete list) which I outline below.
    • Clean up the mess of cyclical dependencies and properly layer the libraries. This is especially important for things like lldb-server that need to link in as little as possible, but regardless it leads to a more robust architecture, faster build and link times, better testability, and is required if we ever want to do a modules build of LLDB
    • Use CMake instead of Xcode project (CMake supports Frameworks). CMake supports Apple Frameworks, so the main roadblock to getting this working is just someone doing it. Segmenting the build process by platform doesn't make sense for the upstream, especially when there is a perfectly workable solution. I have no doubt that the resulting Xcode workspace generated automatically by CMake will not be as "nice" as one that is maintained by hand. We face this problem with Visual Studio on Windows as well. The solution that most people have adopted is to continue using the IDE for code editing and debugging, but for actually running the build, use CMake with Ninja. A similar workflow should still be possible with an OSX CMake build, but as I do not work every day on a Mac, all I can say is that it's possible, I have no idea how impactful it would be on peoples' workflows.
    • Variable naming conventions
      • I don’t expect anyone is too fond of LLDB’s naming conventions, but if we’re committed to joining the LLVM ecosystem, then let’s go all the way.

Hopefully we can get LLVM and Clang to adopt naming member variables with a prefix... If so, that is my main remaining concern.

    • Use more modern C++ and less C
      • Old habits die hard, but this isn’t just a matter of style. It leads to safer, more robust, and less fragile code as well.
    • Shorter functions and classes with more narrowly targeted responsibilities
      • It’s not uncommon to find functions that are hundreds (and in a few cases even 1,000+) of lines long. We really need to be better about breaking functions and classes down into smaller responsibilities. This helps not just for someone coming in to read the function, but also for testing. Smaller functions are easier to unit test.
    • Convert T foo(X, Y, Error &error) functions to Expected<T> foo(X, Y) style (Depends on 1.c)
      • llvm::Expected is based on the llvm::Error class described earlier. It’s used when a function is supposed to return a value, but it could fail. By packaging the error with the return value, it’s impossible to have a situation where you use the return value even in case of an error, and because llvm::Error has mandatory checking, it’s also impossible to have a sitaution where you don’t check the error. So it’s very safe.

As crashes if you don't check the errors. One of the big differences between LLDB and LLVM/Clang is that asserts are used liberally all over clang which make the code very crashy when used in production with uncontrolled input like we get in the debugger. We will need to be very careful with any changes to make sure we don't increase crash frequency.

Whew. That was a lot. If you made it this far, thanks for reading!

Obviously if we were to embark on all of the above, it would take many months to complete everything. So I'm not proposing anyone stop what they're doing to work on this. This is just my own personal wishlist

There are many great things in here. As long as we are careful to not increase the crash frequency in LLDB I am all for it. My mains areas of concern are:
- public API can't change in its current form
- no LLVM or clang classes in the public API as arguments or return values.
- don't crash more by introducing assert heavy code into code paths that use input from outside sources

It seems to me that you are mixing two things: asserting on user inputs vs asserting on internal invariants of the system. LLVM is very intensive about asserting on the second category and this seems fine to me.
Asserting on external inputs is not great (in LLVM as much as in LLDB).

The asserting error class above falls into the second category and is a great tool to enforce programmer error

Following up with Kate’s post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I’ve been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

• De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is similar to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I’ve identified. This list is by no means complete. For each one, I’ve given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:
• Use llvm::Regex instead of lldb::Regex
• llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?
• Risk: 6
• Impact: 3
• Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

As long as it supports all of the features, then this is fine. Otherwise we will need to modify the regular expressions to work with the LLVM version or back port any advances regex features we need into the LLVM regex parser.

• Use llvm streams instead of lldb::StreamString
• Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.
• Interoperates nicely with many existing llvm utility classes
• Risk: 4
• Impact: 5
• Difficulty / Effort: 7

I have worked extensively with both C++ streams and with printf. I don’t find the type safe streaming operators to be readable and a great way to place things into streams. Part of my objection to this will be quelled if the LLVM streams are not stateful like C++ streams are (add an extra “std::hex” into the stream and suddenly other things down stream start coming out in hex). As long as printf functionality is in the LLVM streams I am ok with it.

I agree with you on the streams for the most part. llvm streams do not use stateful manipulators, although they do have stateless manipulators. For example, you can write:

stream << llvm::format_hex(n);

and that would print n as hex, but it wouldn’t affect the printing of subsequent items. You also still get printf style formatting by using the llvm::format stateless manipulator. Like this:

stream << llvm::format("%0.4f", myfloat)

• Use llvm::Error instead of lldb::Error
• llvm::Error is an error class that requires you to check whether it succeeded or it will assert. In a way, it’s similar to a C++ exception, except that it doesn’t come with the performance hit associated with exceptions. It’s extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.
• Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.
• Risk: 7
• Impact: 7
• Difficulty / Effort: 8

We must be very careful here. Nothing can crash LLDB and adding something that will be known to crash in some cases can only be changed if there is a test that can guarantee that it won’t crash. assert() calls in a shared library like LLDB are not OK and shouldn’t fire. Even if they do, when asserts are off, then it shouldn’t crash LLDB. We have made efforts to only use asserts in LLDB for things that really can’t happen, but for things like constructing a StringRef with NULL is one example of why I don’t want to use certain classes in LLVM. If we can remove the crash threat, then lets use them. But many people firmly believe that asserts belong in llvm and clang code and I don’t agree. Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don’t want them.

We can probably add something to llvm::Error to make it not assert but to do something else instead. Something that would be up to the person raising the error, so we could say that all lldb errors wouldn’t cause a problem.

As an aside, have you guys ever talked about the idea of using out of process isolation so it doesn’t bring down all of xcode when it crashes? Obviously it’s a ton of work, but it would solve these kidns of problems.

• StringRef instead of const char *, len everywhere
• Can do most common string operations in a way that is guaranteed to be safe.
• Reduces string manipulation algorithm complexity by an order of magnitude.
• Can potentially eliminate tens of thousands of string copies across the codebase.
• Simplifies code.
• Risk: 3
• Impact: 8
• Difficulty / Effort: 7

I don’t think StringRef needs to be used everywhere, but it many places it does make sense. For example our public API should not contain any LLVM classes in the API. “const char *” is a fine parameter for public functions. I really hate the fact that constructing llvm::StringRef with NULL causes an assertion. Many people don’t know that and don’t take that into account when making their changes. I would love to see the assertion taken out to tell you the truth. That would make me feel better about using StringRef in many more places within LLDB as we shouldn’t ever crash due to this. I would expect most internal APIs are safe to move to llvm::StringRef, but we will need to make sure none of these require a null terminated string which would cause us to have to call “str.str().c_str()”. So internally, yes we can cut over to more use of StringRef. But the public API can’t be changed.

Definitely no changing the public API, I agree with you there. A lot of times where we currently “need” a null terminated string, that’s only because the null terminated string is being passed to a C api which has a StringRef counterpart. Not always, but often. I think when you have StringRefs all the way down, the problem of constructing it with a null pointer largely goes away because you don’t have that many pointers left to begin with.

• ArrayRef instead of const void *, len everywhere
• Same analysis as StringRef

Internally yes, external APIs no.

• MutableArrayRef instead of void *, len everywhere
• Same analysis as StringRef
Internally yes, external APIs no.
• Delete ConstString, use a modified StringPool that is thread-safe.
• StringPool is a non thread-safe version of ConstString.
• Strings are internally refcounted so they can be cleaned up when they are no longer used. ConstStrings are a large source of memory in LLDB, so ref-counting and removing stale strings has the potential to be a huge savings.
• Risk: 2
• Impact: 9
• Difficulty / Effort: 6

We can’t currently rely on ref counting. We currently hand out “const char *” as return values from many public API calls and these rely on the strings living forever. We many many existing clients and we can’t change the public API. So I vote to avoid this. We are already using LLVM string pools under the covers and we also associate the mangled/demangled names in the map as is saves us a ton of cycles when parsing stuff as we never demangle (very expensive) the same name twice. So I don’t see the need to change this as it is already backed by LLVM technology under the covers.

Just thinking out loud here, but one possibility is to have multiple pools. One which is ref counted and one which isn’t. If you need something to live forever, vend it from the non-ref-counted pool. Otherwise vend it from the ref counted pool.

> • Separate testing tools
> • One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

We have a public API... Why are we going to go and spend _any_ time on doing anything else? I just don't get it. What a waste of time. We have a public API. Lets use it. Not simple enough for people? Tough. Testing a debugger should be done using the public API except when we are actually trying to test the LLDB command line in pexpect like tests. Those and only those are fine to covert over to using LIT and use text scraping. But 95% of our testing should be done using the API that our IDEs use. Using MI? Please no.

FWIW, I agree with you about mi.

That said, the public api is problematic. Here you've got an API which, once you do something to, is set in stone forever. And yet in order to test something, you have to add it to the API. So now, in order to test something, you have to make a permanent change to a public API that can never be undone.

So let me clarify: I don't want people adding to the public API for the sole reason of testing.. That falls into the "test another way" category. So I should have said "add it to the public API if it would be a valid addition to the public API.

It's also a public facing API, when a lot of the stuff we need to be able to look at and inspect is not really suitable for public consumption.

Again, this falls into the "test another way" category as is perfect for gtest or lit.

If something is a public API that can never be changed once it's added, then there should be an extremely strict review process in order to add something to it. It should be VERY HARD to modify (it's currently not, which is a debate for another day, but anyway...). And yet that's fundamentally incompatible with having tests be easy to write. When it's hard to add the thing you need to add in order to write the test, then it's hard to write the test.

Yes yes yes. Sorry about the confusion. My first sentence should clear this up...

Furthermore, with a system such as the one I proposed, you could even run tests with LLDB_DISABLE_PYTHON. IDK about you, but that would be *amazing* from my point of view. Even though I spent months making Python 3 work, I would be happy to burn it all with fire and go back to just Python 2 if we had a viable testing strategy.

I am not opposed to that. That being say we could start having most API tests actually use the C++ public API and have the tests written in C++. We would need a .a file with some canned functionality in it, but anything that is testing the public API could just be C++.

So I definitely don't want people adding to the public API just for testing. The rules should be:
- Does my feature belong in the public API?
  - Yes, great, add it and test it there
  - No, test it with gtest or lit

Greg

I can kind of see both sides on this one. Yes, if you can catch instances of UB before it happens that’s helpful. At the same time, when you’ve got a product (such as, say, Xcode), you also want to try as hard as possible not to bring the whole product down. In lldb we have lldbassert, which asserts for real in debug, but in release it logs an error to a file. This is nice because if someone submits a crash report, we can see the assertion failure in the log file.

Now, obviously the real solution is to make LLDB out-of-process so it doesn’t bring down Xcode when it crashes. But (and I have no knowledge of the Xcode / LLDB integration) I can easily see this being a monumental amount of effort.

Personally, asserting in LLDB doesn’t affect me and I’m fine with lldb::Error asserting… But I can kind of see the argument for not wanting it to assert.

On the other hand, if we had better test coverage, then all of these assertions would be caught during test anyway. Which brings us back to item #2 on my list…

Following up with Kate's post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I've been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

  • De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is *similar* to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I've identified. This list is by no means complete. For each one, I've given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:
    • Use llvm::Regex instead of lldb::Regex
      • llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?
      • Risk: 6
      • Impact: 3
      • Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

As long as it supports all of the features, then this is fine. Otherwise we will need to modify the regular expressions to work with the LLVM version or back port any advances regex features we need into the LLVM regex parser.

    • Use llvm streams instead of lldb::StreamString
      • Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.
      • Interoperates nicely with many existing llvm utility classes
      • Risk: 4
      • Impact: 5
      • Difficulty / Effort: 7

I have worked extensively with both C++ streams and with printf. I don't find the type safe streaming operators to be readable and a great way to place things into streams. Part of my objection to this will be quelled if the LLVM streams are not stateful like C++ streams are (add an extra "std::hex" into the stream and suddenly other things down stream start coming out in hex). As long as printf functionality is in the LLVM streams I am ok with it.

    • Use llvm::Error instead of lldb::Error
      • llvm::Error is an error class that *requires* you to check whether it succeeded or it will assert. In a way, it's similar to a C++ exception, except that it doesn't come with the performance hit associated with exceptions. It's extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.
      • Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.
      • Risk: 7
      • Impact: 7
      • Difficulty / Effort: 8

We must be very careful here. Nothing can crash LLDB and adding something that will be known to crash in some cases can only be changed if there is a test that can guarantee that it won't crash. assert() calls in a shared library like LLDB are not OK and shouldn't fire. Even if they do, when asserts are off, then it shouldn't crash LLDB. We have made efforts to only use asserts in LLDB for things that really can't happen, but for things like constructing a StringRef with NULL is one example of why I don't want to use certain classes in LLVM. If we can remove the crash threat, then lets use them. But many people firmly believe that asserts belong in llvm and clang code and I don't agree.

I’m surprise by your aversion to assertions, what is your suggested alternative? Are you expecting to check and handle every possible invariants everywhere and recover (or signal an error) properly? That does not seem practical, and it seems to me that it'd lead to just involving UB (or breaking design invariant) without actually noticing it.

We have to as a debugger. We get input from a variety of different compilers and we parse and use things there were produced by our toolchains and others. Crashing Xcode because someone accidentally created a llvm::StringRef(NULL) should not happen. It is not OK for library code to crash. This is quite OK for compilers, linkers and other tools, but it isn't for any other code. Since there are so many asserts, LLDB code is currently responsible for figuring out what will make clang unhappy and we must be sure to not feed it anything that makes it unhappy or we crash. So I don't see that as better.

Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don't want them.

It is not really supported to use the LLVM C++ headers without assertions and link to a LLVM build with assertions (and vice-versa).

As we have found out. We claim llvm and clang can be linked into tools a libraries, but it really means, you should really use it out of process because it might crash at any time so don't try and use it in a program you actually want to be able run for a long time.

FWIW, strlen(nullptr) will also crash just as easily as StringRef(nullptr) will crash, so that one isn’t a particularly convincing example of poorly used asserts, since the onus on the developer is exactly the same as with strlen. That said, I still know where you’re coming from :slight_smile:

>
> Following up with Kate's post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I've been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.
>
> I’ve grouped the areas for improvement into 3 high level categories.
>
> • De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is *similar* to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I've identified. This list is by no means complete. For each one, I've given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:
> • Use llvm::Regex instead of lldb::Regex
> • llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?
> • Risk: 6
> • Impact: 3
> • Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

As long as it supports all of the features, then this is fine. Otherwise we will need to modify the regular expressions to work with the LLVM version or back port any advances regex features we need into the LLVM regex parser.

> • Use llvm streams instead of lldb::StreamString
> • Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.
> • Interoperates nicely with many existing llvm utility classes
> • Risk: 4
> • Impact: 5
> • Difficulty / Effort: 7

I have worked extensively with both C++ streams and with printf. I don't find the type safe streaming operators to be readable and a great way to place things into streams. Part of my objection to this will be quelled if the LLVM streams are not stateful like C++ streams are (add an extra "std::hex" into the stream and suddenly other things down stream start coming out in hex). As long as printf functionality is in the LLVM streams I am ok with it.
I agree with you on the streams for the most part. llvm streams do not use stateful manipulators, although they do have stateless manipulators. For example, you can write:

stream << llvm::format_hex(n);

and that would print n as hex, but it wouldn't affect the printing of subsequent items. You also still get printf style formatting by using the llvm::format stateless manipulator. Like this:

stream << llvm::format("%0.4f", myfloat)

> • Use llvm::Error instead of lldb::Error
> • llvm::Error is an error class that *requires* you to check whether it succeeded or it will assert. In a way, it's similar to a C++ exception, except that it doesn't come with the performance hit associated with exceptions. It's extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.
> • Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.
> • Risk: 7
> • Impact: 7
> • Difficulty / Effort: 8

We must be very careful here. Nothing can crash LLDB and adding something that will be known to crash in some cases can only be changed if there is a test that can guarantee that it won't crash. assert() calls in a shared library like LLDB are not OK and shouldn't fire. Even if they do, when asserts are off, then it shouldn't crash LLDB. We have made efforts to only use asserts in LLDB for things that really can't happen, but for things like constructing a StringRef with NULL is one example of why I don't want to use certain classes in LLVM. If we can remove the crash threat, then lets use them. But many people firmly believe that asserts belong in llvm and clang code and I don't agree. Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don't want them.
We can probably add something to llvm::Error to make it not assert but to do something else instead. Something that would be up to the person raising the error, so we could say that all lldb errors wouldn't cause a problem.

As an aside, have you guys ever talked about the idea of using out of process isolation so it doesn't bring down all of xcode when it crashes? Obviously it's a ton of work, but it would solve these kidns of problems.

Yep. Xcode 8 does this just for this reason. More details on that later.

> • StringRef instead of const char *, len everywhere
> • Can do most common string operations in a way that is guaranteed to be safe.
> • Reduces string manipulation algorithm complexity by an order of magnitude.
> • Can potentially eliminate tens of thousands of string copies across the codebase.
> • Simplifies code.
> • Risk: 3
> • Impact: 8
> • Difficulty / Effort: 7

I don't think StringRef needs to be used everywhere, but it many places it does make sense. For example our public API should not contain any LLVM classes in the API. "const char *" is a fine parameter for public functions. I really hate the fact that constructing llvm::StringRef with NULL causes an assertion. Many people don't know that and don't take that into account when making their changes. I would love to see the assertion taken out to tell you the truth. That would make me feel better about using StringRef in many more places within LLDB as we shouldn't ever crash due to this. I would expect most internal APIs are safe to move to llvm::StringRef, but we will need to make sure none of these require a null terminated string which would cause us to have to call "str.str().c_str()". So internally, yes we can cut over to more use of StringRef. But the public API can't be changed.
Definitely no changing the public API, I agree with you there. A lot of times where we currently "need" a null terminated string, that's only because the null terminated string is being passed to a C api which has a StringRef counterpart. Not always, but often. I think when you have StringRefs all the way down, the problem of constructing it with a null pointer largely goes away because you don't have that many pointers left to begin with.

> • ArrayRef instead of const void *, len everywhere
> • Same analysis as StringRef

Internally yes, external APIs no.
> • MutableArrayRef instead of void *, len everywhere
> • Same analysis as StringRef
Internally yes, external APIs no.
> • Delete ConstString, use a modified StringPool that is thread-safe.
> • StringPool is a non thread-safe version of ConstString.
> • Strings are internally refcounted so they can be cleaned up when they are no longer used. ConstStrings are a large source of memory in LLDB, so ref-counting and removing stale strings has the potential to be a huge savings.
> • Risk: 2
> • Impact: 9
> • Difficulty / Effort: 6

We can't currently rely on ref counting. We currently hand out "const char *" as return values from many public API calls and these rely on the strings living forever. We many many existing clients and we can't change the public API. So I vote to avoid this. We are already using LLVM string pools under the covers and we also associate the mangled/demangled names in the map as is saves us a ton of cycles when parsing stuff as we never demangle (very expensive) the same name twice. So I don't see the need to change this as it is already backed by LLVM technology under the covers.

Just thinking out loud here, but one possibility is to have multiple pools. One which is ref counted and one which isn't. If you need something to live forever, vend it from the non-ref-counted pool. Otherwise vend it from the ref counted pool.

Again, since we already are using LLVM under the hood here, I would hope to avoid changes if possible.

I’m surprise by your aversion to assertions, what is your suggested alternative? Are you expecting to check and handle every possible invariants everywhere and recover (or signal an error) properly? That does not seem practical, and it seems to me that it’d lead to just involving UB (or breaking design invariant) without actually noticing it.

Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don’t want them.

It is not really supported to use the LLVM C++ headers without assertions and link to a LLVM build with assertions (and vice-versa).

It seems to me that you are mixing two things: asserting on user inputs vs asserting on internal invariants of the system. LLVM is very intensive about asserting on the second category and this seems fine to me.
Asserting on external inputs is not great (in LLVM as much as in LLDB).

The asserting error class above falls into the second category and is a great tool to enforce programmer error

I can kind of see both sides on this one. Yes, if you can catch instances of UB before it happens that’s helpful. At the same time, when you’ve got a product (such as, say, Xcode), you also want to try as hard as possible not to bring the whole product down. In lldb we have lldbassert, which asserts for real in debug, but in release it logs an error to a file. This is nice because if someone submits a crash report, we can see the assertion failure in the log file.

Now, obviously the real solution is to make LLDB out-of-process so it doesn’t bring down Xcode when it crashes. But (and I have no knowledge of the Xcode / LLDB integration) I can easily see this being a monumental amount of effort.

Even then, IMO you are not out of the business of not crashing on people
Sure, now you didn’t crash all of Xcode, but you still lost somebody’s debug session with potentially valuable state in it…

It sounds like llvm::Expected + lldbassert() is a good combination; it makes sure we don’t just drop error conditions on the ground, but it also makes sure that we don’t just give up the ghost and crash…

Personally, asserting in LLDB doesn’t affect me and I’m fine with lldb::Error asserting… But I can kind of see the argument for not wanting it to assert.

On the other hand, if we had better test coverage, then all

I wonder if catching them all can’t somehow be reduced to the halting problem :slight_smile: Most would already be great!

of these assertions would be caught during test anyway. Which brings us back to item #2 on my list…


lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Thanks,
- Enrico
:envelope_with_arrow: egranata@.com :phone: 27683

Right, but we aren’t specifically using the llvm::StringPool class which ref counts. idk, maybe there wouldnt’ be much savings, but I remember you saying a long time ago that ConstStrings are a huge source of memory usage in LLDB. So it seems like if we could ref-count when possible, it would be a major savings.

Following up with Kate’s post from a few weeks ago, I think the dust has settled on the code reformat and it went over pretty smoothly for the most part. So I thought it might be worth throwing out some ideas for where we go from here. I have a large list of ideas (more ideas than time, sadly) that I’ve been collecting over the past few weeks, so I figured I would throw them out in the open for discussion.

I’ve grouped the areas for improvement into 3 high level categories.

• De-inventing the wheel - We should use more code from LLVM, and delete code in LLDB where LLVM provides a solution. In cases where there is an LLVM thing that is similar to what we need, we should extend the LLVM thing to support what we need, and then use it. Following are some areas I’ve identified. This list is by no means complete. For each one, I’ve given a personal assessment of how likely it is to cause some (temporary) hiccups, how much it would help us in the long run, and how difficult it would be to do. Without further ado:
• Use llvm::Regex instead of lldb::Regex
• llvm::Regex doesn’t support enhanced mode. Could we add support for this to llvm::Regex?
• Risk: 6
• Impact: 3
• Difficulty / Effort: 3 (5 if we have to add enhanced mode support)

As long as it supports all of the features, then this is fine. Otherwise we will need to modify the regular expressions to work with the LLVM version or back port any advances regex features we need into the LLVM regex parser.

• Use llvm streams instead of lldb::StreamString
• Supports output re-targeting (stderr, stdout, std::string, etc), printf style formatting, and type-safe streaming operators.
• Interoperates nicely with many existing llvm utility classes
• Risk: 4
• Impact: 5
• Difficulty / Effort: 7

I have worked extensively with both C++ streams and with printf. I don’t find the type safe streaming operators to be readable and a great way to place things into streams. Part of my objection to this will be quelled if the LLVM streams are not stateful like C++ streams are (add an extra “std::hex” into the stream and suddenly other things down stream start coming out in hex). As long as printf functionality is in the LLVM streams I am ok with it.

• Use llvm::Error instead of lldb::Error
• llvm::Error is an error class that requires you to check whether it succeeded or it will assert. In a way, it’s similar to a C++ exception, except that it doesn’t come with the performance hit associated with exceptions. It’s extensible, and can be easily extended to support the various ways LLDB needs to construct errors and error messages.
• Would need to first rename lldb::Error to LLDBError so that te conversion from LLDBError to llvm::Error could be done incrementally.
• Risk: 7
• Impact: 7
• Difficulty / Effort: 8

We must be very careful here. Nothing can crash LLDB and adding something that will be known to crash in some cases can only be changed if there is a test that can guarantee that it won’t crash. assert() calls in a shared library like LLDB are not OK and shouldn’t fire. Even if they do, when asserts are off, then it shouldn’t crash LLDB. We have made efforts to only use asserts in LLDB for things that really can’t happen, but for things like constructing a StringRef with NULL is one example of why I don’t want to use certain classes in LLVM. If we can remove the crash threat, then lets use them. But many people firmly believe that asserts belong in llvm and clang code and I don’t agree.

I’m surprise by your aversion to assertions, what is your suggested alternative? Are you expecting to check and handle every possible invariants everywhere and recover (or signal an error) properly? That does not seem practical, and it seems to me that it’d lead to just involving UB (or breaking design invariant) without actually noticing it.

We have to as a debugger. We get input from a variety of different compilers and we parse and use things there were produced by our toolchains and others. Crashing Xcode because someone accidentally created a llvm::StringRef(NULL) should not happen. It is not OK for library code to crash. This is quite OK for compilers, linkers and other tools, but it isn’t for any other code.

Well, except that no, it is not OK for the compiler either, we want to use it as a library (JIT, etc.).
But it comes back to my point: you are against asserting for enforcing “external input” where there should be sanitization done with proper error handling, which does not say anything about the majority of assertions which are not dealing with user input.

For instance, it is still not clear to me what’s wrong with the asserting Error class mentioned before.

Since there are so many asserts, LLDB code is currently responsible for figuring out what will make clang unhappy and we must be sure to not feed it anything that makes it unhappy or we crash. So I don’t see that as better.

Also many asserts are in header files so even if you build llvm and clang with asserts off, but then build our project that uses LLVM with asserts on, you get LLVM and clang asserts when you don’t want them.

It is not really supported to use the LLVM C++ headers without assertions and link to a LLVM build with assertions (and vice-versa).

As we have found out. We claim llvm and clang can be linked into tools a libraries, but it really means, you should really use it out of process because it might crash at any time so don’t try and use it in a program you actually want to be able run for a long time.

I’m not sure how this sentence relates to the fact that the headers are not ABI insensitive to -DDEBUG.
It seems totally orthogonal.

> • Separate testing tools
> • One question that remains open is how to represent the complicated needs of a debugger in lit tests. Part a) above covers the trivial cases, but what about the difficult cases? In https://reviews.llvm.org/D24591 a number of ideas were discussed. We started getting to this idea towards the end, about a separate tool which has an interface independent of the command line interface and which can be used to test. lldb-mi was mentioned. While I have serious concerns about lldb-mi due to its poorly written and tested codebase, I do agree in principle with the methodology. In fact, this is the entire philosophy behind lit as used with LLVM, clang, lld, etc.

We have a public API... Why are we going to go and spend _any_ time on doing anything else? I just don't get it. What a waste of time. We have a public API. Lets use it. Not simple enough for people? Tough. Testing a debugger should be done using the public API except when we are actually trying to test the LLDB command line in pexpect like tests. Those and only those are fine to covert over to using LIT and use text scraping. But 95% of our testing should be done using the API that our IDEs use. Using MI? Please no.

FWIW, I agree with you about mi.

That said, the public api is problematic. Here you've got an API which, once you do something to, is set in stone forever. And yet in order to test something, you have to add it to the API. So now, in order to test something, you have to make a permanent change to a public API that can never be undone.

It's also a public facing API, when a lot of the stuff we need to be able to look at and inspect is not really suitable for public consumption.

If something is a public API that can never be changed once it's added, then there should be an extremely strict review process in order to add something to it. It should be VERY HARD to modify (it's currently not, which is a debate for another day, but anyway...). And yet that's fundamentally incompatible with having tests be easy to write. When it's hard to add the thing you need to add in order to write the test, then it's hard to write the test.

Furthermore, with a system such as the one I proposed, you could even run tests with LLDB_DISABLE_PYTHON. IDK about you, but that would be *amazing* from my point of view. Even though I spent months making Python 3 work, I would be happy to burn it all with fire and go back to just Python 2 if we had a viable testing strategy.

It would not be amazing. One of the primary benefits of lldb IS the Python API. If you don't think that's wonderful in a debugger, go look at what effort it took to make that happen in gdb once lldb started providing it and you'll see that it was really highly motivated. So switching our testing away from one of our very strong points would be a very bad choice IMHO.

On the "should I add it to the SB API to test it or not question." Most of the time, when I find I need to add an API in order write a test for something, the thing I'm trying to get my hands on is something that other people might be interested in as well. If we go to the mode of adding affordances only for ourselves when writing for testing, we remove one of the times you should be asking yourself "should I add this to the SB API so other people can use it." I'm fine with having an lldb-testing module with API's based on the same methodology as the SB API's. If you think something you are adding is too esoteric to be part of the SB API put it there. But by default put it into the SB API's.

Jim

Obviously I defer to you on whether testing via the SB API is better than what GDB does or used to do. But these are not the only two systems in the world. Having used both LLDB and LLVM’s test suite extensively, I still remain unconvinced that LLDB’s testing situation could not be improved. Do we improve it by doing what GDB did? Obviously not. Can we improve it further by doing something completely different than what GDB did and what LLDB currently does? I remain convinced that we can.

Obviously I defer to you on whether testing via the SB API is better than what GDB does or used to do. But these are not the only two systems in the world. Having used both LLDB and LLVM's test suite extensively, I still remain unconvinced that LLDB's testing situation could not be improved. Do we improve it by doing what GDB did? Obviously not. Can we improve it further by doing something completely different than what GDB did *and* what LLDB currently does? I remain convinced that we can.

I think you misread my message. I said:

Having a Python interface so that developers can program the debugger is very powerful.

As evidence, see all the work the gdb folks did to add one (you have to know a little bit about how gdb works to see how hard this would be, but you can imagine a debugger written in C primarily to dump text to a terminal, think of how you would wrap that in Python and you'll get a sense.)

Now forget about gdb, that was only an example of how much some folks thought a scripting interface was worth.

Instead, focus on this good thing, the scripting interface. We should test this thing that we think is valuable.

What's a good way to make sure we test that?

USE IT FOR TESTING.

Jim