[PATCH] Don't run forever in gdb.base/structs.c

[PATCH] Don't run forever in gdb.base/structs.c

[Doug Evans]

Hi.

If gdb crashes during testing tests may be left to free-run, eating cpu.

This patch fixes one of the more egregious cases since several versions
of the program are built.

I've got patches to fix others.
Just seeing if folks want to comment on this first.

IWBN to have the harness itself cleanup, and I think there's something
we can do there, but that's not always robust either, and I think
multiple levels of robustness would be useful.
Since this testcase is an egregious one, and since this patch simple,
I'm starting with this.

I thought of trying to make the loop limit something less random,
but that itself introduces complications, e.g., if one wants to extend
the test.  So I kept it simple.
One could instead reorganize the test, but it's not worth it,
at least not at this point.

2014-10-01  Doug Evans  <dje@google.com>

	* gdb.base/structs.c (main): Don't run forever.

diff --git a/gdb/testsuite/gdb.base/structs.c b/gdb/testsuite/gdb.base/structs.c
index 60772bb..d0b69a8 100644
--- a/gdb/testsuite/gdb.base/structs.c
+++ b/gdb/testsuite/gdb.base/structs.c
@@ -425,12 +425,14 @@ int main()
   Fun17(foo17);
   Fun18(foo18);
 
-  /* An infinite loop that first clears all the variables and then
+  /* An (almost-)infinite loop that first clears all the variables and then
      calls each function.  This "hack" is to make testing random
      functions easier - "advance funN" is guaranteed to have always
-     been preceded by a global variable clearing zed call.  */
+     been preceded by a global variable clearing zed call.
+     We don't let this run forever in case gdb crashes while testing,
+     we don't want to be left eating all cpu on the user's system.  */
 
