[Lldb-commits] Need to change swig typemap for reading C strings

Wed Nov 15 19:47:19 PST 2017

Yea, if there is a valid string there, it should definitely be returning
"", hard to argue with that.

On Wed, Nov 15, 2017 at 7:06 PM Jason Molenda <jmolenda at apple.com> wrote:

> The general point you're making is reasonable, and something like
> Thread::GetStopDescription is not clear which the correct behavior should
> be.
>
> But I think we can all agree that Process::ReadCStringFromMemory()
> returning a None object when there is an empty c-string is incorrect.
> People are writing code calling Process::ReadCStringFromMemory(), checking
> the SBError object if it was successful, and then treating the return value
> as if it were a string, which is reasonable to do.
>
> I'll check in the fix tomorrow, and update the TestMiniDumpNew.py test,
> thanks.
>
> J
>
> > On Nov 15, 2017, at 6:56 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > I don't really feel strongly about how you fix it.  I'm sure there was a
> good reason for making it do that, but at this point I don't remember what
> it is and I don't think undoing it will cause a problem.
> >
> > That said, part of the difficulty of having an API such as this is that
> Hyrum's Law [http://www.hyrumslaw.com/] is going to continue to bite
> you.  The reason this broke is because there was no test guaranteeing that
> this behavior that people were relying on did not change.
> >
> > As a general rule, it's impossible to guarantee that no observable
> behavior of an API will ever change, so unless there was a test for the
> original behavior (which is documentation that this is behavior people are
> allowed to rely on), I don't really consider it broken.
> >
> > Even if we are in the business of guaranteeing that the API itself won't
> change, we really can't be in the business of guaranteeing that no
> observable behavior of the API will ever change.  That's going to be an
> endless maintenance nightmare.
> >
> > On Wed, Nov 15, 2017 at 6:47 PM Jason Molenda <jmolenda at apple.com>
> wrote:
> > Hi Zachary, reviving a problem I initially found a year ago -- in
> r253487 where a swig typemap was changed for string reading methods.
> >
> > The problem we're seeing with this change is that
> SBProcess::ReadCStringFromMemory() now returns a None object when you try
> to read a zero-length string, and this is breaking a couple of groups'
> python scripts here at Apple.  (it was a low priority thing for me to fix,
> until some behavior changed recently and it started biting the groups a lot
> more frequently).
> >
> > SBProcess::ReadCStringFromMemory() takes an SBError object and returns a
> string.  We have three cases:
> >
> >
> > 1 Non-zero length string read.  SBError says it was successful, string
> should be returned.
> >
> > 2 Zero-length / empty string read.  SBError says it was successful,
> string should be returned (Python None object is returned right now)
> >
> > 3 Memory read error.  SBError says it is an error, ideally None object
> should be returned.
> >
> >
> > For instance,
> >
> > #include <stdlib.h>
> > int main ()
> > {
> >     const char *empty_string = "";
> >     const char *one_letter_string = "1";
> >     const char *invalid_memory_string = (char*)0x100; // lower 4k is
> always PAGEZERO & unreadable on darwin
> >     return empty_string[0] + one_letter_string[0]; // breakpoint here
> > }
> >
> > we'll see:
> >
> >   (lldb) br s -p breakpoint
> >   (lldb) r
> >   (lldb) p empty_string
> >   (const char *) $0 = 0x0000000100000fae ""
> >   (lldb) p one_letter_string
> >   (const char *) $1 = 0x0000000100000faf "1"
> >   (lldb) p invalid_memory_string
> >   (const char *) $2 = 0x0000000000000100 ""
> >   (lldb) scri
> >   >>> err = lldb.SBError()
> >
> >   >>> print lldb.process.ReadCStringFromMemory(0x0000000100000fae, 2048,
> err)
> >   None
> >   >>> print err
> >   success
> >
> >   >>> print lldb.process.ReadCStringFromMemory(0x0000000100000faf, 2048,
> err)
> >   1
> >   >>> print err
> >   success
> >
> >   >>> print lldb.process.ReadCStringFromMemory(0x0000000000000100, 2048,
> err)
> >   None
> >   >>> print err
> >   error: memory read failed for 0x0
> >   >>> print err.Success()
> >   False
> >   >>>
> >
> >
> > Before r253487, the read of a zero-length string and the read of an
> invalid address would both return a zero length python string (and the
> latter would set the SBError).  After the change, both of these return a
> python None object (and the latter sets the SBError).
> >
> >
> > I haven't worked with the typemaps before -- I can restore the previous
> behavior where an empty Python string is returned for both the zero-length
> string and for the unreadable address.  I don't see how I can access the
> SBError object used earlier in these methods.
> >
> >
> > diff --git i/scripts/Python/python-typemaps.swig
> w/scripts/Python/python-typemaps.swig
> > index df16a6d27b3..29e5d9b156d 100644
> > --- i/scripts/Python/python-typemaps.swig
> > +++ w/scripts/Python/python-typemaps.swig
> > @@ -102,7 +102,8 @@
> >  %typemap(argout) (char *dst, size_t dst_len) {
> >     Py_XDECREF($result);   /* Blow away any previous result */
> >     if (result == 0) {
> > -      $result = Py_None;
> > +      lldb_private::PythonString string("");
> > +      $result = string.release();
> >        Py_INCREF($result);
> >     } else {
> >        llvm::StringRef ref(static_cast<const char*>($1), result);
> >
> >
> >
> > This does cause one test in the testsuite to fail --
> >
> > ======================================================================
> > FAIL: test_snapshot_minidump (TestMiniDumpNew.MiniDumpNewTestCase)
> >    Test that if we load a snapshot minidump file (meaning the process
> > ----------------------------------------------------------------------
> > Traceback (most recent call last):
> >   File
> "/Volumes/newwork/github/stable/lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py",
> line 88, in test_snapshot_minidump
> >     self.assertEqual(stop_description, None)
> > AssertionError: '' != None
> >
> Config=x86_64-/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
> > ----------------------------------------------------------------------
> >
> >
> > which is doing
> >
> >
> >         thread = self.process.GetThreadAtIndex(0)
> >         self.assertEqual(thread.GetStopReason(), lldb.eStopReasonNone)
> >         stop_description = thread.GetStopDescription(256)
> >         self.assertEqual(stop_description, None)
> >
> >
> > SBThread::GetStopDescription doesn't have an SBError object to indicate
> that there is no stop description for eStopReasonNone.  I don't think this
> will be a problem if eStopReasonNone is returning an empty python string
> for the StopDescription.
> >
> >
> > I'm not wedded to my current patch, but we do have to come up with
> something that can return a zero length python string for a method like
> SBProcess::ReadCStringFromMemory.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20171116/19bf60ab/attachment.html>