patch for pr9067

patch for pr9067

[Chris Moller]

Tom

I just sent $subject to gdb-patches.  Turns out it was simultaneously 
more complex and simpler than we both thought:  You can't just blindly 
inhibit multiple printings of static members.  Sometimes it's 
legitimate, like when they legitimately appear multiple times, possibly 
on different paths through the tree.  The simple side is that after I'd 
finished writing a tree-path-aware fix for the bug, while I was testing 
it, I noticed that someone had already stuck in an attempted fix, using 
obstacks, but had screwed up their use.  Not that I'm a big fan of 
obstacks, but on the least-tinkering-possible principle, I scrapped my 
new stuff and fixed the obstacks.

Chris

Re: patch for pr9067

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> I just sent $subject to gdb-patches.

Thanks.

Chris> Turns out it was simultaneously more complex and simpler than we
Chris> both thought: You can't just blindly inhibit multiple printings
Chris> of static members.  Sometimes it's legitimate, like when they
Chris> legitimately appear multiple times, possibly on different paths
Chris> through the tree.

I didn't see this in the test case you sent.
Could you add it?

Chris> Not that I'm a big fan of obstacks, but on the
Chris> least-tinkering-possible principle, I scrapped my new stuff and
Chris> fixed the obstacks.

Yeah, that's the right thing to do.

Tom

Re: patch for pr9067

[Chris Moller]

On 02/02/10 18:28, Tom Tromey wrote:
>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>              
>
> Chris>  I just sent $subject to gdb-patches.
>
> Thanks.
>
> Chris>  Turns out it was simultaneously more complex and simpler than we
> Chris>  both thought: You can't just blindly inhibit multiple printings
> Chris>  of static members.  Sometimes it's legitimate, like when they
> Chris>  legitimately appear multiple times, possibly on different paths
> Chris>  through the tree.
>
> I didn't see this in the test case you sent.
> Could you add it?
>    

Sure, I can add it, but there are a handful of existing test cases that 
already cover that circumstance--I found them the hard way when my 
simplistic first-pas "fix" kept filtering out legitimate instances.

> Chris>  Not that I'm a big fan of obstacks, but on the
> Chris>  least-tinkering-possible principle, I scrapped my new stuff and
> Chris>  fixed the obstacks.
>
> Yeah, that's the right thing to do.
>
> Tom
>    

Re: patch for pr9067

[Tom Tromey]

Chris> Sure, I can add it, but there are a handful of existing test cases
Chris> that already cover that circumstance--I found them the hard way when
Chris> my simplistic first-pas "fix" kept filtering out legitimate instances.

If it is triggered by the existing test suite, then that is good enough,
thanks.

Tom

PR 11067

[Chris Moller]

On 02/03/10 13:33, Tom Tromey wrote:
> Then how about http://sourceware.org/bugzilla/show_bug.cgi?id=11067 ?
> I just happened to run across this the other day; if it doesn't suit, I
> can dig through bugzilla and find something else.
>    

The enhancement req reads

> When I print a variable, printing the symbolic name for its value is right, but
> when I print the symbolic name, gdb should print its numeric value instead (or
> in addition).
>    

The simple fix is to always print both the symbolic name and the decimal 
value:

> (gdb) p e
> $1 = Val1 (= 56)
> (gdb) p Val1
> $2 = Val1 (= 56)
> (gdb)

That's a one-line change in c-valprint.c.  Trying to differentiate 
between those two cases looks like it would be significantly more difficult.

Would the one-line change do?  Or does the fix need to do exactly what 
the request says?

Chris

> Tom
>    

Re: PR 11067

[Roland McGrath]

> The simple fix is to always print both the symbolic name and the decimal 
> value:
> 
> > (gdb) p e
> > $1 = Val1 (= 56)
> > (gdb) p Val1
> > $2 = Val1 (= 56)
> > (gdb)

I think it should use a format consistent with p/c, e.g.:

