[Lldb-commits] [PATCH] D74657: [lldb/Plugins] Add `platform process crash-info` command

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks a bit better, but my the purpose of my previous command was not to move the location of the cli command itself -- I think it was ok just where it was. The fact that the part of the functionality is implemented in the Platform class does not mean the cli command has to be there.



================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1574-1577
+            interpreter, "platform process",
+            "Commands to query, launch and attach to "
+            "processes on the current platform.",
+            "platform process [attach|launch|list|crash-info] ...") {
----------------
I am not sure if this command really belongs here. If definitely doesn't fit the description "Commands to query, launch and attach to processes on the current platform.", and if you look at the subcommands, you see that these are all things that you do *before* you get an actual running process. "crash-info" is the exact opposite -- it only makes sense *after* you have a process.

The "process" (where you had it originally) command tree contains commands for interaction with a running process (although it also overlaps these commands somewhat :/). This command is pretty similar to "process status" so I think it makes sense for it to be next to it (in fact, it is so similar, that I'd consider putting this under some "verbose" switch of process status).


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h:93
+  // This struct should only be available in macOS but could be used on other
+  // platforms. #ifdef HAVE_CRASHREPORTERCLIENT_H #include
+  // <CrashReporterClient.h> #endif
----------------
JDevlieghere wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > Assuming the header defines this struct, why not do 
> > > 
> > > ```
> > > #ifdef HAVE_CRASHREPORTERCLIENT_H 
> > > #include <CrashReporterClient.h> 
> > > #else 
> > > struct CrashInfoAnnotations {
> > >   ...
> > > }
> > > #endif 
> > > ```
> > The struct have a different name in the header (which is not a big deal), but I don't see the point of including the header if I give the structure definition below it.
> > 
> > I put the header comment following @teemperor's feedback (https://reviews.llvm.org/D74657#inline-679127) but maybe I could get rid of it ?
> > 
> > 
> I'll leave it up to your discretion, but I'd either specify the struct myself and say it's defined in `CrashReporterClient.h`, or alternatively put the ifdefs in the code and include the header and typedef it if the header is available, and use the custom struct otherwise, which means we still have the layout and type info Raphael asked about. 
I don't think it makes sense to include the system definition if you're going to provide the alternative -- either you support both cross and native scenarios (in which case you can use the hand-rolled version everywhere), or you just support the native one (and you just use the system version). That said, if the struct contains pointer members, I am not sure if you can actually use the native struct like that. What if the process you're debugging is 32-bit ?

And whatever you do, please don't include a system header in the middle of a class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74657