code review 100060043: cmd/objdump: actually accept hex address without "0x" p...

cmd/objdump: actually accept hex address without "0x" prefix.
Fixes issue 7936.

[minux1, 2014-05-05 02:49:42]

Hello golang-codereviews@googlegroups.com,

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

[brainman, 2014-05-05 02:52:34]

LGTM

[brainman, 2014-05-05 05:20:21]

This breaks "go tool pprof --list=..." command:

objdump: invalid start PC: strconv.ParseUint: parsing "0x080f6d20": invalid
syntax

Alex

[minux1, 2014-05-05 05:33:13]

On Mon, May 5, 2014 at 1:20 AM, <alex.brainman@gmail.com> wrote:

> This breaks "go tool pprof --list=..." command:
>
> objdump: invalid start PC: strconv.ParseUint: parsing "0x080f6d20":
> invalid syntax
>
so the problem is actually in the docs. Update. PTAL.

[brainman, 2014-05-05 05:39:18]

On 2014/05/05 05:33:13, minux wrote:
> >
> so the problem is actually in the docs. Update. PTAL.

Your main.go change was OK - it makes objdump read hex addresses, otherwise
objdump assumes addresses are decimal.

As to your doc change, I'm not sure what rsc wants. Lets wait for him.

Alex

[minux1, 2014-05-05 05:40:52]

On Mon, May 5, 2014 at 1:39 AM, <alex.brainman@gmail.com> wrote:

> On 2014/05/05 05:33:13, minux wrote:
>
>> >
>> so the problem is actually in the docs. Update. PTAL.
>>
>
> Your main.go change was OK - it makes objdump read hex addresses,
> otherwise objdump assumes addresses are decimal.
>
if base is 0, then ParseUint will determine the base from the string, 0x
for hexadecimal,
0 for octal, etc.

>
> As to your doc change, I'm not sure what rsc wants. Lets wait for him.
>

[brainman, 2014-05-05 05:44:07]

On 2014/05/05 05:40:52, minux wrote:
> if base is 0, then ParseUint will determine the base from the string, 0x
> for hexadecimal,
> 0 for octal, etc.
> 

I didn't know that. But I still think objdump should do like addr2line does. I
don't see why the two should differ.

Alex

[bradfitz, 2014-05-05 13:33:49]

LGTM

Verified misc/pprof calls objdump like:

  my $objdump = $obj_tool_map{"objdump"};
  my $cmd = sprintf("$objdump -C -d -l --no-show-raw-insn " .
                    "--start-address=0x$start_addr " .
                    "--stop-address=0x$end_addr $prog");

So fixing the docs to match reality seems fine.

Perhaps later we should only accept 0x-prefixed arguments, so "1234" isn't
interpreted as decimal (per strconv ParseInt with base 0), but not now.

[brainman, 2014-05-05 23:32:00]

On 2014/05/05 13:33:49, bradfitz wrote:
> LGTM

The CL description is wrong then.

> 
> Verified misc/pprof ...
> So fixing the docs to match reality seems fine.

I disagree. I think nm, addr2line and objdump should work the same way (re 0x
prefix) - output of nm should be suitable for both addr2line and objdump. It is
not now. Why do we have to remember which tools needs 0x and wich don't? What is
the reason?

> Perhaps later we should only accept 0x-prefixed arguments, so "1234" isn't
> interpreted as decimal (per strconv ParseInt with base 0), but not now.

Please, create an issue then. But why not change now?

Alex

[bradfitz, 2014-05-06 17:47:15]

I have no opinion on how it should be. It seems inconsistent and
misdocumented now, but I don't know the desired behavior.  Russ?





On Mon, May 5, 2014 at 4:32 PM, <alex.brainman@gmail.com> wrote:

