[PATCH] gdb/testsuite: make runto always emit a FAIL on internal error

[Pedro Alves]

On 8/20/20 6:45 PM, Simon Marchi via Gdb-patches wrote:
> I noticed that when a test uses `runto_main` and a GDB internal error
> happens while running to main, no error or fail is emitted.  This is
> because `runto_main` uses the `no-message` option of `runto`.
> 
> As a result, if a test fails to run to main and exits, no sign that
> something went wrong is emitted.  For example, add this always-false
> assertion to compute_frame_id:
> 
>     --- a/gdb/frame.c
>     +++ b/gdb/frame.c
>     @@ -545,6 +545,7 @@ static void
>      compute_frame_id (struct frame_info *fi)
>      {
>        gdb_assert (!fi->this_id.p);
>     +  gdb_assert (false);
> 
>        if (frame_debug)
>          fprintf_unfiltered (gdb_stdlog, "{ compute_frame_id (fi=%d) ",
> 
> ... and run gdb.dwarf2/dw2-align.exp.  No fail or sign that something
> went wrong is shown.  It just appears as if the test gets skipped.
> 
> A developer introducing such a regression in this test today would
> likely notice it, because we are used to diff-ing test results.  So we
> would see some PASSes dispappear for no good reason and look into it.
> 
> But I find it worrysome for two reasons:
> 
> 1. Scripts that analyze regressions (such as the one on the buildbot)
>    may only look for new FAILs or new ERRORs.  It would probably miss
>    this.
> 2. Imagine that we one day have a testsuite that runs cleanly (some
>    people might already run subsets of the testsuite and expect it to
>    all pass), we would just run the testsuite and check that there are
>    no fails (`make check` exits with 0).  It would be easy to miss
>    something like this.
> 
> In case of internal error, I suggest making `runto` emit a FAIL even if
> `no-message` was passed.  This is different from other failure modes
> that might be expected (whchi rightfully cause the test to simply be
> skipped).  An internal error is always bad, so if it happens it should
> noisily fail.

It seems to me that the whole rationale given here is also applicable to
any other failure in runto_main.  In that scenario, the testcase
will equally silently be skipped, which also seems bad.

I'd be more inclined to think that instead we consider removing
no-message from runto_main.  That would also let us remove
the many "cannot run to main" fails sprinkled all over the
place.  I think that all the times I've seen a runto_main call
that forgets to call "fail" (or even check return and bail),
it's been a bug.  And we have many of those.  So must making
runto_main FAIL itself would just fix that for good (except
the bailing out part).

If some caller wants to pass no-message _and_ distinguish internal
errors from other errors, then I'm thinking we should make runto
return a different number for internal errors, like:
   1=success, 0=error, -1=internal-error.

Though I'm not thinking of why a caller would want it -- normally
if you can't run to main for any reason, you'll want to stop the
test.

Thanks,
Pedro Alves