-  while (1)
+  for (i = 0; i < 1000000; ++i)
     {
       zed ();
       L1  = fun1();	

Re: [PATCH] Don't run forever in gdb.base/structs.c

[Pedro Alves]

On 10/01/2014 10:02 PM, Doug Evans wrote:

> If gdb crashes during testing tests may be left to free-run, eating cpu.
> 
> This patch fixes one of the more egregious cases since several versions
> of the program are built.
> 
> I've got patches to fix others.
> Just seeing if folks want to comment on this first.
> 
> IWBN to have the harness itself cleanup, and I think there's something
> we can do there, but that's not always robust either, and I think
> multiple levels of robustness would be useful.

Agreed.

> Since this testcase is an egregious one, and since this patch simple,
> I'm starting with this.

Looks fine with me.

We already do something like this in many tests even.  E.g., of the
top of my head:

$ grep -rn "Don't run forever.  Run just short of it :)" *
gdb.base/watch_thread_num.c:55:    /* Don't run forever.  Run just short of it :)  */
gdb.mi/nsintrall.c:55:    /* Don't run forever.  Run just short of it :)  */
gdb.mi/nsmoribund.c:35:  /* Don't run forever.  Run just short of it :)  */
gdb.threads/pending-step.c:54:    /* Don't run forever.  Run just short of it :)  */
gdb.threads/watchthreads.c:71:    /* Don't run forever.  Run just short of it :)  */
gdb.threads/threadapply.c:72:    /* Don't run forever.  Run just short of it :)  */
gdb.threads/thread-specific.c:42:    /* Don't run forever.  Run just short of it :)  */
gdb.threads/thread-specific.c:56:    /* Don't run forever.  Run just short of it :)  */
gdb.threads/schedlock.c:55:    /* Don't run forever.  Run just short of it :)  */

In a few other tests, we use "alarm()", though IMO it's best to avoid
that if possible, to expose the test on as much targets as possible.
E.g., alarm() IIRC isn't available on mingw unless you
specify __USE_MINGW_ALARM.  Bare metal targets may have trouble
with it too, etc.

Thanks,
Pedro Alves

Re: [PATCH] Don't run forever in gdb.base/structs.c

[Doug Evans]

On Wed, Oct 1, 2014 at 4:51 PM, Pedro Alves <palves@redhat.com> wrote:
> On 10/01/2014 10:02 PM, Doug Evans wrote:
>
>> If gdb crashes during testing tests may be left to free-run, eating cpu.
>>
>> This patch fixes one of the more egregious cases since several versions
>> of the program are built.
>>
>> I've got patches to fix others.
>> Just seeing if folks want to comment on this first.
>>
>> IWBN to have the harness itself cleanup, and I think there's something
>> we can do there, but that's not always robust either, and I think
>> multiple levels of robustness would be useful.
>
> Agreed.
>
>> Since this testcase is an egregious one, and since this patch simple,
>> I'm starting with this.
>
> Looks fine with me.

Committed, thanks.

> We already do something like this in many tests even.  E.g., of the
> top of my head:
>
> $ grep -rn "Don't run forever.  Run just short of it :)" *
> gdb.base/watch_thread_num.c:55:    /* Don't run forever.  Run just short of it :)  */
> gdb.mi/nsintrall.c:55:    /* Don't run forever.  Run just short of it :)  */
> gdb.mi/nsmoribund.c:35:  /* Don't run forever.  Run just short of it :)  */
> gdb.threads/pending-step.c:54:    /* Don't run forever.  Run just short of it :)  */
> gdb.threads/watchthreads.c:71:    /* Don't run forever.  Run just short of it :)  */
> gdb.threads/threadapply.c:72:    /* Don't run forever.  Run just short of it :)  */
> gdb.threads/thread-specific.c:42:    /* Don't run forever.  Run just short of it :)  */
> gdb.threads/thread-specific.c:56:    /* Don't run forever.  Run just short of it :)  */
> gdb.threads/schedlock.c:55:    /* Don't run forever.  Run just short of it :)  */
>
> In a few other tests, we use "alarm()", though IMO it's best to avoid
> that if possible, to expose the test on as much targets as possible.
> E.g., alarm() IIRC isn't available on mingw unless you
> specify __USE_MINGW_ALARM.  Bare metal targets may have trouble
> with it too, etc.

Agreed.

Re: [PATCH] Don't run forever in gdb.base/structs.c

[Pedro Alves]

On 10/02/2014 09:10 PM, Doug Evans wrote:
> On Wed, Oct 1, 2014 at 4:51 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 10/01/2014 10:02 PM, Doug Evans wrote:
>>
>>> If gdb crashes during testing tests may be left to free-run, eating cpu.
>>>
>>> This patch fixes one of the more egregious cases since several versions
>>> of the program are built.
>>>
>>> I've got patches to fix others.
>>> Just seeing if folks want to comment on this first.
>>>
>>> IWBN to have the harness itself cleanup, and I think there's something
>>> we can do there, but that's not always robust either, and I think
>>> multiple levels of robustness would be useful.
>>
>> Agreed.
>>
>>> Since this testcase is an egregious one, and since this patch simple,
>>> I'm starting with this.
>>
>> Looks fine with me.
> 
> Committed, thanks.

Thank you.  Guess this could be material for:

 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

?

>> In a few other tests, we use "alarm()", though IMO it's best to avoid
>> that if possible, to expose the test on as much targets as possible.
>> E.g., alarm() IIRC isn't available on mingw unless you
>> specify __USE_MINGW_ALARM.  Bare metal targets may have trouble
>> with it too, etc.
> 
> Agreed.

Expanding on that a bit: TBC, I see no problem with alarm() if the
test is already using other posix-ish things, like pthread_create,
or signal.  I'd avoid iff the test otherwise could be mostly
plain C/C++ and OS independent, like structs.c.

Thanks,
Pedro Alves

Re: [PATCH] Don't run forever in gdb.base/structs.c

[Doug Evans]

On Fri, Oct 3, 2014 at 1:57 AM, Pedro Alves <palves@redhat.com> wrote:
> On 10/02/2014 09:10 PM, Doug Evans wrote:
>> On Wed, Oct 1, 2014 at 4:51 PM, Pedro Alves <palves@redhat.com> wrote:
>>> On 10/01/2014 10:02 PM, Doug Evans wrote:
>>>
>>>> If gdb crashes during testing tests may be left to free-run, eating cpu.
>>>>
>>>> This patch fixes one of the more egregious cases since several versions
>>>> of the program are built.
>>>>
>>>> I've got patches to fix others.
>>>> Just seeing if folks want to comment on this first.
>>>>
>>>> IWBN to have the harness itself cleanup, and I think there's something
>>>> we can do there, but that's not always robust either, and I think
>>>> multiple levels of robustness would be useful.
>>>
>>> Agreed.
>>>
>>>> Since this testcase is an egregious one, and since this patch simple,
>>>> I'm starting with this.
>>>
>>> Looks fine with me.
>>
>> Committed, thanks.
>
> Thank you.  Guess this could be material for:
>
>  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

Good idea.  Done.