[Lldb-commits] [PATCH] D135622: [lldb] Add a "diagnostics dump" command

Tue Oct 25 08:04:14 PDT 2022

JDevlieghere added inline comments.

================
Comment at: lldb/source/Utility/Diagnostics.cpp:70

-  Error error = Create(FileSpec(diagnostics_dir.str()));
+  Error error = Create(*diagnostics_dir);
   if (error) {
----------------
DavidSpickett wrote:
> JDevlieghere wrote:
> > DavidSpickett wrote:
> > > Silly question, is it intentional that we write out files after saying they have been written?
> > > 
> > > Of course it makes sense to print something up front, otherwise the error here would come out of the blue, but perhaps "LLDB diagnostics will be written to..." instead?
> > > 
> > > (this is probably better changed in that earlier patch not sure if that went in yet)
> > Yes, that was @labath's suggestion because when we call this from the signal handler, we might crash again in the process and wouldn't print anything at all. 
> Maybe I can be clearer:
> ```
> LLDB diagnostics written to <...>
> Please include the directory content when filing a bug report
> <we crash during writing the files>
> ```
> 
> So the files haven't been written, or at least they are in an undefined state, but the output looks like we finished writing them. So `will be written` is more accurate. If I see this in a test log file I'm going to wonder did it crash writing them or did it already write them, which means I'd have to reproduce locally. With the sequence of events clearer, I can at least check obvious things like a full disk before needing to do that.
> 
> Small thing but it's a few words might save some annoyance down the road.
> 
> (printing up front I agree with, that part is good)
Makes sense!

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

https://reviews.llvm.org/D135622