[Lldb-commits] [PATCH] D134991: [lldb] Add diagnostics

Mon Oct 3 10:55:20 PDT 2022

JDevlieghere added a comment.

In D134991#3830682 <https://reviews.llvm.org/D134991#3830682>, @DavidSpickett wrote:

> I like the direction, thanks for working on this.
>
> To no one's surprise, I was thinking about testing :)
>
> - Errors related to the filesystem e.g. can't open a path, is too complicated to setup on demand. The code looks to be passing on the error, which is good enough most of the time.
> - The callbacks are added in c++, so testing if you add A then remove B and so on doesn't make sense on the same copy of lldb (and upstream will always have the same set of callbacks anyway).
> - Dumping statistics uses an existing method, you just make up the filename. So it's the statistics code's responsibility to test what that function does.
>
> Maybe there could be a smoke test that just checks that the dir is made and has some files ending in `stats.json` in it? I think clang does something like this though they may have a `--please-crash` option too.

I appreciate you thinking this through. Back in the days of the reproducers, we would generate a reproducer on a crash (similar to what we do here) or through a command (`reproducer generate`). I think we'll want to a command here too for the case where something went wrong but without crashing LLDB. I think that part should be easy to test and I was planning on adding a test as part of that.

> Unrelated - there's maybe a situation where the same set of callbacks are added in a different order, perhaps? But am I correct in thinking that this isn't an issue because these will be writing to different files? Stats has one set of files, logging has another set, etc. So the end result is always a dir of separate files one or more for each thing that registers.

Yeah, in my mind all the callbacks should be orthogonal. If it wasn't for the layering, I'd make it all the responsibility of the Diagnostics class. If someone has a better idea than callbacks please let me know.

In D134991#3830744 <https://reviews.llvm.org/D134991#3830744>, @labath wrote:

> Adding @rupprecht, as he might be interested in following this.
>
> I don't want to get too involved in this, but the first thought on my mind is "do you really want to do all this from within a signal handler?". The fact that we're running this code means that we're already in a bad state, and its pretty hard to say how far we will manage to get without triggering another crash.

We had a similar issue for the reproducers. In my experience, we mostly got away with it, but as you said, there's no guarantees. It all depends on what exactly what the callback is doing. Maybe that's something that should be controllable: e.g. some callbacks might want to do more/less based on whether they're called from a signal handler vs when they're triggered from a command.

That said, I see the crash case as a "best effort" kind of thing.

> At the very least, I think we should print the crash dump directory as the first order of business, before we start running random callbacks.

That's a good point.

> There are various ways to solve that, like moving the dumping code to another process, or being very careful about what you run inside the crash handler. The one thing that they all have in common is that they are harder to design/implement that what you have now. So, if you want try this out, and accept the risk that it may not be enough to capture all the crashes you're interested in, then I won't stand in your way...

Yup that's the approach we took for the reproducer. If you remember, we found that copying a bunch of files wasn't the best thing to do in a signal handler (surprise surprise) so we moved that work out of process (but still wrote out the mapping, similar to clang). I think we can take a similar approach here.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134991/new/

https://reviews.llvm.org/D134991