Re: GDB testsuite overrides default_target_compile and breaks

[Jacob Bachmeyer]

Tom Tromey wrote:
> Jacob> What is needed in brief:
> Jacob> (1) Merge the features of default_target_compile that the GDB
> Jacob> testsuite depends on upstream for 1.6.3.
> Jacob> (2) Find the actual extensibility that GDB needs here and add that
> Jacob> support to the default_target_compile rewrite slated for 1.6.4.
>
> Sounds reasonable to me.
>
> Jacob> The changes were to an internal component and broke an out-of-tree
> Jacob> copy of same.  I have to draw a line somewhere, and "monkeypatching
> Jacob> the core is not supported and can break" is necessary for DejaGnu to
> Jacob> develop at all.
>
> FWIW this makes sense to me.  I think it's never been a great idea to
> have this override in gdb.  Dejagnu wasn't very actively maintained for
> a long time, and had low engagement from gdb developers, so it was more
> like a convenient way to move forward.
>   

From how it was explained to me, that may have been the only way 
forward at the time.

> Jacob> Monkeypatching the DejaGnu core is not supported and cannot be
> Jacob> supported long-term.
>
> gdb does it other spots as well.  For example, check-test-names.exp
> overrides log_summary and reset_vars that gdb can show duplicated test
> names, and gdb.exp renames "cd" to avoid problems with relative log file
> names.
>   

I will look into these and see what we can do to move generally useful 
functionality upstream.  (There are many features in the GDB testsuite 
that I eventually want in DejaGnu, like parallel testing, although the 
core implementation of that might be somewhat different as I want it to 
be as close to transparent as possible.)

Quick checks on those examples suggest that they will not cause problems 
(until the overridden procedures disappear into an internal namespace) 
because check-test-names.exp wraps log_summary and reset_vars instead of 
replacing them with out-of-tree copies.  Wrapping "cd" avoids problems 
for similar reasons (technically that one is monkeypatching Tcl itself 
rather than DejaGnu) but DejaGnu's own initialization procedures should 
probably ensure that the log file is active with an absolute file name.

I might have to add a "monkeypatch the monkeypatches" module, loaded 
after the init files, to maintain backwards compatibility with the 
recent GDB releases, but that is a last resort.

> Jacob> GDB seems to have also extended default_target_compile, so a
> Jacob> discussion on this is needed because there seems to be a need for some
> Jacob> kind of language extensibility feature to allow new
> Jacob> language-default-selector options to be defined without overriding the
> Jacob> entire default_target_compile procedure
>
> Yes, that would be good to have.  I think this has been patched when a
> new language is added to gdb; changing this function to work in some
> extensible way would be a nice improvement... while there are no
> concrete plans I know of to add new languages to gdb, one never knows
> when someone may show up with patches.
>   

The rewritten default_target_compile will be less of an "if forest" and 
more table-driven.  Language defaults then become entries in an internal 
table, so all we need is an API call for init files to add entries to 
that table.

> Jacob> If I understand correctly, merging the features currently carried in
> Jacob> gdb/testsuite/lib/future.exp upstream will cause all existing GDB 
> Jacob> releases to correctly use the upstream version of
> Jacob> default_target_compile, making the partial reversion patch
> Jacob> unnecessary.
>
> Yes, assuming that gdb's detection code continues to work.  It does
> this:
>
>     if {[info procs find_gnatmake] == ""} {
>         rename gdb_find_gnatmake find_gnatmake
>         set use_gdb_compile 1
>     }
>     # ... sequence of such checks ...
>     if {$use_gdb_compile} {
>         catch {rename default_target_compile {}}
>         rename gdb_default_target_compile default_target_compile
>     }
>   

That code is testing the existence of API procedures to decide if it 
wants to override an internal procedure.  There are no plans to remove 
find_* prior to 2.0; even if a better API ("testsuite tool"?) is 
introduced, compatibility shims will remain.  (Now that I think about 
it, even 2.0 may still carry a "compatibility mode" for the 1.x API.)

> Anyway, I compared gdb_default_target_compile with
> default_target_compile.  I think there are two kinds of changes.
>
> [...]
>
> I can write some patches to merge this stuff in.
>   

Many thanks for the patch set.  I will merge them and add documentation 
and tests over the weekend or so.


-- Jacob