[RFC] delete gdb.cp/ambiguous.exp ?

[RFC] delete gdb.cp/ambiguous.exp ?

[Doug Evans]

Along the lines of dead code removal ...

What should we do with this testcase?

The messages it is looking for from gdb do not exist,
but the whole thing is not run for gcc anyway.

I ask because if we choose to not delete the testcase
then I will need to add a similar escape for clang.

# tests relating to ambiguous class members
# Written by Satish Pai <pai@apollo.hp.com> 1997-07-28

...

if { [test_compiler_info gcc-*] } then { continue }

...

# print out various class objects' members.  The values aren't
# important, just check that the warning is emitted at the
# right times. 

# X is derived from A1 and A2; both A1 and A2 have a member 'x'
send_gdb "print x.x\n"
gdb_expect {
   -re "warning: x ambiguous; using X::A2::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
       pass "print x.x"
   }
   -re "warning: x ambiguous; using X::A1::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
       pass "print x.x"
   }
   -re ".*$gdb_prompt $" { fail "print x.x" }
   timeout { fail "(timeout) print x.x" }
}

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Joel Brobecker]

> What should we do with this testcase?
> 
> The messages it is looking for from gdb do not exist,
> but the whole thing is not run for gcc anyway.
> 
> I ask because if we choose to not delete the testcase
> then I will need to add a similar escape for clang.

I took a brief look to see if the testcase could be somewhat
rescued, but it didn't feel like it was worth our trouble.
I vote for deleting it.

-- 
Joel

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Pedro Alves]

On 08/19/2014 12:23 AM, Doug Evans wrote:
> Along the lines of dead code removal ...
> 
> What should we do with this testcase?
> 
> The messages it is looking for from gdb do not exist,

Hmm.  These are:

# X is derived from A1 and A2; both A1 and A2 have a member 'x'
send_gdb "print x.x\n"
gdb_expect {
   -re "warning: x ambiguous; using X::A2::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
       pass "print x.x"
   }
   -re "warning: x ambiguous; using X::A1::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
       pass "print x.x"
   }
   -re ".*$gdb_prompt $" { fail "print x.x" }
   timeout { fail "(timeout) print x.x" }
}

Actually enabling the test (removing the skip, and adding
nowarnings), we see that indeed GDB outputs no warning:

 print x.x
 $1 = 994109222
 (gdb) FAIL: gdb.cp/ambiguous.exp: print x.x

but, GDB also silently picks an 'x'.  If we try compiling 'x.x', we get:

 ambiguous.cc: In function ‘int main()’:
 ambiguous.cc:103:67: error: request for member ‘x’ is ambiguous
    i += k.i + m.w + a1.x + a2.x + a3.x + x.z + l.z + n.r + j.j + x.x;
                                                                    ^
 ambiguous.cc:15:7: note: candidates are: int A2::x
    int x;
        ^
 ambiguous.cc:9:7: note:                 int A1::x

And gdb.cp/inherit.exp has:

    # Print all members of g_D.
    #
    # g_D.A::a and g_D.A::x are ambiguous member accesses.
    gdb_test "print g_D.A::a" "base class 'A' is ambiguous in type 'D'"

So shouldn't GDB error out in the 'x.x' case too ?  Or at
least warn, like the test suggests (I bet this came in with
the big HP merge years ago).

In this case below, GDB does error out, though the test
expects a warning:

 print (A1)j
 base class 'A1' is ambiguous in type 'J'
 (gdb) FAIL: gdb.cp/ambiguous.exp: print (A1)j

Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.cp/ambiguous.exp ...
FAIL: gdb.cp/ambiguous.exp: print x.x
FAIL: gdb.cp/ambiguous.exp: print n.x
FAIL: gdb.cp/ambiguous.exp: print j.x
FAIL: gdb.cp/ambiguous.exp: print jva1.x
FAIL: gdb.cp/ambiguous.exp: print jva2.x
FAIL: gdb.cp/ambiguous.exp: print (A1)j
FAIL: gdb.cp/ambiguous.exp: print (A1)jva1

