|
|
Created:
10 years, 3 months ago by shanemhansen Modified:
10 years, 2 months ago Reviewers:
rsc CC:
golang-codereviews, josharian, rsc Visibility:
Public. |
Descriptiongdb: Add partial python3 + go1.2 support to runtime-gdb.py
Update issue 6963 Fixes pretty printing maps and updates
functions for interacting with $len(). goroutine $n bt
remains not working. Tested on gdb using python 2 and 3.
Fixes issue 7052
Update issue 6963
Fixes issue 6698
Patch Set 1 #Patch Set 2 : diff -r 5069dab9a825 https://code.google.com/p/go #Patch Set 3 : diff -r 5069dab9a825 https://code.google.com/p/go #
Total comments: 12
Patch Set 4 : diff -r 5069dab9a825 https://code.google.com/p/go #Patch Set 5 : diff -r 5069dab9a825 https://code.google.com/p/go #
Total comments: 16
Patch Set 6 : diff -r 87dea3f5ebe7 https://code.google.com/p/go #MessagesTotal messages: 16
Hello golang-codereviews@googlegroups.com (cc: josharian@gmail.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Cool! Before starting to review, let's make sure that continued gdb support is in the cards. I just sent an email to go-dev asking about it. Also, if you've not signed the CLA, now might be a good time to go do that: http://golang.org/doc/contribute.html -josh
Sign in to reply to this message.
I can't review my own code, but I wanted to make some comments on python2/python3 differences since the audience might not be that familiar with them (frankly I'm not familiar with the differences either). https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... File src/pkg/runtime/runtime-gdb.py (right): https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:20: sys.stderr.write("Loading Go Runtime support.") print was turned from a statement to a function in python3. http://docs.python.org/dev/howto/pyporting.html#from-future-import-print-func... One way to avoid issues is to use the sys.stderr.write function call for both python 2 and 3. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:23: long = int python2 and python3 both have a builtin int type which handles integers of arbitrary length. long has been redundant since sometime during python2. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:24: xrange = range In python2 xrange returned a lazy iterator whereas range returned a list. They are largely interchangeable in for loops. In fact python3's range is python2's xrange. (python3's range is a lazy iterator). https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:67: for idx in range(int(str(self.val["len"]))): Total hack here. self.val["len"] couldn't be used as an int in python3. Not sure why. However it's string representation could be parsed as an int. It works when tested. shrug. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:78: pattern = re.compile(r'^map\[.*\].*$') This seems to be a api/compiler change? go1.2 uses map[string] or something for the type, not struct hash. Maybe it's gdb that got smarter I don't know, but this is what the map type currently looks like. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:96: for bucket in xrange(2 ** int(B)): Random cast to make things work. Honestly don't know why it was needed. Just tested that this worked for pretty printing maps in go1.2 with python2 and python3 versions of gdb. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:361: print(s, ptr['goid'], "%8s" % sts[long((ptr['status']))], blk.function) print is now a function, the python2 parser allows parentheses around the argument to the print statement. This is actually a bug I need to fix. This version prints a tuple rather than separating the values by space. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:369: return [ptr['sched'][x].cast(vp) for x in ('pc', 'sp')] The syntax changed such that commas alone are not enough to make 'pc', 'sp' a tuple to iterate over. ('pc', 'sp') is a tuple in both python2 and python3. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:393: print("No such goroutine: ", goid) TODO: use actual print function, accidentally printing a tuple here. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:419: except Exception as e: Minor syntax change that's backwards compatible. Pretty self explanatory.
Sign in to reply to this message.
Thanks for starting this. Your work looks farther along than mine. I'll abandon my CL and help you with this, if you don't mind pulling in these missing fixes from my CL (https://codereview.appspot.com/47230043): - You'll need `from __future__ import print_function` for Python 2 support for printing (as a non-tuple). This means no Python 2.5 support, but so does the `except Exception as e` syntax, and 2.5 is *really* old nowadays. - Python 3 doesn't support the % operator for string formatting. Use string.format instead. Any thoughts on how to write tests for any of this? I see lots of what look like specific bug fixes here, and it'd be very nice to start to have some regression tests. Maybe http://tromey.com/blog/?p=548 could provide a start? I want to take another, closer look, but that likely won't happen until next week; sorry. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... File src/pkg/runtime/runtime-gdb.py (right): https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:20: sys.stderr.write("Loading Go Runtime support.") Nitpick: I find `print("Loading Go Runtime support.", file=sys.stderr)` a bit nicer. https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:67: for idx in range(int(str(self.val["len"]))): What does the discursion through str buy here?
Sign in to reply to this message.
Also, with regard to the description: - I don't know what the right prefix is for this, but "gdb" seems more apropos than "runtime" (?). - Looks like you have leading whitespace on the first line. - Change `Refs issue 6963.` to `Update issue 6963` (no trailing period), on its own line, at the end. - Add `Fixes issue 7052` on its own line, at the end. Thanks again for tackling this! -josh
Sign in to reply to this message.
On 2014/01/17 16:37:36, josharian wrote: > Thanks for starting this. Your work looks farther along than mine. I'll abandon > my CL and help you with this, if you don't mind pulling in these missing fixes > from my CL (https://codereview.appspot.com/47230043%29: > > - You'll need `from __future__ import print_function` for Python 2 support for > printing (as a non-tuple). This means no Python 2.5 support, but so does the > `except Exception as e` syntax, and 2.5 is *really* old nowadays. > - Python 3 doesn't support the % operator for string formatting. Use > string.format instead. > > Any thoughts on how to write tests for any of this? I see lots of what look like > specific bug fixes here, and it'd be very nice to start to have some regression > tests. Maybe http://tromey.com/blog/?p=548 could provide a start? > > I want to take another, closer look, but that likely won't happen until next > week; sorry. > > https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... > File src/pkg/runtime/runtime-gdb.py (right): > > https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... > src/pkg/runtime/runtime-gdb.py:20: sys.stderr.write("Loading Go Runtime > support.") > Nitpick: I find `print("Loading Go Runtime support.", file=sys.stderr)` a bit > nicer. > > https://codereview.appspot.com/53590043/diff/40001/src/pkg/runtime/runtime-gd... > src/pkg/runtime/runtime-gdb.py:67: for idx in range(int(str(self.val["len"]))): > What does the discursion through str buy here? Made changes. Apparently the discursion through str did nothing. int() however is needed.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks for continuing to hack on this, Shane, and for your patience. Sorry for my slow replies; quite busy at the moment. The more I look at the code and at your changes, the clearer it is that there's a fair amount of cleanup to be done: * I'd like the code to get a clean bill of health from `pep8 --ignore=W191,E501 src/pkg/runtime/runtime-gdb.py` (not hard). * There's the goroutine command to fix. * I'd really like to get to a point that we can build automated tests (I'm starting on that now). It seems like we should work on stabilizing and submitting this set of changes first, though, and then do further cleanup/work in follow-up CLs. Sound good? Two description things: * "Update issue 6963" should be at the end of the description, on its own line. * Would you mind adding "Fixes issue 6698" as well? Just found that issue. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... File src/pkg/runtime/runtime-gdb.py (right): https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:110: key = b['keys'][i] If we're going to rename k to key, let's rename v to val as well. I'd just as soon leave them k and v, though. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:116: yield '{0}'.format(cnt), key Hmm, a simpler option than .format, both here and several other places, is just str(cnt). https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:151: Nitpick, but blank lines shouldn't contain whitespace (PEP-8). https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:157: except Exception as e: Here and several other places, the "as e" is unnecessary if e isn't actually used. Thanks for adding Exception, though; much better. Even better yet would be to catch the precise exception types that are expected to occur. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:293: gdb.Function.__init__(self, "len") Nice. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x in ('pc', 'sp')) Are you sure that it is safe to just use int here instead of doing the gdb type lookup + cast?
Sign in to reply to this message.
I don't have a clear plan for automated tests yet. I'd welcome some ideas. I'm developing with pylint, you may have noticed that's what several of the changes were, but I though that figuring out exactly what exception types were being thrown might change the behavior of the code. As far as making goroutine $n bt work, I'm wondering if that's a larger task. The regular gdb stuff won't work because it's goroutines w/ segmented stacks right? I know that part of the reason it's broken is that the stack pointer from find_goroutines is 0, which makes no sense so I don't know if find_goroutines is currently looking at the right structs. I could definitely use some pointers (pardon the pun) to the goroutine internals/existing non-plugin gdb support. On Tue, Jan 21, 2014 at 9:40 AM, <josharian@gmail.com> wrote: > Thanks for continuing to hack on this, Shane, and for your patience. > Sorry for my slow replies; quite busy at the moment. > > The more I look at the code and at your changes, the clearer it is that > there's a fair amount of cleanup to be done: > > * I'd like the code to get a clean bill of health from `pep8 > --ignore=W191,E501 src/pkg/runtime/runtime-gdb.py` (not hard). > * There's the goroutine command to fix. > * I'd really like to get to a point that we can build automated tests > (I'm starting on that now). > > It seems like we should work on stabilizing and submitting this set of > changes first, though, and then do further cleanup/work in follow-up > CLs. Sound good? > > Two description things: > > * "Update issue 6963" should be at the end of the description, on its > own line. > * Would you mind adding "Fixes issue 6698" as well? Just found that > issue. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py > File src/pkg/runtime/runtime-gdb.py (right): > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py#newcode110 > src/pkg/runtime/runtime-gdb.py:110: key = b['keys'][i] > If we're going to rename k to key, let's rename v to val as well. I'd > just as soon leave them k and v, though. > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py#newcode116 > src/pkg/runtime/runtime-gdb.py:116: yield '{0}'.format(cnt), key > Hmm, a simpler option than .format, both here and several other places, > is just str(cnt). > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py#newcode151 > src/pkg/runtime/runtime-gdb.py:151: > Nitpick, but blank lines shouldn't contain whitespace (PEP-8). > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py#newcode157 > src/pkg/runtime/runtime-gdb.py:157: except Exception as e: > Here and several other places, the "as e" is unnecessary if e isn't > actually used. Thanks for adding Exception, though; much better. Even > better yet would be to catch the precise exception types that are > expected to occur. > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py#newcode293 > src/pkg/runtime/runtime-gdb.py:293: gdb.Function.__init__(self, "len") > Nice. > > https://codereview.appspot.com/53590043/diff/80001/src/ > pkg/runtime/runtime-gdb.py#newcode368 > src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x > in ('pc', 'sp')) > Are you sure that it is safe to just use int here instead of doing the > gdb type lookup + cast? > > https://codereview.appspot.com/53590043/ >
Sign in to reply to this message.
On 2014/01/21 17:20:34, shanemhansen wrote: > I don't have a clear plan for automated tests yet. I'd welcome some ideas. > I'm developing with pylint, you may have noticed that's what several of the > changes were, but I though that figuring out exactly what exception types > were being thrown might change the behavior of the code. > > As far as making goroutine $n bt work, I'm wondering if that's a larger > task. The regular gdb stuff won't work because it's goroutines w/ segmented > stacks right? I know that part of the reason it's broken is that the stack > pointer from find_goroutines is 0, which makes no sense so I don't know if > find_goroutines is currently looking at the right structs. I could > definitely use some pointers (pardon the pun) to the goroutine > internals/existing non-plugin gdb support. > > > > > > > On Tue, Jan 21, 2014 at 9:40 AM, <mailto:josharian@gmail.com> wrote: > > > Thanks for continuing to hack on this, Shane, and for your patience. > > Sorry for my slow replies; quite busy at the moment. > > > > The more I look at the code and at your changes, the clearer it is that > > there's a fair amount of cleanup to be done: > > > > * I'd like the code to get a clean bill of health from `pep8 > > --ignore=W191,E501 src/pkg/runtime/runtime-gdb.py` (not hard). > > * There's the goroutine command to fix. > > * I'd really like to get to a point that we can build automated tests > > (I'm starting on that now). > > > > It seems like we should work on stabilizing and submitting this set of > > changes first, though, and then do further cleanup/work in follow-up > > CLs. Sound good? > > > > Two description things: > > > > * "Update issue 6963" should be at the end of the description, on its > > own line. > > * Would you mind adding "Fixes issue 6698" as well? Just found that > > issue. > > > > > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py > > File src/pkg/runtime/runtime-gdb.py (right): > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode110 > > src/pkg/runtime/runtime-gdb.py:110: key = b['keys'][i] > > If we're going to rename k to key, let's rename v to val as well. I'd > > just as soon leave them k and v, though. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode116 > > src/pkg/runtime/runtime-gdb.py:116: yield '{0}'.format(cnt), key > > Hmm, a simpler option than .format, both here and several other places, > > is just str(cnt). > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode151 > > src/pkg/runtime/runtime-gdb.py:151: > > Nitpick, but blank lines shouldn't contain whitespace (PEP-8). > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode157 > > src/pkg/runtime/runtime-gdb.py:157: except Exception as e: > > Here and several other places, the "as e" is unnecessary if e isn't > > actually used. Thanks for adding Exception, though; much better. Even > > better yet would be to catch the precise exception types that are > > expected to occur. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode293 > > src/pkg/runtime/runtime-gdb.py:293: gdb.Function.__init__(self, "len") > > Nice. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode368 > > src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x > > in ('pc', 'sp')) > > Are you sure that it is safe to just use int here instead of doing the > > gdb type lookup + cast? > > > > https://codereview.appspot.com/53590043/ > > Great feedback, I'll work on it.
Sign in to reply to this message.
https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... File src/pkg/runtime/runtime-gdb.py (right): https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:110: key = b['keys'][i] Sounds good. This was a pylint change because key shadows a variable. On 2014/01/21 16:40:00, josharian wrote: > If we're going to rename k to key, let's rename v to val as well. I'd just as > soon leave them k and v, though. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:116: yield '{0}'.format(cnt), key On 2014/01/21 16:40:00, josharian wrote: > Hmm, a simpler option than .format, both here and several other places, is just > str(cnt). Done. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:151: On 2014/01/21 16:40:00, josharian wrote: > Nitpick, but blank lines shouldn't contain whitespace (PEP-8). Done. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:157: except Exception as e: I'll precise exception types. I'll work on that. It was linting/python3 driven. One problem with except: vs except Exception: is that there are actually exceptions that can be thrown that aren't base classes of Exception and shouldn't be caught. It's weird I know, but twisted used to do it which is why I avoid except: On 2014/01/21 16:40:00, josharian wrote: > Here and several other places, the "as e" is unnecessary if e isn't actually > used. Thanks for adding Exception, though; much better. Even better yet would be > to catch the precise exception types that are expected to occur. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:293: gdb.Function.__init__(self, "len") lol, pylint On 2014/01/21 16:40:00, josharian wrote: > Nice. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x in ('pc', 'sp')) I can't be 100% sure. I can tell you this. ptr['sched']['pc'] and sp are uintptr which is either uint64 or uint32 depending on the platform. So why cast is to void *? This function is only used in one place, and I personally thought it was easier to make pc and sp integers, but they don't have to be. On 2014/01/21 16:40:00, josharian wrote: > Are you sure that it is safe to just use int here instead of doing the gdb type > lookup + cast?
Sign in to reply to this message.
> I don't have a clear plan for automated tests yet. I'd welcome some ideas. Let's do design discussion in an issue: https://code.google.com/p/go/issues/detail?id=7161 I'll add my (still nascent) thoughts there. > I'm developing with pylint, you may have noticed that's what several of the > changes were, but I though that figuring out exactly what exception types > were being thrown might change the behavior of the code. Fair enough. This will be safer to do once we have some tests. > As far as making goroutine $n bt work, I'm wondering if that's a larger > task. The regular gdb stuff won't work because it's goroutines w/ segmented > stacks right? I know that part of the reason it's broken is that the stack > pointer from find_goroutines is 0, which makes no sense so I don't know if > find_goroutines is currently looking at the right structs. I could > definitely use some pointers (pardon the pun) to the goroutine > internals/existing non-plugin gdb support. Yep. Want to file an issue for it, and add these comments/questions there? -josh > On Tue, Jan 21, 2014 at 9:40 AM, <mailto:josharian@gmail.com> wrote: > > > Thanks for continuing to hack on this, Shane, and for your patience. > > Sorry for my slow replies; quite busy at the moment. > > > > The more I look at the code and at your changes, the clearer it is that > > there's a fair amount of cleanup to be done: > > > > * I'd like the code to get a clean bill of health from `pep8 > > --ignore=W191,E501 src/pkg/runtime/runtime-gdb.py` (not hard). > > * There's the goroutine command to fix. > > * I'd really like to get to a point that we can build automated tests > > (I'm starting on that now). > > > > It seems like we should work on stabilizing and submitting this set of > > changes first, though, and then do further cleanup/work in follow-up > > CLs. Sound good? > > > > Two description things: > > > > * "Update issue 6963" should be at the end of the description, on its > > own line. > > * Would you mind adding "Fixes issue 6698" as well? Just found that > > issue. > > > > > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py > > File src/pkg/runtime/runtime-gdb.py (right): > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode110 > > src/pkg/runtime/runtime-gdb.py:110: key = b['keys'][i] > > If we're going to rename k to key, let's rename v to val as well. I'd > > just as soon leave them k and v, though. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode116 > > src/pkg/runtime/runtime-gdb.py:116: yield '{0}'.format(cnt), key > > Hmm, a simpler option than .format, both here and several other places, > > is just str(cnt). > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode151 > > src/pkg/runtime/runtime-gdb.py:151: > > Nitpick, but blank lines shouldn't contain whitespace (PEP-8). > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode157 > > src/pkg/runtime/runtime-gdb.py:157: except Exception as e: > > Here and several other places, the "as e" is unnecessary if e isn't > > actually used. Thanks for adding Exception, though; much better. Even > > better yet would be to catch the precise exception types that are > > expected to occur. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode293 > > src/pkg/runtime/runtime-gdb.py:293: gdb.Function.__init__(self, "len") > > Nice. > > > > https://codereview.appspot.com/53590043/diff/80001/src/ > > pkg/runtime/runtime-gdb.py#newcode368 > > src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x > > in ('pc', 'sp')) > > Are you sure that it is safe to just use int here instead of doing the > > gdb type lookup + cast? > > > > https://codereview.appspot.com/53590043/ > >
Sign in to reply to this message.
Looks like your latest changes haven't been uploaded? https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... File src/pkg/runtime/runtime-gdb.py (right): https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:157: except Exception as e: > One problem with except: vs except Exception: is that there are actually > exceptions that can be thrown that aren't base classes of Exception and > shouldn't be caught. It's weird I know, but twisted used to do it which is why I > avoid except: Yep. Bare except clauses are very rarely correct. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x in ('pc', 'sp')) > I can't be 100% sure. I can tell you this. > ptr['sched']['pc'] and sp are uintptr which is either uint64 or uint32 depending > on the platform. So why cast is to void *? void* should always be the size of a uintptr, which is (I think) why it was used. The only scenario I can think of int-vs-cast mattering is if gdb's python was compiled with a different int size than the executable being debugged--unlikely but still possible. I think it'd be better to keep it as it was.
Sign in to reply to this message.
https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... File src/pkg/runtime/runtime-gdb.py (right): https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:110: key = b['keys'][i] On 2014/01/21 17:25:44, shanemhansen wrote: > Sounds good. This was a pylint change because key shadows a variable. > On 2014/01/21 16:40:00, josharian wrote: > > If we're going to rename k to key, let's rename v to val as well. I'd just as > > soon leave them k and v, though. > I'm leaving them as k,v and changing the shadowed variable. https://codereview.appspot.com/53590043/diff/80001/src/pkg/runtime/runtime-gd... src/pkg/runtime/runtime-gdb.py:368: return (int(ptr['sched'][x]) for x in ('pc', 'sp')) On 2014/01/22 00:36:12, josharian wrote: > > I can't be 100% sure. I can tell you this. > > ptr['sched']['pc'] and sp are uintptr which is either uint64 or uint32 > depending > > on the platform. So why cast is to void *? > > void* should always be the size of a uintptr, which is (I think) why it was > used. > > The only scenario I can think of int-vs-cast mattering is if gdb's python was > compiled with a different int size than the executable being debugged--unlikely > but still possible. I think it'd be better to keep it as it was. I remember the problem now. When pc and sp are gdb.Value's which represent void* python2 has trouble casting them to an int (by invoking the gdb.Value.__int__() method). So I can either return void* can take a useless trip through str() or just not cast the values and send them straight to python arbitrary size ints. Of course it can't be that simple. The truth is that on newer versions of gdb the str() representation of the pc is something like '0xdeadbeef <runtime.gosched+39>'. Luckily on newer versions of gdb int(pc) works. On older versions of gdb int(pc) fails, but str(pc) is '0xdeadbeef', so we can just calculate the value with int(str(pc), 16). Despite the extra hoops, I think the approach I'm taking which is to try the int cast and fall back to a trip through str() is more pythonic.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=2e349400827a *** gdb: Add partial python3 + go1.2 support to runtime-gdb.py Update issue 6963 Fixes pretty printing maps and updates functions for interacting with $len(). goroutine $n bt remains not working. Tested on gdb using python 2 and 3. Fixes issue 7052 Update issue 6963 Fixes issue 6698 LGTM=rsc R=golang-codereviews, josharian, rsc CC=golang-codereviews https://codereview.appspot.com/53590043 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|