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

[Jason Molenda via lldb-commits]

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.