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

[Zachary Turner via lldb-commits]

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

On Wed, Nov 18, 2015 at 5:18 PM Jim Ingham <jingham at apple.com> wrote:

>
> > On Nov 18, 2015, at 4:31 PM, Zachary Turner via lldb-commits <
> lldb-commits at lists.llvm.org> wrote:
> >
> > zturner added a reviewer: clayborg.
> > zturner added a comment.
> >
> > +greg since this touches some SB stuff and the ObjectFile plugins.
> >
> >
> > ================
> > Comment at: include/lldb/API/SBProcess.h:345-346
> > @@ +344,4 @@
> > +    // Save the state of the process in a core file (or mini dump on
> Windows).
> > +    lldb::SBError
> > +    SaveCore(const char *file_name);
> > +
> > ----------------
> > 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.
>
> Once we pass an llvm::StringRef to an SB API it becomes part of the API,
> and changes to llvm::StringRef make us binary incompatible.  Maybe
> llvm::StringRef won't ever change, but there's no guarantee about that, so
> unless we get some very persuasive benefit from this, I'd strongly prefer
> to use more vanilla data types.
>
> Also, I would like to keep the process of compiling against the SB API's
> as simple as possible.  Right now you don't need the llvm headers to build
> an app that just uses the SB API's.  Again, the benefit would have to be
> fairly great before I'd want to add this requirement.
>
> If you want to be more formal about this, then have the API take an
> SBFileSpec.
>
> Jim
>
> >
> >
> >
> > ================
> > 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):
> > ----------------
> > 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 :)
> >
> > ================
> > Comment at:
> packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:32
> > @@ +31,3 @@
> > +        self.assertTrue(process.Kill().Success())
> > +        # Clean up the mini dump file.
> > +        os.unlink(core)
> > ----------------
> > 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.
> >
> > ================
> > Comment at:
> packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:33
> > @@ +32,3 @@
> > +        # Clean up the mini dump file.
> > +        os.unlink(core)
> > +
> > ----------------
> > This isn't exception safe right now.  For example, it won't unlink the
> file if one the asserts fails.
> >
> > ================
> > Comment at:
> source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:17
> > @@ +16,3 @@
> > +
> > +#ifdef _WIN32
> > +#include "lldb/Host/windows/windows.h"
> > ----------------
> > 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.
> >
> >
> > http://reviews.llvm.org/D14793
> >
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20151119/591c1857/attachment.html>