code review 53590043: gdb: Add partial python3 + go1.2 support to runtime...

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

[shanemhansen, 2014-01-17 04:24:07]

Hello golang-codereviews@googlegroups.com (cc: josharian@gmail.com),

I'd like you to review this change to
https://code.google.com/p/go

[josharian, 2014-01-17 13:58:33]

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

[shanemhansen, 2014-01-17 16:32:02]

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.

[josharian, 2014-01-17 16:37:36]

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?

[josharian, 2014-01-17 16:44:18]

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

[shanemhansen, 2014-01-17 16:57:30]

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.

[shanemhansen, 2014-01-20 04:58:49]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[josharian, 2014-01-21 16:39:59]

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?

[shanemhansen, 2014-01-21 17:20:34]

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/
>

[shanemhansen, 2014-01-21 17:25:33]

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.

[shanemhansen, 2014-01-21 17:25:43]

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?

[josharian, 2014-01-21 17:44:19]

> 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/
> >

[josharian, 2014-01-22 00:36:11]

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.

[shanemhansen, 2014-01-24 05:33:59]

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.

[rsc, 2014-02-24 15:13:04]

LGTM

[rsc, 2014-02-24 15:13:32]

*** 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>