-- 
Thanks,
Pedro Alves

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Doug Evans]

On Tue, Aug 19, 2014 at 9:49 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/19/2014 12:23 AM, Doug Evans wrote:
>> Along the lines of dead code removal ...
>>
>> What should we do with this testcase?
>>
>> The messages it is looking for from gdb do not exist,
>
> Hmm.  These are:
>
> # X is derived from A1 and A2; both A1 and A2 have a member 'x'
> send_gdb "print x.x\n"
> gdb_expect {
>    -re "warning: x ambiguous; using X::A2::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
>        pass "print x.x"
>    }
>    -re "warning: x ambiguous; using X::A1::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
>        pass "print x.x"
>    }
>    -re ".*$gdb_prompt $" { fail "print x.x" }
>    timeout { fail "(timeout) print x.x" }
> }
>
> Actually enabling the test (removing the skip, and adding
> nowarnings), we see that indeed GDB outputs no warning:

But given the early exit the test itself is never run.
And it's been this way since at least 2003 (commit 4d9eda44f), and
longer (that commit just changed the style of the gcc test)!

I'm all for filing a bug and recording the test in the bug report.
I'm even ok with keeping the test as is.
The high order bit for me is exploring what the community wishes be
done with this kind of "dead code".

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Pedro Alves]

On 08/19/2014 06:24 PM, Doug Evans wrote:

>> Actually enabling the test (removing the skip, and adding
>> nowarnings), we see that indeed GDB outputs no warning:
> 
> But given the early exit the test itself is never run.

As I said, I removed that.  ;-)

> And it's been this way since at least 2003 (commit 4d9eda44f), and
> longer (that commit just changed the style of the gcc test)!

Yeah, this probably came in in the big HP merge, much
earlier than that.  There was once a gdb.hp/ambiguous.exp, even, and
this probably got copied from that.
There was once a big
everything-goes-we-dont-have-time-to-clean-things-up-before-accepting HP
import/contribution, and bits may be skipped on GCC just because the
HP folks at the time didn't want to deal with it in their local fork.

> I'm all for filing a bug and recording the test in the bug report.
> I'm even ok with keeping the test as is.
> The high order bit for me is exploring what the community wishes be
> done with this kind of "dead code".

You're asking for a generic opinion, of the abstract "community",
while I think "case by case" generally applies.  ;-)  My inclination for
tests is to first check whether there's something salvageable.  If
someone wrote the test, it was probably important enough.  If it's indeed
"dead code", then of I'd go for just removing it.  I looked, and it seemed
to me that the test actually covers an aspect of printing that we don't
cover elsewhere, and actually reveals a bug.
So IMO, in this particular case, we should keep the test, remove the gcc check,
modernize and KFAIL it , and then have the bug fixed (if people agree it's
a bug).  I'm of course not suggesting you do that all of yourself, but
since you asked, that's what I'd prefer see done in this case.

Thanks,
Pedro Alves

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Doug Evans]

On Tue, Aug 19, 2014 at 10:50 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/19/2014 06:24 PM, Doug Evans wrote:
>
>>> Actually enabling the test (removing the skip, and adding
>>> nowarnings), we see that indeed GDB outputs no warning:
>>
>> But given the early exit the test itself is never run.
>
> As I said, I removed that.  ;-)

I know (I thought that was obvious).
If you want to actually check in its removal, we can have that discussion.

>> And it's been this way since at least 2003 (commit 4d9eda44f), and
>> longer (that commit just changed the style of the gcc test)!
>
> Yeah, this probably came in in the big HP merge, much
> earlier than that.  There was once a gdb.hp/ambiguous.exp, even, and
> this probably got copied from that.
> There was once a big
> everything-goes-we-dont-have-time-to-clean-things-up-before-accepting HP
> import/contribution, and bits may be skipped on GCC just because the
> HP folks at the time didn't want to deal with it in their local fork.
>
>> I'm all for filing a bug and recording the test in the bug report.
>> I'm even ok with keeping the test as is.
>> The high order bit for me is exploring what the community wishes be
>> done with this kind of "dead code".
>
> You're asking for a generic opinion, of the abstract "community",
> while I think "case by case" generally applies.  ;-)

