[patch 2/2] Assert leftover cleanups in TRY_CATCH

[patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

Hi,

as discussed in:
	Re: Cleanups and Exception handlers
	http://sourceware.org/ml/gdb/2013-05/msg00007.html
	Message-ID: <20130501152116.GA7529@host2.jankratochvil.net>

this gdb_assert can be useful.  Unfortunately there may be leftover
"regressions" due to it not caught by the testsuite.


Thanks,
Jan


gdb/
2013-05-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* cleanups.c (restore_my_cleanups): New gdb_assert for SENTINEL_CLEANUP.

diff --git a/gdb/cleanups.c b/gdb/cleanups.c
index c403db7..02db9f5 100644
--- a/gdb/cleanups.c
+++ b/gdb/cleanups.c
@@ -261,6 +261,7 @@ save_final_cleanups (void)
 static void
 restore_my_cleanups (struct cleanup **pmy_chain, struct cleanup *chain)
 {
+  gdb_assert (*pmy_chain == SENTINEL_CLEANUP);
   *pmy_chain = chain;
 }
 

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Pedro Alves]

On 05/01/2013 05:57 PM, Jan Kratochvil wrote:

> gdb/
> 2013-05-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* cleanups.c (restore_my_cleanups): New gdb_assert for SENTINEL_CLEANUP.

This is fine with me.

Thanks,
-- 
Pedro Alves

[commit] [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Thu, 02 May 2013 19:01:58 +0200, Pedro Alves wrote:
> On 05/01/2013 05:57 PM, Jan Kratochvil wrote:
> > 	* cleanups.c (restore_my_cleanups): New gdb_assert for SENTINEL_CLEANUP.
> 
> This is fine with me.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-05/msg00022.html


Thanks,
Jan

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> 2013-05-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* cleanups.c (restore_my_cleanups): New gdb_assert for SENTINEL_CLEANUP.

Thanks for doing this.  I think it is a nice addition.

We could do this for all cleanup-creating functions, at least when using
GCC, if we didn't mind putting a declaration at the start of each such
function:

#if ... gcc ..
#define CHECK_CLEANUP \
  struct cleanup *__dummy ## __LINE__ \
     __attribute__ ((cleanup (check_cleanup))) \
     = get_checking_cleanup_pointer ();
#endif


This would call check_cleanup when the function exited normally, so we
could verify that the cleanup chain was properly reset.

I think it would be possible to automate adding this declaration in all
needed spots.  I'm curious what you think about it.

Tom

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Mon, 06 May 2013 19:56:56 +0200, Tom Tromey wrote:
> I'm curious what you think about it.

C++ exceptions solve it all, everyone knows it, it is simple, effective and at
least in comparison with the existing GDB system it is foolproof.


Jan

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> On Mon, 06 May 2013 19:56:56 +0200, Tom Tromey wrote:
>> I'm curious what you think about it.

Jan> C++ exceptions solve it all, everyone knows it, it is simple,
Jan> effective and at least in comparison with the existing GDB system
Jan> it is foolproof.

I know, and I agree that it would yield a better gdb, but I don't think
it is going to happen.

Given that, and the constant stream of cleanup bugs -- or potential
bugs, relying on constant and careful review to catch them -- I think we
have to examine second-best solutions.

The cleanup checker is one attempt at this.  I'm not sure everybody will
accept the code changes it needs.  Checking cleanups at runtime is
another approach.

Tom

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Mon, 06 May 2013 20:50:47 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> C++ exceptions solve it all, everyone knows it, it is simple,
> Jan> effective and at least in comparison with the existing GDB system
> Jan> it is foolproof.
> 
> I know, and I agree that it would yield a better gdb, but I don't think
> it is going to happen.

I can't seriously reply these questions anymore.  GCC already requires C++ so
why GDB cannot?  I do not remember any valid reason against C++ from all the
GDB discussions around it.

GDB still is barely usable for real C++ application debugging, debugging
multiple virtual class inheritance does not work, one has to use printfs
instead.  Inferior breakpoint with conditional to stop only after thousands of
iterations is so unusably slow it is faster to rebuild the inferior with the
conditional put into inferior's source. etc. etc.

And with all this work ahead continuously wasting engineering time on
reimplementing C++-in-C...

Although I was waiting for -Wc++-compat by Matt Rice at least as a first stap
but that probably won't happen by Matt Rice so it needs a reassignment.


Jan

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Doug Evans]