> On 2014/05/05 13:33:49, bradfitz wrote:
>
>> LGTM
>>
>
> The CL description is wrong then.
>
>
>  Verified misc/pprof ...
>>
>> So fixing the docs to match reality seems fine.
>>
>
> I disagree. I think nm, addr2line and objdump should work the same way
> (re 0x prefix) - output of nm should be suitable for both addr2line and
> objdump. It is not now. Why do we have to remember which tools needs 0x
> and wich don't? What is the reason?
>
>
>  Perhaps later we should only accept 0x-prefixed arguments, so "1234"
>>
> isn't
>
>> interpreted as decimal (per strconv ParseInt with base 0), but not
>>
> now.
>
> Please, create an issue then. But why not change now?
>
> Alex
>
> https://codereview.appspot.com/100060043/
>

[iant, 2014-05-06 18:23:40]

All that really matters here is that the program works with pprof on Macs. 
Otherwise I don't think we care.

In Go 1.2 the objdump program, written in C, simply called strtoull(argv[1], 0,
16).  That means that it always read hex numbers, and ignored any leading 0x.  I
don't think we have any direct equivalent to that in the strconv package.

[bradfitz, 2014-05-06 18:34:30]

So let's just:
strconv.ParseInt(strings.TrimPrefix(v, "0x"), 16, 64)

And document it as "optional 0x prefix"?



On Tue, May 6, 2014 at 11:23 AM, <iant@golang.org> wrote:

> All that really matters here is that the program works with pprof on
> Macs.  Otherwise I don't think we care.
>
> In Go 1.2 the objdump program, written in C, simply called
> strtoull(argv[1], 0, 16).  That means that it always read hex numbers,
> and ignored any leading 0x.  I don't think we have any direct equivalent
> to that in the strconv package.
>
> https://codereview.appspot.com/100060043/
>

[brainman, 2014-05-06 23:21:25]

On 2014/05/06 18:34:30, bradfitz wrote:
> So let's just:
> strconv.ParseInt(strings.TrimPrefix(v, "0x"), 16, 64)
> 
> And document it as "optional 0x prefix"?
> 

SGTM (for both addr2line and objdump). If someone wants to change the code and
documentation, I will provide tests later. Still it would be nice to hear from
Russ - perhaps he had some other ideas.

Alex

[minux1, 2014-05-07 21:48:59]

PTAL.

[bradfitz, 2014-05-07 21:57:00]

LGTM

[iant, 2014-05-07 22:04:27]

LGTM

[brainman, 2014-05-07 22:30:50]

LGTM assuming we will change addr2line to do the same.

Alex

[minux1, 2014-05-08 05:26:00]

*** Submitted as https://code.google.com/p/go/source/detail?r=e3d533e55750 ***

cmd/objdump: actually accept hex address without "0x" prefix.
Fixes issue 7936.

LGTM=alex.brainman, bradfitz, iant
R=golang-codereviews, alex.brainman, bradfitz, iant
CC=golang-codereviews
https://codereview.appspot.com/100060043

[minux1, 2014-05-08 05:30:13]

On 2014/05/07 22:30:50, brainman wrote:
> LGTM assuming we will change addr2line to do the same.
are you talking about something like https://codereview.appspot.com/91250043?

[brainman, 2014-05-08 06:07:44]

On 2014/05/08 05:30:13, minux wrote:
> On 2014/05/07 22:30:50, brainman wrote:
> > LGTM assuming we will change addr2line to do the same.
> are you talking about something like https://codereview.appspot.com/91250043

Yes, please. But I cannot review it.

Alex

[minux1, 2014-05-08 06:24:54]

On Thu, May 8, 2014 at 2:07 AM, <alex.brainman@gmail.com> wrote:

> On 2014/05/08 05:30:13, minux wrote:
>
>> On 2014/05/07 22:30:50, brainman wrote:
>> > LGTM assuming we will change addr2line to do the same.
>> are you talking about something like
>>
> https://codereview.appspot.com/91250043
>
> Yes, please. But I cannot review it.
>
ok, but i don't understand the necessity of that change.
could you explain? thanks.

[brainman, 2014-05-08 06:35:22]

On 2014/05/08 06:24:54, minux wrote:
> >
> ok, but i don't understand the necessity of that change.
> could you explain? thanks.

For consistency. I think both addr2line and objdump should accept same address
formats. As it stands now objdump accepts both abcd and 0xabcd addresses, while
addr2line will reject 0xabcd.

Alex