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

Wed Nov 15 21:17:28 PST 2017

Also, it would be nice (but again this isn't immediately affecting me so I
can't really put too much pressure here) if it continued to return None in
case of error.  Any code that would be affected by changing the error
return value from None to "" was very likely broken already and this just
exposed an existing bug.

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

> Yeah, I'm going to check in one based on my little example program.  The
> only tricky bit is
>
>
>
> > >     const char *invalid_memory_string = (char*)0x100; // lower 4k is
> always PAGEZERO & unreadable on darwin
>
>
> I'll need some address that is unmapped for other platforms.  Maybe
> address -1 or something.
>
>
>
>
> > On Nov 15, 2017, at 7:48 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Probably obvious, can you add some tests to verify the new behavior?
> >
> > On Wed, Nov 15, 2017 at 7:47 PM Zachary Turner <zturner at google.com>
> wrote:
> > 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/9fe49ee9/attachment-0001.html>