Probably obvious, can you add some tests to verify the new behavior? On Wed, Nov 15, 2017 at 7:47 PM Zachary Turner <ztur...@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 <jmole...@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 <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
