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 <ztur...@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 <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