(gdb) p 'a'
$1 = 97 'a'
(gdb) p ENUM1
$2 = (enum frobozz) 1 ENUM1
(gdb)



Thanks,
Roland

Re: PR 11067

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> The simple fix is to always print both the symbolic name and the
Chris> decimal value:

>> (gdb) p e
>> $1 = Val1 (= 56)
>> (gdb) p Val1
>> $2 = Val1 (= 56)
>> (gdb)

Chris> That's a one-line change in c-valprint.c.  Trying to differentiate
Chris> between those two cases looks like it would be significantly more
Chris> difficult.

Chris> Would the one-line change do?  Or does the fix need to do exactly
Chris> what the request says?

The one-line fix is fine.

Tom

Re: PR 11067

[Tom Tromey]

>>>>> "Roland" == Roland McGrath <roland@redhat.com> writes:

Roland> I think it should use a format consistent with p/c, e.g.:
Roland> (gdb) p 'a'
Roland> $1 = 97 'a'
Roland> (gdb) p ENUM1
Roland> $2 = (enum frobozz) 1 ENUM1

Works for me.  Chris, please do it this way, thanks.

Tom

Re: PR 11067

[Daniel Jacobowitz]

On Wed, Feb 03, 2010 at 03:48:20PM -0700, Tom Tromey wrote:
> >>>>> "Roland" == Roland McGrath <roland@redhat.com> writes:
> 
> Roland> I think it should use a format consistent with p/c, e.g.:
> Roland> (gdb) p 'a'
> Roland> $1 = 97 'a'
> Roland> (gdb) p ENUM1
> Roland> $2 = (enum frobozz) 1 ENUM1
> 
> Works for me.  Chris, please do it this way, thanks.

FWIW, we've had trouble in the past with the 97 'a' output, because
you get something out that you can't paste back in (not a valid
expression).  I know I did when working on C++ template value
parameters, but that's a special case.  More importantely, I believe
MI frontends had some trouble.

Roland's example is unintuitive to me since "1 ENUM1" just makes me
think "what is that?  it's not C, how are those two things related?".
The closest C language version would be "ENUM1 = 1".

-- 
Daniel Jacobowitz
CodeSourcery

Re: PR 11067

[Chris Moller]

On 02/03/10 17:48, Tom Tromey wrote:
>>>>>> "Roland" == Roland McGrath<roland@redhat.com>  writes:
>>>>>>              
>
> Roland>  I think it should use a format consistent with p/c, e.g.:
> Roland>  (gdb) p 'a'
> Roland>  $1 = 97 'a'
> Roland>  (gdb) p ENUM1
> Roland>  $2 = (enum frobozz) 1 ENUM1
>
> Works for me.  Chris, please do it this way, thanks.
>    

Okay, a whole 5-line patch yields

    (gdb) p e
    $1 = (enum E) 56 Val1
    (gdb) p Val1
    $2 = (enum E) 56 Val1
    (gdb)

(The enum, BTW, is

    num E {
       Val1 = 56,
       Val2
    };

because you can find a 0 any old place.)

Personally, I think

    $2 = (enum E) 56 Val1

is a bit hard to read.  I'd vote for something like

    $2 = (enum E)56 'Val1'

But whatever...


> Tom
>    

Re: PR 11067

[Roland McGrath]

> Roland's example is unintuitive to me since "1 ENUM1" just makes me
> think "what is that?  it's not C, how are those two things related?".
> The closest C language version would be "ENUM1 = 1".

Yeah, I was going for parity and the /c format would not have been my first
choice if we didn't already have it.  I think:

$2 = (enum frobozz) 1 = ENUM1

would be fine too, or maybe reverse the clauses.  "(type) digit" is
probably what's most likely always to be pasteable anywhere, in case
it's into a context where you don't have the same scope for the enum
constant (in C++).


Thanks,
Roland

Re: PR 11067

[Daniel Jacobowitz]

