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

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
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;
+}
----------------
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?


http://reviews.llvm.org/D14793