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 <ztur...@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 <jmole...@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. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