On Mon, May 6, 2013 at 6:47 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Mon, 06 May 2013 20:50:47 +0200, Tom Tromey wrote:
>> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
>> Jan> C++ exceptions solve it all, everyone knows it, it is simple,
>> Jan> effective and at least in comparison with the existing GDB system
>> Jan> it is foolproof.
>>
>> I know, and I agree that it would yield a better gdb, but I don't think
>> it is going to happen.
>
> I can't seriously reply these questions anymore.  GCC already requires C++ so
> why GDB cannot?  I do not remember any valid reason against C++ from all the
> GDB discussions around it.
>
> GDB still is barely usable for real C++ application debugging, debugging
> multiple virtual class inheritance does not work, one has to use printfs
> instead.  Inferior breakpoint with conditional to stop only after thousands of
> iterations is so unusably slow it is faster to rebuild the inferior with the
> conditional put into inferior's source. etc. etc.

I'm not going to enter an opinion of C over C++ here,
but I don't see how inferior breakpoint slowness is related.
[I can imagine a reason why multiple virtual class inheritance is related,
but then again I'm not sure I'd want to see gdb use it itself.]

btw, is the slowness just an all-stop thing?
Or is there room for massive improvement in non-stop too?

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Doug Evans]

On Mon, May 6, 2013 at 9:37 PM, Doug Evans <dje@google.com> wrote:
> On Mon, May 6, 2013 at 6:47 PM, Jan Kratochvil
> btw, is the slowness just an all-stop thing?
> Or is there room for massive improvement in non-stop too?

I'm setting aside target-side condition evaluation of course.

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Tue, 07 May 2013 06:37:08 +0200, Doug Evans wrote:
> but I don't see how inferior breakpoint slowness is related.

It is very unrelated but it was illustrating there is still a lot of work on
GDB to do.


> [I can imagine a reason why multiple virtual class inheritance is related,
> but then again I'm not sure I'd want to see gdb use it itself.]

I do not know if there is a use case in GDB really worth it; it was again just
an illustration there is still a lot of work on GDB to do.


> btw, is the slowness just an all-stop thing?
> Or is there room for massive improvement in non-stop too?

It was single-threaded mode.


On Tue, 07 May 2013 06:49:48 +0200, Doug Evans wrote:
> I'm setting aside target-side condition evaluation of course.

target-side condition evaluation is a good idea:

time gdb ./loop -ex 'b 4 if i==360000' -ex r -q -ex 'set confirm no' -ex q
real	1m11.586s

gdbserver :1234 ./loop
time gdb ./loop -ex 'target remote localhost:1234' -ex 'b 4 if i==360000' -ex c -q -ex 'set confirm no' -ex q
real	0m21.862s

"set breakpoint condition-evaluation target" really helps a lot.
Although recompilation with a compiled-in conditional is still faster.

loop.c:
--------------------
#include <stdio.h>
int main(void) {
  for (int i=0;;i++)
    if (i==360000)
      break;
  return 0;
}


Regards,
Jan

Benchmarking (was Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH)

[Stan Shebs]

On 5/7/13 7:00 AM, Jan Kratochvil wrote:

> 
> target-side condition evaluation is a good idea:
> 
> time gdb ./loop -ex 'b 4 if i==360000' -ex r -q -ex 'set confirm no' -ex q
> real	1m11.586s
> 
> gdbserver :1234 ./loop
> time gdb ./loop -ex 'target remote localhost:1234' -ex 'b 4 if i==360000' -ex c -q -ex 'set confirm no' -ex q
> real	0m21.862s
> 
> "set breakpoint condition-evaluation target" really helps a lot.

This reminds me of something that has been on my mind recently -
detecting performance regression with the testsuite.

I added a test for fast tracepoints a while back (tspeed.exp) that also
went to some trouble to get numbers for fast tracepoint performance,
although it just reports them, they are not used to pass/fail.

However, if target-side conditionals get worse due to some random
change, or GDB startup time gets excessive, these are things that we
know real users care about.  On the other hand, this is hard to test
automatically, and no wants to hack dejagnu that much.  Maybe an excuse
to dabble in a more-modern testing framework?  Are there good options?

Stan
stan@codesourcery.com

Re: Benchmarking (was Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH)

[Doug Evans]

