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

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments.


================
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);
+
----------------
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.

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

================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:52
@@ +51,3 @@
+#endif
+    return false;
+}
----------------
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


http://reviews.llvm.org/D14793