This is an archive of the discontinued LLVM Phabricator instance.

Enable saving of mini dumps with lldb process save-core.
ClosedPublic

Authored by amccarth on Nov 18 2015, 4:18 PM.

Details

Summary

Enable the saving of a mini dump (on Windows) using the existing lldb save-core command.

Also plumbs a new method into SBProcess, and adds a very basic test to make sure it produces a file.

Future tests will open the mini dump in LLDB and make sure that it has the right information.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth updated this revision to Diff 40575.Nov 18 2015, 4:18 PM
amccarth retitled this revision from to Enable saving of mini dumps with lldb process save-core..
amccarth updated this object.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.

+greg since this touches some SB stuff and the ObjectFile plugins.

include/lldb/API/SBProcess.h
345–346 ↗(On Diff #40575)

Greg, is there any reason why this couldn't be an llvm::StringRef? I know historically we have used this char* mechanism, but it seems like StringRef could be a better, safer alternative than passing raw strings through the SB API. As long as you provide an appropriate typemap, everything should work out.

packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py
20 ↗(On Diff #40575)

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 :)

32 ↗(On Diff #40575)

Can you assert true that the file exists? Is there some thing additional that we can add where you then load the core and try to read something out (say the executable name), and assert that it's equal to a.out?

Right now all we know is that there's a file.

33 ↗(On Diff #40575)

This isn't exception safe right now. For example, it won't unlink the file if one the asserts fails.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp
17 ↗(On Diff #40575)

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.

clayborg requested changes to this revision.Nov 18 2015, 4:51 PM
clayborg edited edge metadata.

See inlined comments.

include/lldb/API/SBProcess.h
344–346 ↗(On Diff #40575)

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.

source/API/SBProcess.cpp
1433–1437 ↗(On Diff #40575)

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;
}
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp
52 ↗(On Diff #40575)

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

This revision now requires changes to proceed.Nov 18 2015, 4:51 PM

Code correction and added that we should make sure the process is alive and stopped.

source/API/SBProcess.cpp
1433–1437 ↗(On Diff #40575)

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).

Fair enough. I figured it was worth asking about, but the rationale makes
sense.

zturner added inline comments.Nov 18 2015, 7:03 PM
source/API/SBProcess.cpp
1433–1437 ↗(On Diff #40575)

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?

Yes, I agree Zach, the caller of SaveCore should verify the process is in a good state first. The only downside of that is if the windows mini-dumper can actually make a core file on a running process, we might want to allow this and make actual SaveCore implementations make that call?

Yes, I agree Zach, the caller of SaveCore should verify the process is in a good state first. The only downside of that is if the windows mini-dumper can actually make a core file on a running process, we might want to allow this and make actual SaveCore implementations make that call?

Windows has to have a stopped process before you can make a core. In other windows debuggers (MSVC and WinDbg, for example) the user of the debugger has to issue a stop command before the "Save Core Dump" command will work.

OK, then callers of SaveCore should make sure the process is in a good state before calling the API.

amccarth marked 5 inline comments as done.Nov 19 2015, 4:16 PM
amccarth added inline comments.
include/lldb/API/SBProcess.h
344–346 ↗(On Diff #40575)

Acknowledged.

packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py
20 ↗(On Diff #40575)

Where is not_in defined?

source/API/SBProcess.cpp
1433–1437 ↗(On Diff #40575)

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.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp
17 ↗(On Diff #40575)

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.

52 ↗(On Diff #40575)

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.

amccarth updated this revision to Diff 40716.Nov 19 2015, 4:17 PM
amccarth edited edge metadata.
amccarth marked an inline comment as done.

Addresses some of the comments.

clayborg requested changes to this revision.Nov 19 2015, 4:40 PM
clayborg edited edge metadata.

Fix the caps on the error message, remove the new files and inline the code and this will be ready.

source/API/SBProcess.cpp
1445 ↗(On Diff #40716)

remove the caps on "The" so it it just "the".

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
159 ↗(On Diff #40716)

I would rather just have the code from SaveMiniDump inlined in this file, no need for a new file just for one function.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp
52–53 ↗(On Diff #40716)

That is fine and returning false makes more sense after reading your above comment.

53–55 ↗(On Diff #40716)

Remove this file and inline the code into ObjectFilePECOFF::SaveCore().

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.h
1–24 ↗(On Diff #40716)

Remove this file and inline the code into ObjectFilePECOFF::SaveCore().

This revision now requires changes to proceed.Nov 19 2015, 4:40 PM
amccarth marked an inline comment as done.Nov 19 2015, 4:48 PM
amccarth added inline comments.
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
159 ↗(On Diff #40716)

I would prefer that too, but it's not possible.

This implementation depends on Windows headers which create conflicts with identifiers used in the main file. (I had a note about that somewhere. If I can't find it, I'll add a comment explanining that.)

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp
53–55 ↗(On Diff #40716)

Can't do, as explained.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.h
1–24 ↗(On Diff #40716)

Can't do as explained in previous response.

Ok, so fix the caps and rename the file to be WindowsMiniDump.cpp/.h. I would rather it not have the "ObjectFilePECOFF" prefix because it would indicated that it is a new subclass of ObjectFile.

amccarth marked an inline comment as not done.Nov 20 2015, 2:49 PM

The new files are renamed per clayborg's suggestion.

packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py
21 ↗(On Diff #40716)

Given that this is proving to be a diversion right now, I'm going back to skipUnlessWindows.

amccarth updated this revision to Diff 40835.Nov 20 2015, 2:51 PM
amccarth edited edge metadata.

Renames the files per the suggestion and switches back to the @skipIfWindows decorator on the tests (for now).

clayborg accepted this revision.Nov 20 2015, 3:05 PM
clayborg edited edge metadata.

Looks good. Thanks for all the fixes.

This revision is now accepted and ready to land.Nov 20 2015, 3:05 PM
This revision was automatically updated to reflect the committed changes.

Greg: Since this adds a new file and I don't have a Mac, can you update
the Xcode project?

Yep, I took care of that shortly after your commit.