On Tue, May 14, 2013 at 5:13 PM, Stan Shebs <stanshebs@earthlink.net> wrote:
> On 5/7/13 7:00 AM, Jan Kratochvil wrote:
>
>>
>> target-side condition evaluation is a good idea:
>>
>> time gdb ./loop -ex 'b 4 if i==360000' -ex r -q -ex 'set confirm no' -ex q
>> real  1m11.586s
>>
>> gdbserver :1234 ./loop
>> time gdb ./loop -ex 'target remote localhost:1234' -ex 'b 4 if i==360000' -ex c -q -ex 'set confirm no' -ex q
>> real  0m21.862s
>>
>> "set breakpoint condition-evaluation target" really helps a lot.
>
> This reminds me of something that has been on my mind recently -
> detecting performance regression with the testsuite.

tis on my todo list.
Got a time machine?

IIRC Redhat had the seeds of something, but it needed more work.

> I added a test for fast tracepoints a while back (tspeed.exp) that also
> went to some trouble to get numbers for fast tracepoint performance,
> although it just reports them, they are not used to pass/fail.
>
> However, if target-side conditionals get worse due to some random
> change, or GDB startup time gets excessive, these are things that we
> know real users care about.  On the other hand, this is hard to test
> automatically, and no wants to hack dejagnu that much.  Maybe an excuse
> to dabble in a more-modern testing framework?  Are there good options?

re: dejagnu hacking: depends on what's needed.

Sometimes regressions aren't really noticed unless one is debugging a
really large app.
A second slowdown might be attributable to many things, but  in a
bigger app it could be minutes and now we're talking real money.

It's trivial enough to write a program to generate apps (of whatever
size and along whatever axis is useful) for the task at hand.
And it's trivial enough to come up with a set of benchmarks (I have an
incomplete set I use for my standard large app benchmark), and a
harness to run them.
IWBN if running the tests didn't take a lot of time but alas some
things only show up at scale.
Plus one needs to run them a sufficient number of times to make the data usable.
Running a benchmark with different sized tests and comparing the
relative times can help.

[One thing one could do is, e.g., run gdb under valgrind and use
instruction counts as a proxy for performance.
[One also needs to measure memory usage.]
It has the property of being deterministic, and with a set of
testcases for each benchmark could reveal problems.
One can't do just this because it doesn't measure, e.g., disk/network
latency which can be critical.
I'm sure one could write a tool to approximate the times.
Going this route is slower of course.]

Ultimately I'd expect this to be separate from "make check" (or at
least something one has to ask for explicitly).
But we *do* need something and the sooner the better.

Re: Benchmarking (was Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH)

[Tom Tromey]

Doug> IIRC Redhat had the seeds of something, but it needed more work.

I don't remember it.

I think some standard benchmarking tests would be wonderful.

Tom

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I can't seriously reply these questions anymore.  GCC already
Jan> requires C++ so why GDB cannot?  I do not remember any valid reason
Jan> against C++ from all the GDB discussions around it.

I was frustrated too, but I'm not the one to convince.

Jan> GDB still is barely usable for real C++ application debugging,
Jan> debugging multiple virtual class inheritance does not work

Could you make sure to file bugs for this?
I was surprised to hear there are still bugs here.

Tom

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Tue, 07 May 2013 16:35:50 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> GDB still is barely usable for real C++ application debugging,
> Jan> debugging multiple virtual class inheritance does not work
> 
> Could you make sure to file bugs for this?
> I was surprised to hear there are still bugs here.

Filed (seems to be harmless now but it looked as broken to me):
	virtual inheritance: __in_chrg and __vtt_parm
	http://sourceware.org/bugzilla/show_bug.cgi?id=15437

The other problem with "double-pointers" is in fact an already filed one:
	"set print object on" + printing variables of type: a reference to derived class.
	http://sourceware.org/bugzilla/show_bug.cgi?id=15401


Jan

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Joel Brobecker]

> Thanks for doing this.  I think it is a nice addition.

I agree. My only additional comment, after having hit this assertion,
is that it seems a bit harsh to be doing an abort in this case.
The problem showed up during a testsuite run, and so I would
investigate it regardless. But I can imagine the same issue
occurring during a real debugging session trying to chase a bug -
I'd be pretty upset to see my session ended like that.

Should we consider changing it into an internal_warning?

> We could do this for all cleanup-creating functions, at least when using
> GCC, if we didn't mind putting a declaration at the start of each such
> function:
> 
> #if ... gcc ..
> #define CHECK_CLEANUP \
>   struct cleanup *__dummy ## __LINE__ \
>      __attribute__ ((cleanup (check_cleanup))) \
>      = get_checking_cleanup_pointer ();
> #endif
> 
> 
> This would call check_cleanup when the function exited normally, so we
> could verify that the cleanup chain was properly reset.

