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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