On Wed, Feb 03, 2010 at 03:25:06PM -0800, Roland McGrath wrote:
> Yeah, I was going for parity and the /c format would not have been my first
> choice if we didn't already have it.  I think:
> 
> $2 = (enum frobozz) 1 = ENUM1
> 
> would be fine too, or maybe reverse the clauses.  "(type) digit" is
> probably what's most likely always to be pasteable anywhere, in case
> it's into a context where you don't have the same scope for the enum
> constant (in C++).

Something to consider is what contexts you want this output in.  I
think this would get old really fast:

$2 = { code = CODE_ADD = (enum code) 1, ops = OPS_TWO = (enum ops) 2 }

vs

$2 = { code = CODE_ADD, ops = OPS_TWO }
(gdb) p CODE_ADD
$3 = CODE_ADD = (enum code) 1

Hmm, can't say I like that format any better either, but there's got
to be a good one.  I usually p/d CODE_ADD to find out the value...

My takeaway point here is to consider the overall impact.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PR 11067

[Chris Moller]

On 02/03/10 18:28, Daniel Jacobowitz wrote:
> On Wed, Feb 03, 2010 at 03:25:06PM -0800, Roland McGrath wrote:
>    
>> Yeah, I was going for parity and the /c format would not have been my first
>> choice if we didn't already have it.  I think:
>>
>> $2 = (enum frobozz) 1 = ENUM1
>>
>> would be fine too, or maybe reverse the clauses.  "(type) digit" is
>> probably what's most likely always to be pasteable anywhere, in case
>> it's into a context where you don't have the same scope for the enum
>> constant (in C++).
>>      
>
> Something to consider is what contexts you want this output in.  I
> think this would get old really fast:
>
> $2 = { code = CODE_ADD = (enum code) 1, ops = OPS_TWO = (enum ops) 2 }
>
> vs
>
> $2 = { code = CODE_ADD, ops = OPS_TWO }
> (gdb) p CODE_ADD
> $3 = CODE_ADD = (enum code) 1
>
> Hmm, can't say I like that format any better either, but there's got
> to be a good one.  I usually p/d CODE_ADD to find out the value...
>    

While I was looking for where this stuff is formatted, I noticed it goes 
through python/py-prettyprint.c which, I assume, can format things any 
way you like it.  How about just something generic by default and use a 
pretty-print thingy if that doesn't work?  (Uh, assuming 
py-prettyprint.c does that....)

> My takeaway point here is to consider the overall impact.
>
>    

Re: PR 11067

[Tom Tromey]

Chris> While I was looking for where this stuff is formatted, I noticed it
Chris> goes through python/py-prettyprint.c which, I assume, can format
Chris> things any way you like it.  How about just something generic by
Chris> default and use a pretty-print thingy if that doesn't work?  (Uh,
Chris> assuming py-prettyprint.c does that....)

This only works if you add python code for it.  I think it is preferable
to print enumerators nicely by default, not as a special add-on.

My advice is, read through the follow-ups and make an informed choice.
Daniel makes good points.

Tom

Re: PR 11067

[Chris Moller]

Just FYI, this patch breaks a handful of existing tests:

    gdb.base/setvar.exp
    gdb.cp/classes.exp
    gdb.cp/m-data.exp
    gdb.cp/m-static.exp
    cp/namespace.exp
    gdb.mi/mi-var-display.exp
    gdb.python/py-value.exp

None of them are hard to fix in the expects--I just wanted The World to 
realise that there are going to be noticeable consequences to this 
fairly minor nicety.  It'll take me a bit to fix the t/cs--if The Powers 
That Be would rather this patch not proceed, I'd appreciate it if you'd 
let me know sooner rather than later and spare me all the fix-up work.

Re: PR 11067

[Tom Tromey]

Chris> Just FYI, this patch breaks a handful of existing tests:
Chris>    gdb.base/setvar.exp
Chris>    gdb.cp/classes.exp
Chris>    gdb.cp/m-data.exp
Chris>    gdb.cp/m-static.exp
Chris>    cp/namespace.exp
Chris>    gdb.mi/mi-var-display.exp
Chris>    gdb.python/py-value.exp

