Generalizing DejaGnu timeout scaling (was: Re: [PATCH DejaGNU/GCC 0/1] Support per-test execution timeout factor)

[Jacob Bachmeyer]

Maciej W. Rozycki wrote:
> On Wed, 3 Jan 2024, Hans-Peter Nilsson wrote:
>
>   
> > >  The test execution timeout is different from the tool execution timeout 
> > > where it is GCC execution that is being guarded against taking excessive 
> > > amount of time on the test host rather than the resulting test case 
> > > executable run on the target afterwards, as concerned here.  GCC already 
> > > has a `dg-timeout-factor' setting for the tool execution timeout, but has 
> > > no means to increase the test execution timeout.  The GCC side of these 
> > > changes adds a corresponding `dg-test-timeout-factor' setting.
> > >       
> > Hmm.  I think it would be more correct to emphasize that the 
> > existing dg-timeout-factor affects both the tool execution *and* 
> > the test execution, whereas your new dg-test-timeout-factor only 
> > affects the test execution.  (And still measured on the host.)
> >     
>
>  Not really, `dg-timeout-factor' is only applied to tool execution and it 
> doesn't affect test execution.  Timeout value reporting used to be limited 
> in DejaGNU, but you can enable it easily now by adding the DejaGNU patch 
> series referred in the cover letter and see that `dg-timeout-factor' is 
> ignored for test execution.
>   

Then we need a better name for this new feature that more clearly 
indicates that it applies to running executables compiled as part of a 
test.  Also, 'test_timeout' is documented as a knob for site 
configuration to twiddle, not for testsuites to adjust.  I support 
adding scale factors for testsuites to indicate "this test takes longer 
than usual" but these will need to be thought through.  This quick hack 
will cause future maintenance problems.

> > Usually the compilation time is close to 0, so is this based on 
> > an actual need more than an itchy "wart"?
> >
> > Or did I miss something?
> >     
>
>  Compilation is usually quite fast, but this is not always the case.  If 
> you look at the tests that do use `dg-timeout-factor' in GCC, and some 
> commits that added the setting, then you ought to find actual use cases.  
> I saw at least one such a test that takes an awful lot of time here on a 
> reasonably fast host machine and still passes where GCC has been built 
> with optimisation enabled, but does time out in the compilation phase if 
> the compiler has been built at -O0 for debugging purposes.  I'd have to 
> chase it though if you couldn't find it as I haven't written the name 
> down.
>
>  So yes, `dg-timeout-factor' does have its use, but it is different from 
> that of `dg-test-timeout-factor', hence the need for a separate setting.

This name has already caused confusion and the patch has not even been 
accepted yet.  The feature is desirable but this implementation is not 
acceptable.

At the moment, there are two blocking issues with this patch:

1.  The global variable name 'test_timeout_factor' is not acceptable 
because it has already caused confusion, apparently among GCC developers 
who should be familiar with the GCC testsuite.  If it already confuses 
GCC testsuite domain experts, its meaning is too unclear for general 
use.  While looking for alternative names, I found the fundamental 
problem with this proposed implementation:  test phases (such as running 
a test program versus running the tool itself) are defined by the 
testsuite, not by the framework.  DejaGnu therefore cannot explicitly 
support this as offered because the proposal violates encapsulation both 
ways.

2.  New code in DejaGnu using expr(n) is to have the expression braced 
as recommended in the expr(n) manpage, unless it actually uses the 
semantics provided by unbraced expr expressions, in which case it 
*needs* a comment explaining and justifying that.

The second issue is trivially fixable, but the first appears fatal.


There is a new "testcase" mulitplex command in Git master, which will be 
included in the next release, that is intended for testsuites to express 
dynamic state.  The original planned use was to support hierarchical 
test groups, for which a "testcase group" command is currently defined.  
In the future, dg.exp will be extended to use "testcase group" to 
delimit each testcase that it processes, and the framework will itself 
explicitly track each test script as a group.  (DejaGnu's current 
semantics implicitly group tests by test scripts, but only by (*.exp) 
scripts.)  Could this multiplex be a suitable place to put this API feature?

Using a command also has the advantage that it will cause a hard failure 
if the framework does not implement it, unlike a variable that a test 
script can set for the framework to silently ignore, leading to 
hard-to-reproduce test (timeout) failures if an older framework is used 
with a testsuite expecting this feature.  The semantics of "testcase 
patience" or similar would be defined to extend to the end of the group 
(or test script in versions of DejaGnu that do not fully implement 
groups) in which it is executed.  This limited scope is needed because 
allowing timeout scale factors to "bleed over" to the next test script 
would play havoc with the planned native parallel testing support, where 
the "next" script could have already started in another process.

I suggest a few possible commands off the top of my head:
   testcase ask patience WHAT FACTOR
   testcase declare patience WHAT FACTOR
   testcase patience WHAT FACTOR

The FACTOR is a scale factor, similar to the proposed 
'test_timeout_factor' or possibly the keyword "reset" (or special value 
0?) to clear a previous factor before leaving a group.  Multiple 
invocations stack:  the effective scale factor is the product of all 
applicable scale factors.  (This will have straightforward interactions 
with groups:  leaving a group will restore the scale factor in effect 
when the group was entered.  The initial scale factor at top-level is 1, 
for any WHAT.)

The WHAT is a keyword from a to-be-determined set.  There is a 
possibility that parts of the framework might eventually respond to 
certain WHAT values, but for now, would "dg-run" be suitable to express 
a timeout for running a test program and "dg-compile" for the timeout on 
running GCC itself?  This could lead to reserving dg-* WHAT values for 
dg.exp based testsuites to define, with a convention that dg-WHAT scales 
the timeout for "dg-do WHAT".

Leaving the definition of WHAT to the testsuite is not an insurmountable 
barrier, as providing an inquiry command for the testsuite to use would 
not be difficult.  This seems to lead towards a "testcase declare 
patience WHAT FACTOR" and "testcase inquire patience WHAT" pair.  The 
former multiplies the current WHAT scale factor by FACTOR, while the 
latter returns the appropriate running product.

All this provides a nice way to add upstream support for dg-patience ("{ 
dg-patience dg-run 3 }" or "{ dg-patience dg-compile 2 }") or a similar 
tag to dg.exp, but still leaves the issue of communicating /which/ scale 
factor to use to the various command execution procedures.  Here we come 
back to the same problem, since the current API shape (not changing 
anytime soon) does not provide a way to pass a timeout value or scale 
factor, other than using a "magic" variable.  So we are back to 
'timeout_scale_factor', but documented in the procedure documentation 
for the remote_* procedures.  In this case, the framework could use 
uplevel to read the variable as a local variable in the caller's frame, 
so the gcc-dg-test procedure would only need to do {set 
timeout_scale_factor [testcase inquire patience dg-run]} before using 
remote_load to run the test program.  (Expect does similar things, 
according to its manpage.)

The *_load procedures in the config/*.exp are not documented and 
config/README specifically says that they are to be called using the 
remote_* procedures.  While using a "magic" variable would require some 
neat tricks with uplevel/upvar, it should work as long as testsuites use 
the documented entrypoints.  (The *_load procedures from config/*.exp 
are likely to disappear into Tcl namespaces and/or parent interpreters 
in the future anyway.)

Comments before I start on an implementation?


-- Jacob