>
> > There was no way to find out what's wrong if SBProcess
> SBTarget::LoadCore(const char *core_file) failed.
> > Additionally, the implementation was unconditionally setting sb_process,
> so it wasn't even possible to check if the return SBProcess is valid.
>
> Not terribly important, but:
>
> >>> process = lldb.target.LoadCore("/not/here/no_core")
> >>> process.IsValid()
> False
>
> Pretty much all the SB objects have an IsValid method to test whether the
> call that made them was successful or not. But IsValid doesn't specify the
> reason why the call didn't work, so having an error parameter is still a
> good thing.
True. In this case the SBTarget::LoadCore() implementation had a bug
though, which was causing it to return a "valid" process even on some
failure situations (when the actual Process::LoadCore() failed).
Full context in the change diff, but the key part is:
...
if (process_sp) {
error.SetError(process_sp->LoadCore());
* if (error.Success()) // this was missing*
sb_process.SetSP(process_sp);
}
...
Beyond this small fix, I also wanted the actual error for external
diagnostic purposes (for example it's valuable to know what was the actual
problem: corrupted, truncated or unsupported minidump, etc.)
On Mon, Jun 11, 2018 at 5:47 PM, Jim Ingham <jing...@apple.com> wrote:
>
>
> > On Jun 11, 2018, at 2:19 PM, Leonard Mosescu via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > Author: lemo
> > Date: Mon Jun 11 14:19:26 2018
> > New Revision: 334439
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=334439&view=rev
> > Log:
> > Add a new SBTarget::LoadCore() overload which surfaces errors if the
> load fails
> >
> > There was no way to find out what's wrong if SBProcess
> SBTarget::LoadCore(const char *core_file) failed.
> > Additionally, the implementation was unconditionally setting sb_process,
> so it wasn't even possible to check if the return SBProcess is valid.
>
> Not terribly important, but:
>
> >>> process = lldb.target.LoadCore("/not/here/no_core")
> >>> process.IsValid()
> False
>
> Pretty much all the SB objects have an IsValid method to test whether the
> call that made them was successful or not. But IsValid doesn't specify the
> reason why the call didn't work, so having an error parameter is still a
> good thing.
>
> Jim
>
>
> >
> > This change adds a new overload which surfaces the errors and also
> returns a valid SBProcess only if the core load succeeds:
> >
> > SBProcess SBTarget::LoadCore(const char *core_file, SBError &error);
> >
> > Differential Revision: https://reviews.llvm.org/D48049
> >
> >
> > Modified:
> > lldb/trunk/include/lldb/API/SBTarget.h
> > lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump-new/TestMiniDumpNew.py
> > lldb/trunk/scripts/interface/SBTarget.i
> > lldb/trunk/source/API/SBTarget.cpp
> >
> > Modified: lldb/trunk/include/lldb/API/SBTarget.h
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/
> lldb/API/SBTarget.h?rev=334439&r1=334438&r2=334439&view=diff
> > ============================================================
> ==================
> > --- lldb/trunk/include/lldb/API/SBTarget.h (original)
> > +++ lldb/trunk/include/lldb/API/SBTarget.h Mon Jun 11 14:19:26 2018
> > @@ -165,6 +165,7 @@ public:
> > bool stop_at_entry, lldb::SBError &error);
> >
> > SBProcess LoadCore(const char *core_file);
> > + SBProcess LoadCore(const char *core_file, lldb::SBError &error);
> >
> > //------------------------------------------------------------------
> > /// Launch a new process with sensible defaults.
> > @@ -718,9 +719,9 @@ public:
> > // Finds all breakpoints by name, returning the list in bkpt_list.
> Returns
> > // false if the name is not a valid breakpoint name, true otherwise.
> > bool FindBreakpointsByName(const char *name, SBBreakpointList
> &bkpt_list);
> > -
> > +
> > void GetBreakpointNames(SBStringList &names);
> > -
> > +
> > void DeleteBreakpointName(const char *name);
> >
> > bool EnableAllBreakpoints();
> >
> > Modified: lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/postmortem/minidump-new/TestMiniDumpNew.py
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/
> Python/lldbsuite/test/functionalities/postmortem/
> minidump-new/TestMiniDumpNew.py?rev=334439&r1=334438&r2=334439&view=diff
> > ============================================================
> ==================
> > --- lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/postmortem/minidump-new/TestMiniDumpNew.py (original)
> > +++ lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/postmortem/minidump-new/TestMiniDumpNew.py Mon Jun 11
> 14:19:26 2018
> > @@ -59,6 +59,24 @@ class MiniDumpNewTestCase(TestBase):
> > self.dbg.SetOutputFileHandle(None, False)
> > self.dbg.SetErrorFileHandle(None, False)
> >
> > + def test_loadcore_error_status(self):
> > + """Test the SBTarget.LoadCore(core, error) overload."""
> > + self.dbg.CreateTarget(None)
> > + self.target = self.dbg.GetSelectedTarget()
> > + error = lldb.SBError()
> > + self.process = self.target.LoadCore("linux-x86_64.dmp", error)
> > + self.assertTrue(self.process, PROCESS_IS_VALID)
> > + self.assertTrue(error.Success())
> > +
> > + def test_loadcore_error_status_failure(self):
> > + """Test the SBTarget.LoadCore(core, error) overload."""
> > + self.dbg.CreateTarget(None)
> > + self.target = self.dbg.GetSelectedTarget()
> > + error = lldb.SBError()
> > + self.process = self.target.LoadCore("non-existent.dmp", error)
> > + self.assertFalse(self.process, PROCESS_IS_VALID)
> > + self.assertTrue(error.Fail())
> > +
> > def test_process_info_in_minidump(self):
> > """Test that lldb can read the process information from the
> Minidump."""
> > # target create -c linux-x86_64.dmp
> >
> > Modified: lldb/trunk/scripts/interface/SBTarget.i
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/scripts/
> interface/SBTarget.i?rev=334439&r1=334438&r2=334439&view=diff
> > ============================================================
> ==================
> > --- lldb/trunk/scripts/interface/SBTarget.i (original)
> > +++ lldb/trunk/scripts/interface/SBTarget.i Mon Jun 11 14:19:26 2018
> > @@ -77,7 +77,7 @@ public:
> >
> > static const char *
> > GetBroadcasterClassName ();
> > -
> > +
> > bool
> > IsValid() const;
> >
> > @@ -145,7 +145,7 @@ public:
> > /// An optional listener that will receive all process events.
> > /// If \a listener is valid then \a listener will listen to all
> > /// process events. If not valid, then this target's debugger
> > - /// (SBTarget::GetDebugger()) will listen to all process
> events.
> > + /// (SBTarget::GetDebugger()) will listen to all process events.
> > ///
> > /// @param[in] argv
> > /// The argument array.
> > @@ -175,7 +175,7 @@ public:
> > /// The working directory to have the child process run in
> > ///
> > /// @param[in] launch_flags
> > - /// Some launch options specified by logical OR'ing
> > + /// Some launch options specified by logical OR'ing
> > /// lldb::LaunchFlags enumeration values together.
> > ///
> > /// @param[in] stop_at_entry
> > @@ -203,7 +203,7 @@ public:
> > run to completion if no user interaction is required.
> > ") Launch;
> > lldb::SBProcess
> > - Launch (SBListener &listener,
> > + Launch (SBListener &listener,
> > char const **argv,
> > char const **envp,
> > const char *stdin_path,
> > @@ -213,7 +213,7 @@ public:
> > uint32_t launch_flags, // See LaunchFlags
> > bool stop_at_entry,
> > lldb::SBError& error);
> > -
> > +
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > /// Launch a new process with sensible defaults.
> > @@ -250,10 +250,10 @@ public:
> > executable.
> > ") LaunchSimple;
> > lldb::SBProcess
> > - LaunchSimple (const char **argv,
> > + LaunchSimple (const char **argv,
> > const char **envp,
> > const char *working_directory);
> > -
> > +
> > lldb::SBProcess
> > Launch (lldb::SBLaunchInfo &launch_info, lldb::SBError& error);
> >
> > @@ -264,6 +264,10 @@ public:
> > /// @param[in] core_file
> > /// File path of the core dump.
> > ///
> > + /// @param[out] error
> > + /// An error explaining what went wrong if the operation fails.
> > + /// (Optional)
> > + ///
> > /// @return
> > /// A process object for the newly created core file.
> > //------------------------------------------------------------------
> > @@ -276,10 +280,12 @@ public:
> > ") LoadCore;
> > lldb::SBProcess
> > LoadCore(const char *core_file);
> > -
> > +
> > lldb::SBProcess
> > - Attach (lldb::SBAttachInfo &attach_info, lldb::SBError& error);
> > -
> > + LoadCore(const char *core_file, lldb::SBError &error);
> > +
> > + lldb::SBProcess
> > + Attach(lldb::SBAttachInfo &attach_info, lldb::SBError& error);
> >
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > @@ -363,7 +369,7 @@ public:
> > const char *url,
> > const char *plugin_name,
> > SBError& error);
> > -
> > +
> > lldb::SBFileSpec
> > GetExecutable ();
> >
> > @@ -401,10 +407,10 @@ public:
> >
> > lldb::ByteOrder
> > GetByteOrder ();
> > -
> > +
> > uint32_t
> > GetAddressByteSize();
> > -
> > +
> > const char *
> > GetTriple ();
> >
> > @@ -457,21 +463,21 @@ public:
> > /// A logical OR of one or more FunctionNameType enum bits that
> > /// indicate what kind of names should be used when doing the
> > /// lookup. Bits include fully qualified names, base names,
> > - /// C++ methods, or ObjC selectors.
> > + /// C++ methods, or ObjC selectors.
> > /// See FunctionNameType for more details.
> > ///
> > /// @return
> > - /// A lldb::SBSymbolContextList that gets filled in with all of
> > + /// A lldb::SBSymbolContextList that gets filled in with all of
> > /// the symbol contexts for all the matches.
> > //------------------------------------------------------------------
> > ") FindFunctions;
> > lldb::SBSymbolContextList
> > - FindFunctions (const char *name,
> > + FindFunctions (const char *name,
> > uint32_t name_type_mask = lldb::eFunctionNameTypeAny);
> > -
> > +
> > lldb::SBType
> > FindFirstType (const char* type);
> > -
> > +
> > lldb::SBTypeList
> > FindTypes (const char* type);
> >
> > @@ -497,7 +503,7 @@ public:
> > //------------------------------------------------------------------
> > ") FindGlobalVariables;
> > lldb::SBValueList
> > - FindGlobalVariables (const char *name,
> > + FindGlobalVariables (const char *name,
> > uint32_t max_matches);
> >
> > %feature("docstring", "
> > @@ -515,7 +521,7 @@ public:
> > lldb::SBValue
> > FindFirstGlobalVariable (const char* name);
> >
> > -
> > +
> > lldb::SBValueList
> > FindGlobalVariables(const char *name,
> > uint32_t max_matches,
> > @@ -544,26 +550,26 @@ public:
> >
> > lldb::SBAddress
> > ResolveLoadAddress (lldb::addr_t vm_addr);
> > -
> > +
> > lldb::SBAddress
> > ResolvePastLoadAddress (uint32_t stop_id, lldb::addr_t vm_addr);
> >
> > SBSymbolContext
> > - ResolveSymbolContextForAddress (const SBAddress& addr,
> > + ResolveSymbolContextForAddress (const SBAddress& addr,
> > uint32_t resolve_scope);
> >
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > - /// Read target memory. If a target process is running then memory
> > + /// Read target memory. If a target process is running then memory
> > /// is read from here. Otherwise the memory is read from the object
> > /// files. For a target whose bytes are sized as a multiple of host
> > /// bytes, the data read back will preserve the target's byte order.
> > ///
> > /// @param[in] addr
> > - /// A target address to read from.
> > + /// A target address to read from.
> > ///
> > /// @param[out] buf
> > - /// The buffer to read memory into.
> > + /// The buffer to read memory into.
> > ///
> > /// @param[in] size
> > /// The maximum number of host bytes to read in the buffer passed
> > @@ -589,7 +595,7 @@ public:
> > BreakpointCreateByLocation (const lldb::SBFileSpec &file_spec,
> uint32_t line, lldb::addr_t offset);
> >
> > lldb::SBBreakpoint
> > - BreakpointCreateByLocation (const lldb::SBFileSpec &file_spec,
> uint32_t line,
> > + BreakpointCreateByLocation (const lldb::SBFileSpec &file_spec,
> uint32_t line,
> > lldb::addr_t offset, SBFileSpecList
> &module_list);
> >
> > lldb::SBBreakpoint
> > @@ -598,14 +604,14 @@ public:
> > lldb::SBBreakpoint
> > BreakpointCreateByName (const char *symbol_name,
> > uint32_t func_name_type, //
> Logical OR one or more FunctionNameType enum bits
> > - const SBFileSpecList &module_list,
> > + const SBFileSpecList &module_list,
> > const SBFileSpecList &comp_unit_list);
> >
> > lldb::SBBreakpoint
> > BreakpointCreateByName (const char *symbol_name,
> > uint32_t func_name_type, //
> Logical OR one or more FunctionNameType enum bits
> > lldb::LanguageType symbol_language,
> > - const SBFileSpecList &module_list,
> > + const SBFileSpecList &module_list,
> > const SBFileSpecList &comp_unit_list);
> >
> > %typemap(in) (const char **symbol_name, uint32_t num_names) {
> > @@ -670,7 +676,7 @@ public:
> > lldb::SBBreakpoint
> > BreakpointCreateByRegex (const char *symbol_name_regex,
> > lldb::LanguageType symbol_language,
> > - const SBFileSpecList &module_list,
> > + const SBFileSpecList &module_list,
> > const SBFileSpecList &comp_unit_list);
> >
> > lldb::SBBreakpoint
> > @@ -708,7 +714,7 @@ public:
> > lldb::SBBreakpoint
> > FindBreakpointByID (break_id_t break_id);
> >
> > -
> > +
> > bool FindBreakpointsByName(const char *name, SBBreakpointList
> &bkpt_list);
> >
> > void DeleteBreakpointName(const char *name);
> > @@ -726,12 +732,12 @@ public:
> >
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > - /// Read breakpoints from source_file and return the newly created
> > + /// Read breakpoints from source_file and return the newly created
> > /// breakpoints in bkpt_list.
> > ///
> > /// @param[in] source_file
> > /// The file from which to read the breakpoints
> > - ///
> > + ///
> > /// @param[out] bkpt_list
> > /// A list of the newly created breakpoints.
> > ///
> > @@ -740,12 +746,12 @@ public:
> > //------------------------------------------------------------------
> > ") BreakpointsCreateFromFile;
> > lldb::SBError
> > - BreakpointsCreateFromFile(SBFileSpec &source_file,
> > + BreakpointsCreateFromFile(SBFileSpec &source_file,
> > SBBreakpointList &bkpt_list);
> >
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > - /// Read breakpoints from source_file and return the newly created
> > + /// Read breakpoints from source_file and return the newly created
> > /// breakpoints in bkpt_list.
> > ///
> > /// @param[in] source_file
> > @@ -754,7 +760,7 @@ public:
> > /// @param[in] matching_names
> > /// Only read in breakpoints whose names match one of the names
> in this
> > /// list.
> > - ///
> > + ///
> > /// @param[out] bkpt_list
> > /// A list of the newly created breakpoints.
> > ///
> > @@ -779,7 +785,7 @@ public:
> > ") BreakpointsCreateFromFile;
> > lldb::SBError
> > BreakpointsWriteToFile(SBFileSpec &dest_file);
> > -
> > +
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > /// Write breakpoints listed in bkpt_list to dest_file.
> > @@ -800,42 +806,42 @@ public:
> > //------------------------------------------------------------------
> > ") BreakpointsCreateFromFile;
> > lldb::SBError
> > - BreakpointsWriteToFile(SBFileSpec &dest_file,
> > + BreakpointsWriteToFile(SBFileSpec &dest_file,
> > SBBreakpointList &bkpt_list,
> > bool append = false);
> >
> > uint32_t
> > GetNumWatchpoints () const;
> > -
> > +
> > lldb::SBWatchpoint
> > GetWatchpointAtIndex (uint32_t idx) const;
> > -
> > +
> > bool
> > DeleteWatchpoint (lldb::watch_id_t watch_id);
> > -
> > +
> > lldb::SBWatchpoint
> > FindWatchpointByID (lldb::watch_id_t watch_id);
> > -
> > +
> > bool
> > EnableAllWatchpoints ();
> > -
> > +
> > bool
> > DisableAllWatchpoints ();
> > -
> > +
> > bool
> > DeleteAllWatchpoints ();
> >
> > lldb::SBWatchpoint
> > - WatchAddress (lldb::addr_t addr,
> > - size_t size,
> > - bool read,
> > + WatchAddress (lldb::addr_t addr,
> > + size_t size,
> > + bool read,
> > bool write,
> > SBError &error);
> > -
> > +
> >
> > lldb::SBBroadcaster
> > GetBroadcaster () const;
> > -
> > +
> > %feature("docstring", "
> > //------------------------------------------------------------------
> > /// Create an SBValue with the given name by treating the memory
> starting at addr as an entity of type.
> > @@ -859,20 +865,20 @@ public:
> >
> > lldb::SBValue
> > CreateValueFromData (const char *name, lldb::SBData data,
> lldb::SBType type);
> > -
> > +
> > lldb::SBValue
> > CreateValueFromExpression (const char *name, const char* expr);
> > -
> > +
> > %feature("docstring", "
> > Disassemble a specified number of instructions starting at an
> address.
> > Parameters:
> > base_addr -- the address to start disassembly from
> > count -- the number of instructions to disassemble
> > flavor_string -- may be 'intel' or 'att' on x86 targets to
> specify that style of disassembly
> > - Returns an SBInstructionList.")
> > + Returns an SBInstructionList.")
> > ReadInstructions;
> > lldb::SBInstructionList
> > - ReadInstructions (lldb::SBAddress base_addr, uint32_t count);
> > + ReadInstructions (lldb::SBAddress base_addr, uint32_t count);
> >
> > lldb::SBInstructionList
> > ReadInstructions (lldb::SBAddress base_addr, uint32_t count, const
> char *flavor_string);
> > @@ -883,7 +889,7 @@ public:
> > base_addr -- used for symbolicating the offsets in the byte
> stream when disassembling
> > buf -- bytes to be disassembled
> > size -- (C++) size of the buffer
> > - Returns an SBInstructionList.")
> > + Returns an SBInstructionList.")
> > GetInstructions;
> > lldb::SBInstructionList
> > GetInstructions (lldb::SBAddress base_addr, const void *buf, size_t
> size);
> > @@ -895,17 +901,17 @@ public:
> > flavor -- may be 'intel' or 'att' on x86 targets to specify
> that style of disassembly
> > buf -- bytes to be disassembled
> > size -- (C++) size of the buffer
> > - Returns an SBInstructionList.")
> > + Returns an SBInstructionList.")
> > GetInstructionsWithFlavor;
> > lldb::SBInstructionList
> > GetInstructionsWithFlavor (lldb::SBAddress base_addr, const char
> *flavor_string, const void *buf, size_t size);
> > -
> > +
> > lldb::SBSymbolContextList
> > FindSymbols (const char *name, lldb::SymbolType type =
> eSymbolTypeAny);
> >
> > bool
> > GetDescription (lldb::SBStream &description, lldb::DescriptionLevel
> description_level);
> > -
> > +
> > lldb::addr_t
> > GetStackRedZoneSize();
> >
> > @@ -934,12 +940,12 @@ public:
> > '''A helper object that will lazily hand out lldb.SBModule
> objects for a target when supplied an index, or by full or partial path.'''
> > def __init__(self, sbtarget):
> > self.sbtarget = sbtarget
> > -
> > +
> > def __len__(self):
> > if self.sbtarget:
> > return int(self.sbtarget.GetNumModules())
> > return 0
> > -
> > +
> > def __getitem__(self, key):
> > num_modules = self.sbtarget.GetNumModules()
> > if type(key) is int:
> > @@ -982,11 +988,11 @@ public:
> > else:
> > print("error: unsupported item type: %s" % type(key))
> > return None
> > -
> > +
> > def get_modules_access_object(self):
> > '''An accessor function that returns a modules_access()
> object which allows lazy module access from a lldb.SBTarget object.'''
> > return self.modules_access (self)
> > -
> > +
> > def get_modules_array(self):
> > '''An accessor function that returns a list() that contains
> all modules in a lldb.SBTarget object.'''
> > modules = []
> > @@ -1017,13 +1023,13 @@ public:
> >
> > __swig_getmethods__["broadcaster"] = GetBroadcaster
> > if _newclass: broadcaster = property(GetBroadcaster, None,
> doc='''A read only property that an lldb object that represents the
> broadcaster (lldb.SBBroadcaster) for this target.''')
> > -
> > +
> > __swig_getmethods__["byte_order"] = GetByteOrder
> > if _newclass: byte_order = property(GetByteOrder, None, doc='''A
> read only property that returns an lldb enumeration value
> (lldb.eByteOrderLittle, lldb.eByteOrderBig, lldb.eByteOrderInvalid) that
> represents the byte order for this target.''')
> > -
> > +
> > __swig_getmethods__["addr_size"] = GetAddressByteSize
> > if _newclass: addr_size = property(GetAddressByteSize, None,
> doc='''A read only property that returns the size in bytes of an address
> for this target.''')
> > -
> > +
> > __swig_getmethods__["triple"] = GetTriple
> > if _newclass: triple = property(GetTriple, None, doc='''A read
> only property that returns the target triple (arch-vendor-os) for this
> target as a string.''')
> >
> >
> > Modified: lldb/trunk/source/API/SBTarget.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/
> SBTarget.cpp?rev=334439&r1=334438&r2=334439&view=diff
> > ============================================================
> ==================
> > --- lldb/trunk/source/API/SBTarget.cpp (original)
> > +++ lldb/trunk/source/API/SBTarget.cpp Mon Jun 11 14:19:26 2018
> > @@ -203,6 +203,11 @@ SBStructuredData SBTarget::GetStatistics
> > }
> >
> > SBProcess SBTarget::LoadCore(const char *core_file) {
> > + lldb::SBError error; // Ignored
> > + return LoadCore(core_file, error);
> > +}
> > +
> > +SBProcess SBTarget::LoadCore(const char *core_file, lldb::SBError
> &error) {
> > SBProcess sb_process;
> > TargetSP target_sp(GetSP());
> > if (target_sp) {
> > @@ -210,9 +215,14 @@ SBProcess SBTarget::LoadCore(const char
> > ProcessSP process_sp(target_sp->CreateProcess(
> > target_sp->GetDebugger().GetListener(), "", &filespec));
> > if (process_sp) {
> > - process_sp->LoadCore();
> > - sb_process.SetSP(process_sp);
> > + error.SetError(process_sp->LoadCore());
> > + if (error.Success())
> > + sb_process.SetSP(process_sp);
> > + } else {
> > + error.SetErrorString("Failed to create the process");
> > }
> > + } else {
> > + error.SetErrorString("SBTarget is invalid");
> > }
> > return sb_process;
> > }
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