I'm asking people for their opinion on this case (and cases like it).
If I call them a "community", which is what gdb@ and gdb-patches@ is
to me, I don't see the problem.

Removal of dead code is, in my experience here, *rarely* a case by
case decision.
[each case may have minor details that are different, but the high
order bit is generally DELETE IT :-)]
And yet this is a little different than an #if 0 in code, so I'm asking.
We certainly want to document this issue in some way, but if we're
just going to leave the test as is, not being run, and yet have to
disable it for new compilers as they come along, then I think there's
value in documenting the issue in a different way (e.g., bug reports
are fine for this sort of thing).

> My inclination for
> tests is to first check whether there's something salvageable.  If
> someone wrote the test, it was probably important enough.  If it's indeed
> "dead code", then of I'd go for just removing it.  I looked, and it seemed
> to me that the test actually covers an aspect of printing that we don't
> cover elsewhere, and actually reveals a bug.
> So IMO, in this particular case, we should keep the test, remove the gcc check,
> modernize and KFAIL it , and then have the bug fixed (if people agree it's
> a bug).  I'm of course not suggesting you do that all of yourself, but
> since you asked, that's what I'd prefer see done in this case.

This is feedback I can work with.  Thanks.

Going forward, as we discover bugs, are people ok with checking in a
KFAIL'd test for a bug before the bug is fixed?  "Just fix the bug."
is the canonical response.  And yet there are often higher priorities.
How do we document the bug's existence so that it's not forgotten
until someone has time to fix it?  File a bug report, of course.
Well, I file bug reports all the time, I certainly don't have time to
fix all of them instead of filing the bug report.  And if in
reproducing the bug I'm 80% of the way there in writing the test, is
it ok to at least finish and check in that part, even if I don't have
time to fix the bug?

Or, instead of allowing that, should we just "grandfather in", so to
speak, all existing disabled tests, not allow new KFAIL'd ones, and
migrate these existing tests to KFAIL if we don't have time to fix the
bug?

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Pedro Alves]

On 08/19/2014 09:10 PM, Doug Evans wrote:
> On Tue, Aug 19, 2014 at 10:50 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 08/19/2014 06:24 PM, Doug Evans wrote:
>>
>>>> Actually enabling the test (removing the skip, and adding
>>>> nowarnings), we see that indeed GDB outputs no warning:
>>>
>>> But given the early exit the test itself is never run.
>>
>> As I said, I removed that.  ;-)
> 
> I know (I thought that was obvious).

The statement you replied to clearly said I locally enabled the test
(_that_ much seems obvious to me), and you replied back as if I hadn't.
It just sounded like the start of going in circles.  So I still don't
know what did you intend to actually say or refute to.

>>> I'm all for filing a bug and recording the test in the bug report.
>>> I'm even ok with keeping the test as is.
>>> The high order bit for me is exploring what the community wishes be
>>> done with this kind of "dead code".
>>
>> You're asking for a generic opinion, of the abstract "community",
>> while I think "case by case" generally applies.  ;-)
> 
> I'm asking people for their opinion on this case (and cases like it).
> If I call them a "community", which is what gdb@ and gdb-patches@ is
> to me, I don't see the problem.

