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

Wed Nov 15 19:48:06 PST 2017

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/0295a399/attachment-0001.html>