[Lldb-commits] [PATCH] D14793: Enable saving of mini dumps with lldb process save-core.

[Adrian McCarthy via lldb-commits]

amccarth marked 5 inline comments as done.

================
Comment at: include/lldb/API/SBProcess.h:344-346
@@ -343,1 +343,5 @@
 
+    // Save the state of the process in a core file (or mini dump on Windows).
+    lldb::SBError
+    SaveCore(const char *file_name);
+
----------------
clayborg wrote:
> I don't want any llvm in our public API. Especially llvm::StringRef as it likes to assert when people use it incorrectly (construct one with nullptr and it will assert). So no llvm or clang in our public API.
Acknowledged.

================
Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:20
@@ +19,3 @@
+    @not_remote_testsuite_ready
+    @skipUnlessWindows
+    def test_windows_mini_dump (self):
----------------
zturner wrote:
> How do you feel about `@skipIf(oslist=not_in(['windows']))`?
> 
> The generic skipIf and expectedFailure decorators were extended recently and are now flexible enough to handle unless cases, so I'm partial to the idea of eventually killing off the rest of the decorators, and just having a single one that handles everything.  But I want to dogfood the idea first and see what people think about it :)
Where is not_in defined?

================
Comment at: source/API/SBProcess.cpp:1433-1437
@@ +1432,7 @@
+{
+    ProcessSP process_sp(GetSP());
+    lldb::SBFileSpec core_file(file_name);
+    lldb::SBError error;
+    error.SetError(PluginManager::SaveCore(process_sp, core_file.get()));
+    return error;
+}
----------------
zturner wrote:
> clayborg wrote:
> > clayborg wrote:
> > > You need to check your process_sp before using it since it comes from a std::weak_ptr<>. You also need to take the process run lock. You also don't need to create a lldb::SBFileSpec since you actually need a lldb_private::FileSpec. The code should be:
> > > 
> > > ```
> > > lldb::SBError
> > > SBProcess::SaveCore(const char *file_name)
> > > {
> > >     lldb::SBError error;
> > >     if (process_sp)
> > >     {
> > >         Mutex::Locker api_locker (process_sp->GetTarget().GetAPIMutex());
> > >         FileSpec core_file(file_name, false);
> > >         error.ref() = PluginManager::SaveCore(process_sp, core_file.get());
> > >     }
> > >     else
> > >         error.SetErrorString ("SBProcess is invalid");
> > >     return error;
> > > }
> > > ```
> > Code correction:
> > ```
> > lldb::SBError
> > SBProcess::SaveCore(const char *file_name)
> > {
> >     lldb::SBError error;
> >     if (process_sp)
> >     {
> >         Mutex::Locker api_locker (process_sp->GetTarget().GetAPIMutex());
> >         FileSpec core_file(file_name, false);
> >         error.ref() = PluginManager::SaveCore(process_sp, core_file);
> >     }
> >     else
> >         error.SetErrorString ("SBProcess is invalid");
> >     return error;
> > }
> > ```
> > 
> > We might also want to check to make sure the process is alive (the "save-core" command makes sure the process has been launched) and we should also make sure it does the right thing if the process is running (stop the process, make a core file, then resume if the process was running, or just fail if the process is running).
> I actually wonder if it should just generate an error if the process is not stopped.  Putting a bunch of extra logic in the implementation of `SaveCore()` seems counter to the "thin" nature of the wrapping.  Seems to me like the person using the SB API should stop it themselves before calling `SaveCore`.  
> 
> What do you think?
OK, I check process_sp, I take the mutex, and I ensure that the process is already stopped.

I also added another test to make sure SaveCore fails if the process is not stopped.

================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:17
@@ +16,3 @@
+
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
----------------
zturner wrote:
> I think it would be better to just not even compile this file on non-Windows.  Exclude it at the CMake level, and have `SaveCore()` return false directly.  In the future we may add a way to deal with mini dumps outside of the Windows API, and then we can add that as a separate file like `ObjectFilePECOFFMiniDumpRaw.cpp` which can be used always, and then delete this one.
But if I pull this file out, then I have to have special casing in both the CMake and ObjectFilePECOFF file, which seems a violation of DRY.

If we make a general mini dump writer for any platform, then this just replaces this implementation.

================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:52
@@ +51,3 @@
+#endif
+    return false;
+}
----------------
clayborg wrote:
> You should fill in the error and probably use a #else clause:
> 
> #else
>     error.SetErrorString("windows mini dumps are only supported on native windows machines");
>     return false;
> #endif
Setting the error here would create a subtle bug.

Returning false will cause the PluginManager to keep trying other plugins to see if there's one that can handle it.  If none of the plugins succeed, it produces a "no ObjectFile plugins were able to save a core for this process" message.  But if one of them does, it returns whatever the last error set was.

It seems lame that PluginManager::SaveCore is sharing the same instance of the error result across attempts.  I'd be happy to fix it there.

As for the #else, that was my original inclination, but I've been told that LLVM/LLDB style is to prefer early returns.


http://reviews.llvm.org/D14793