[
This is getting off topic, and about how we can improve communication
[which I am all for!], but I think the problem for me is in the very
lose definition of what is a "this case", and then wanting a generic rule.
I don't know what you mean by that exactly.  It begs for a generic
"thou shalt do this or that" kind of answer, but, personally, I find it
difficult to answer such a question.  I think we should be reasonable and
look at things in a little more detail than applying a rubber stamp rule.
So I did my best to actually look at the test and give out a rationale
for why I thought _enabling_ the test (because it's skipped today) would be
more useful than deleting it under a broad "dead code" rule.  The answer
I got back, in turn was, paraphrasing: "yeah, but never runs today or for
a long while!".  Well, I know that, of course, otherwise I wouldn't have
gone through the trouble of force-enabling the test locally!  Again, that
feels like a discussion that will quickly degenerate to going around in
circles.  It feels like we tend to talk past one another frequently,
while I believe there's no intention to on either side.  How can we
improve here?
]

> 
> Removal of dead code is, in my experience here, *rarely* a case by
> case decision.
> [each case may have minor details that are different, but the high
> order bit is generally DELETE IT :-)]

Right.  When I say "case by case", I'm kind of defending against
considering unreachable C code the same as #if 0 code that is serving
as comment the same as clearly dead testsuite/lib/*.exp code
the same as a test that happens to be skipped on the most popular platform.

I don't think it's so rare that an investigation finds that what is currently
dead could or should be reachable and a fix in that direction is made.
And we don't have to go very far for that:

 https://sourceware.org/ml/gdb-patches/2014-08/msg00180.html

> And yet this is a little different than an #if 0 in code, so I'm asking.

Yeah.

> We certainly want to document this issue in some way, but if we're
> just going to leave the test as is, not being run, and yet have to
> disable it for new compilers as they come along, then I think there's
> value in documenting the issue in a different way (e.g., bug reports
> are fine for this sort of thing).
> 
>> My inclination for
>> tests is to first check whether there's something salvageable.  If
>> someone wrote the test, it was probably important enough.  If it's indeed
>> "dead code", then of I'd go for just removing it.  I looked, and it seemed
>> to me that the test actually covers an aspect of printing that we don't
>> cover elsewhere, and actually reveals a bug.
>> So IMO, in this particular case, we should keep the test, remove the gcc check,
>> modernize and KFAIL it , and then have the bug fixed (if people agree it's
>> a bug).  I'm of course not suggesting you do that all of yourself, but
>> since you asked, that's what I'd prefer see done in this case.
> 
> This is feedback I can work with.  Thanks.
> 
> Going forward, as we discover bugs, are people ok with checking in a
> KFAIL'd test for a bug before the bug is fixed? 

It depends on the bug, but in general yes I'm okay with KFAILs.
I've added and fixed some myself.

> "Just fix the bug."
> is the canonical response.  

Again a generalization.  ;-)  Myself, I'll give out that response to
workarounds when I think the fix is doable in reasonable time compared
to the time needed to do the workaround.  I may be mistaken, but I don't
recall saying that in response to a new KFAIL test, unless again, I
trust the fix is more trivial than the submitter initially believes.

> And yet there are often higher priorities.
> How do we document the bug's existence so that it's not forgotten
> until someone has time to fix it?  File a bug report, of course.
> Well, I file bug reports all the time, I certainly don't have time to
> fix all of them instead of filing the bug report.  And if in
> reproducing the bug I'm 80% of the way there in writing the test, is
> it ok to at least finish and check in that part, even if I don't have
> time to fix the bug?

IMO, yes, within reason.

> Or, instead of allowing that, should we just "grandfather in", so to
> speak, all existing disabled tests, not allow new KFAIL'd ones, and
> migrate these existing tests to KFAIL if we don't have time to fix the
> bug?

I don't think we should forbid new KFAILed tests as a rule.

Thanks,
Pedro Alves

Re: [RFC] delete gdb.cp/ambiguous.exp ?

[Doug Evans]

[Note: When the "guidelines anyway" quote came to me,
I thought I'd have some light fun with the rest of the message.]

Pedro Alves writes:
 > On 08/19/2014 09:10 PM, Doug Evans wrote:
 > > On Tue, Aug 19, 2014 at 10:50 AM, Pedro Alves <palves@redhat.com> wrote:
 > >> On 08/19/2014 06:24 PM, Doug Evans wrote:
 > >>
 > >>>> Actually enabling the test (removing the skip, and adding
 > >>>> nowarnings), we see that indeed GDB outputs no warning:
 > >>>
 > >>> But given the early exit the test itself is never run.
 > >>
 > >> As I said, I removed that.  ;-)
 > > 
 > > I know (I thought that was obvious).
 > 
 > The statement you replied to clearly said I locally enabled the test
 > (_that_ much seems obvious to me), and you replied back as if I hadn't.

Misunderstanding then.

"Oh, the misunderstandings!"

http://www.imdb.com/title/tt0562996/

 > It just sounded like the start of going in circles.  So I still don't
 > know what did you intend to actually say or refute to.

It wasn't clear to me what, if anything, you were proposing.
Were you proposing to check in "[removing] the skip",
and possibly marking the FAILs as KFAILs?
And, possibly leaving it at that until someone has time to fix the bug?
I know you were showing what would happen if the check was removed.
But it was not clear at all that you were proposing to check that in.

Further confusion on my part came when there was a question of
whether there was actually a bug here"
"So shouldn't GDB error out in the 'x.x' case too ?  Or at
least warn, like the test suggests"

One recognizes that there is (at least one) gdb bug here.
But instead of a bug report we have a testcase that has been disabled
to, presumably, document the issue in place of a bug report.
It's not the norm here.

"It's an anomaly. Anomalies bug me."

http://www.imdb.com/title/tt0606021/

 > >>> I'm all for filing a bug and recording the test in the bug report.
 > >>> I'm even ok with keeping the test as is.
 > >>> The high order bit for me is exploring what the community wishes be
 > >>> done with this kind of "dead code".
 > >>
 > >> You're asking for a generic opinion, of the abstract "community",
 > >> while I think "case by case" generally applies.  ;-)
 > > 
 > > I'm asking people for their opinion on this case (and cases like it).
 > > If I call them a "community", which is what gdb@ and gdb-patches@ is
 > > to me, I don't see the problem.
 > 
 > [
 > This is getting off topic, and about how we can improve communication
 > [which I am all for!], but I think the problem for me is in the very
 > lose definition of what is a "this case", and then wanting a generic rule.

I did not use the word "rule", as if I'm looking for a "thou shalt"
kind of thing.  [as opposed to, say, guidelines]
I'm curious if anyone else thought that, and if so ... why?

"Hang the code, and hang the rules. They're more like guidelines anyway."

http://www.imdb.com/search/text?realm=title&field=quotes&q=guidelines%20anyway

 > I don't know what you mean by that exactly.

Ask for clarification.

"Clarification - its a beautiful thing."

http://www.imdb.com/title/tt0606036/

 > It begs for a generic
 > "thou shalt do this or that" kind of answer, but, personally, I find it
 > difficult to answer such a question.  I think we should be reasonable and
 > look at things in a little more detail than applying a rubber stamp rule.

"I think we should be reasonable ..."
I think we can assume we're in agreement here.

"Well, this is an Enterprise first - Dr. McCoy, Mr. Spock and
Engineer Scott find themselves in complete agreement.
Can I stand the strain?"

http://www.imdb.com/title/tt0708468/

 > So I did my best to actually look at the test and give out a rationale
 > for why I thought _enabling_ the test (because it's skipped today) would be
 > more useful than deleting it under a broad "dead code" rule.  The answer
 > I got back, in turn was, paraphrasing: "yeah, but never runs today or for
 > a long while!".  Well, I know that, of course, otherwise I wouldn't have
 > gone through the trouble of force-enabling the test locally!  Again, that
 > feels like a discussion that will quickly degenerate to going around in
 > circles.  It feels like we tend to talk past one another frequently,
 > while I believe there's no intention to on either side.  How can we
 > improve here?
 > ]

As a start, I propose asking for clarification earlier and more clearly,
and without going down a road of making assumptions about what might
have been meant.

"It's all becoming clear. The Doctor is the man you'd like to be,
doing impossible things with cricket balls."

http://www.imdb.com/title/tt1000254/

 > > 
 > > Removal of dead code is, in my experience here, *rarely* a case by
 > > case decision.
 > > [each case may have minor details that are different, but the high
 > > order bit is generally DELETE IT :-)]
 > 
 > Right.  When I say "case by case", I'm kind of defending against
 > considering unreachable C code the same as #if 0 code that is serving
 > as comment the same as clearly dead testsuite/lib/*.exp code
 > the same as a test that happens to be skipped on the most popular platform.
 > 
 > I don't think it's so rare that an investigation finds that what is currently
 > dead could or should be reachable and a fix in that direction is made.
 > And we don't have to go very far for that:
 > 
 >  https://sourceware.org/ml/gdb-patches/2014-08/msg00180.html
 > 
 > > And yet this is a little different than an #if 0 in code, so I'm asking.
 > 
 > Yeah.

"I'm confused.
Yeah, well it's a big club - we should get t-shirts."

http://www.imdb.com/title/tt1721225/

 > > We certainly want to document this issue in some way, but if we're
 > > just going to leave the test as is, not being run, and yet have to
 > > disable it for new compilers as they come along, then I think there's
 > > value in documenting the issue in a different way (e.g., bug reports
 > > are fine for this sort of thing).
 > > 
 > >> My inclination for
 > >> tests is to first check whether there's something salvageable.  If
 > >> someone wrote the test, it was probably important enough.  If it's indeed
 > >> "dead code", then of I'd go for just removing it.  I looked, and it seemed
 > >> to me that the test actually covers an aspect of printing that we don't
 > >> cover elsewhere, and actually reveals a bug.
 > >> So IMO, in this particular case, we should keep the test, remove the gcc check,
 > >> modernize and KFAIL it , and then have the bug fixed (if people agree it's
 > >> a bug).  I'm of course not suggesting you do that all of yourself, but
 > >> since you asked, that's what I'd prefer see done in this case.
 > > 
 > > This is feedback I can work with.  Thanks.
 > > 
 > > Going forward, as we discover bugs, are people ok with checking in a
 > > KFAIL'd test for a bug before the bug is fixed? 
 > 
 > It depends on the bug, but in general yes I'm okay with KFAILs.
 > I've added and fixed some myself.

This is feedback I can use.

 > > "Just fix the bug."
 > > is the canonical response.  
 > 
 > Again a generalization.  ;-)

Eh?  I'm stating what I've seen from my experience.
We may have different definitions of the word "canonical",
and mine may certainly be different than yours.

"Good different or bad different?
Just different."

http://www.imdb.com/title/tt0562994/

 > Myself, I'll give out that response to
 > workarounds when I think the fix is doable in reasonable time compared
 > to the time needed to do the workaround.  I may be mistaken, but I don't
 > recall saying that in response to a new KFAIL test, unless again, I
 > trust the fix is more trivial than the submitter initially believes.
 > 
 > > And yet there are often higher priorities.
 > > How do we document the bug's existence so that it's not forgotten
 > > until someone has time to fix it?  File a bug report, of course.
 > > Well, I file bug reports all the time, I certainly don't have time to
 > > fix all of them instead of filing the bug report.  And if in
 > > reproducing the bug I'm 80% of the way there in writing the test, is
 > > it ok to at least finish and check in that part, even if I don't have
 > > time to fix the bug?
 > 
 > IMO, yes, within reason.

This is usable feedback.

 > > Or, instead of allowing that, should we just "grandfather in", so to
 > > speak, all existing disabled tests, not allow new KFAIL'd ones, and
 > > migrate these existing tests to KFAIL if we don't have time to fix the
 > > bug?
 > 
 > I don't think we should forbid new KFAILed tests as a rule.

More usable feedback.  Thanks.

"Data, you sound as if you're stuck in a... feedback loop!"

http://www.imdb.com/title/tt0708688/