Yeah.

Chris> None of them are hard to fix in the expects--I just wanted The World
Chris> to realise that there are going to be noticeable consequences to this
Chris> fairly minor nicety.  It'll take me a bit to fix the t/cs--if The
Chris> Powers That Be would rather this patch not proceed, I'd appreciate it
Chris> if you'd let me know sooner rather than later and spare me all the
Chris> fix-up work.

Usually you just have to do it and see what happens.  Even if we all
agree on a format here, someone else might disagree on gdb-patches, or
they might think of some new reason for it to be different.

I, personally, want some form of this change, because every time I type
"p FOO" and get "$1 = FOO", I think, grrr, gdb could have told me what I
really wanted to know.  If enough people dislike it we can always add
'set print blah blah'.

Tom

Re: PR 11067

[Chris Moller]

On 02/04/10 18:53, Tom Tromey wrote:
> Chris>  Just FYI, this patch breaks a handful of existing tests:
> Chris>     gdb.base/setvar.exp
> Chris>     gdb.cp/classes.exp
> Chris>     gdb.cp/m-data.exp
> Chris>     gdb.cp/m-static.exp
> Chris>     cp/namespace.exp
> Chris>     gdb.mi/mi-var-display.exp
> Chris>     gdb.python/py-value.exp
>
> Yeah.
>
> Chris>  None of them are hard to fix in the expects--I just wanted The World
> Chris>  to realise that there are going to be noticeable consequences to this
> Chris>  fairly minor nicety.  It'll take me a bit to fix the t/cs--if The
> Chris>  Powers That Be would rather this patch not proceed, I'd appreciate it
> Chris>  if you'd let me know sooner rather than later and spare me all the
> Chris>  fix-up work.
>
> Usually you just have to do it and see what happens.  Even if we all
> agree on a format here, someone else might disagree on gdb-patches, or
> they might think of some new reason for it to be different.
>
> I, personally, want some form of this change, because every time I type
> "p FOO" and get "$1 = FOO", I think, grrr, gdb could have told me what I
> really wanted to know.  If enough people dislike it we can always add
> 'set print blah blah'.
>    

Based on what people were saying yesterday, I thought I'd go with

    $1 = Val1=(enum E)56

It looks like a legitimate C expression.  Lacking spaces, it's cohesive, 
and it's easily understood to present the same information in multiple 
formats.

> Tom
>    

Re: PR 11067

[Roland McGrath]

> Based on what people were saying yesterday, I thought I'd go with
> 
>     $1 = Val1=(enum E)56
> 
> It looks like a legitimate C expression.  Lacking spaces, it's cohesive, 
> and it's easily understood to present the same information in multiple 
> formats.

Please don't skimp on the spaces.

Re: PR 11067

[Chris Moller]

No matter what format I use, someone won't like it, so how about this:

I'll add a new "set" option, "set enum-fmt" or something like that that 
can take a printf-style format string.  Default format if the string 
isn't set is current behaviour, otherwise the string is parsed, looking 
for %-directives, %v for the numeric value of the enum, %s for the 
symbolic value, and %e for the enum decl.  My spaceless format, e.g., 
would be set with a format of

    set enum-fmt "%s=(enum %e)%d"

If Roland wants spaces, he can use

    set enum-fmt "%s = (enum %e) %d"

or whatever.

I've never been a big fan of breaking historical behaviour, and this 
avoids that.  It's a bigger patch, but I think it's a better one.

cm


On 02/05/10 15:18, Roland McGrath wrote:
>> Based on what people were saying yesterday, I thought I'd go with
>>
>>      $1 = Val1=(enum E)56
>>
>> It looks like a legitimate C expression.  Lacking spaces, it's cohesive,
>> and it's easily understood to present the same information in multiple
>> formats.
>>      
>
> Please don't skimp on the spaces.
>