That would have helped my investigation, because the error
was raised in a frame long after the function causing the problem
had returned. Looking at the contents of the cleanup_chain,
you get a hint of what it might be, but in my case, it was
a null_cleanup, so not so helpful...

> I think it would be possible to automate adding this declaration in
> all needed spots.  I'm curious what you think about it.

I'm certainly curious about the suggestion. How would the current
code be adapted to make this work?

-- 
Joel

[patch] [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Tue, 07 May 2013 08:23:05 +0200, Joel Brobecker wrote:
> I agree. My only additional comment, after having hit this assertion,
> is that it seems a bit harsh to be doing an abort in this case.
> The problem showed up during a testsuite run, and so I would
> investigate it regardless. But I can imagine the same issue
> occurring during a real debugging session trying to chase a bug -
> I'd be pretty upset to see my session ended like that.
> 
> Should we consider changing it into an internal_warning?

I forgot the direction GDB should not crash on recoverable issues.


Jan

gdb/
2013-05-07  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* cleanups.c (restore_my_cleanups): Replace gdb_assert by
	internal_warning.


diff --git a/gdb/cleanups.c b/gdb/cleanups.c
index 02db9f5..1b5d973 100644
--- a/gdb/cleanups.c
+++ b/gdb/cleanups.c
@@ -261,7 +261,10 @@ save_final_cleanups (void)
 static void
 restore_my_cleanups (struct cleanup **pmy_chain, struct cleanup *chain)
 {
-  gdb_assert (*pmy_chain == SENTINEL_CLEANUP);
+  if (*pmy_chain != SENTINEL_CLEANUP)
+    internal_warning (__FILE__, __LINE__,
+		      _("restore_my_cleanups has found a stale cleanup"));
+
   *pmy_chain = chain;
 }
 

[commit] [patch] [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Tue, 07 May 2013 16:19:57 +0200, Jan Kratochvil wrote:
> gdb/
> 2013-05-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* cleanups.c (restore_my_cleanups): Replace gdb_assert by
> 	internal_warning.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-05/msg00115.html


Jan

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Tom Tromey]

Joel> Should we consider changing it into an internal_warning?

I think that is fine as long as it still makes tests fail.

Tom> I think it would be possible to automate adding this declaration in
Tom> all needed spots.  I'm curious what you think about it.

Joel> I'm certainly curious about the suggestion. How would the current
Joel> code be adapted to make this work?

Well, first a manual patch both to define this macro appropriately and
to add whatever supporting functions are needed.

Then, modify the cleanup checker to add this declaration to any function
it thinks could use it.  The cleanup checker already has some code,
written in a more optimistic time, to try to determine whether a
function could be converted to "RAII style".  It could be adapted to
insert the macro use at the right spot.

Then, rebuild and fix whatever bugs were introduced.

Finally, update our patch review guidelines so we know to look for this
macro in ordinary cleanup-using functions.


Alternatively I think we could probably change all the code to be
cleanup-checker-clean.  I'll try to prep that series soon to see what
people think.  I think it actually less work.

Tom

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Jan Kratochvil]

On Tue, 07 May 2013 16:40:32 +0200, Tom Tromey wrote:
> Joel> Should we consider changing it into an internal_warning?
> 
> I think that is fine as long as it still makes tests fail.

FYI even the gdb_assert did not make all tests fail, I had to grep gdb.log.


Jan

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Tom Tromey]

Tom> Alternatively I think we could probably change all the code to be
Tom> cleanup-checker-clean.  I'll try to prep that series soon to see what
Tom> people think.  I think it actually less work.

Actually, I forgot how hard this is.

There are some tricky cases -- e.g., try to remove the dangling cleanup
from c-exp.y:operator_stoken, or from
ada-lang.c:old_renaming_is_invisible.

Or, there is some spaghetti code, like
disasm.c:do_mixed_source_and_assembly.

And, there is some code where I don't know what is intended, like
record-full.c:record_full_wait_1 (multiple returns without cleaning up),
or record-btrace.c:record_btrace_insn_history.

So, getting to zero complaints from the checker is not easy.
Maybe I will hack the checker to ignore some issues.

Tom

Re: [patch 2/2] Assert leftover cleanups in TRY_CATCH

[Joel Brobecker]

> Joel> Should we consider changing it into an internal_warning?
> 
> I think that is fine as long as it still makes tests fail.

FWIW, agreed. I saw Jan's message that some tests did not fail, but
that is a test issue, IMO. In my case, the warning would have
caused a failure as well.

-- 
Joel