Re: [PATCH 2/2] Move loaded_libs check into search_and_load_file

[Jacob Bachmeyer]

Ben Elliston wrote:
> > The solution presented here has search_and_load_file take a callback
> > function, this allows the "has this file been loaded already?" check
> > to only be performed for the users that care, while also allowing
> > the check to be deferred until we know exactly which file we are
> > going to load.  I also include a check specifically to spot the case
> > where a tool init file is loaded as a library, and warn about it.
> >     
>
> Any comments on this, Jacob?
>   
I will write this as an answer to Ben's question; this would have been a 
reply to the original patch instead if Ben had not asked that question.  
I thought that this would be short, then I started writing.

I have several immediate issues:  (The more trivial issues are listed 
first.)

First, I found a typo:

verbose "filter_load_once: $filename (loadeding)" 2


Second, the proposed patch changes the calling sequence for 
search_and_load_file incompatibly.  Any other callers outside of the 
DejaGnu core will break.  Since "filter_load_always" is the existing 
behavior, it should also be the default value for that argument.  This 
avoids having to change most call sites and simplifies the patch.

Third, the filter_load_* procedures are ending up in the global 
namespace.  I am reluctant to add symbols to the global namespace in 
DejaGnu simply because we already have quite a mess there.  Adding 
procedures will, of course, cease to be a concern after the DejaGnu core 
is eventually moved into Tcl namespaces, but right now, every procedure 
in the global namespace is potentially an API entrypoint.  I really do 
not want to add more API symbols to further complicate an eventual 
namespace move if they can be avoided, and here they can be avoided.

Fourth, I believe that this would be the first quasi-use of callbacks in 
DejaGnu and I would recommend against adding that idiom to the code base 
at this time.  I say "quasi-use" because Tcl does not actually have 
functions as first-class objects ("everything is a string"), so the 
filter function is actually being passed by name.  This is asking for 
trouble with a later move to namespaces when someone inevitably has a 
custom filter function.  This is a customization point that creates 
future maintenance burden greater than its utility.

Fifth, this is incomplete:  the patch does not use [file normalize] and 
even [file normalize] does not detect hard links or normalize symlinks 
directly to files.  This looks like a half-solution to me that will 
cause further confusion later when someone else manages to break it.  
The complaint will be that "it says right there that it will only load a 
file once".  (If we really wanted to be thorough, we could compute a 
digest of every file before sourcing it and only source the file if we 
have not seen that digest before, but this is getting overcomplicated 
fast.)  I argue that it is better to omit a feature than to 
half-implement it.  The existing multiple-inclusion guard is specified 
in terms of the requested file name in the program source, so it is 
fully implemented.

Sixth, this detects a high-level bug in low-level code.  This puts a 
very particular special case, meaningful at a high level, into not only 
the lowest layer, but into a callback from that layer that is named to 
express a generic policy.  A generic "filter_load_once" should not care 
about why the file is being loaded, only whether it has been loaded 
before.  Arguably, if filter_load_once cares about $type at all, it 
should be indexing loaded_libs by the logical tuple ($type,$filename), 
which puts us right back where we started, albeit with a possibly 
slightly more elegant structure in loaded_libs.  This is a large patch 
for a small and questionable gain.

Seventh, I am concerned that this may be taking a high-level feature 
(the multiple-inclusion guard for library modules) and pushing it into 
lower level code, with a subtle change to the semantics:  the 
multiple-inclusion guard is now applied to the name of the file being 
read rather than to the name under which the file was requested.  I do 
not know if any existing testsuites would be sensitive to this change, 
but it *is* a change to the load_lib API call.

Eighth, arguably the old behavior of counting the tool init file as a 
library module was a bug in DejaGnu -- it does not make logical sense, 
although even I did not see this when I wrote the patch that changed 
that, thus recording it as "tool/$file" instead of omitting it from 
loaded_libs entirely.

Ninth, considering the motivation for this proposed patch, this feels 
like swatting a fly with a thermonuclear ICBM strike.  It is a 
significant change to the program organization (in an area that is 
likely to be particularly sensitive to changes, as it is also at the 
bottom of a call chain that is entered from board description files) to 
detect a bug that has so far been unique.  This would be different if 
many testsuites were "double-loading" the tool init files and breaking, 
but the earlier change only exposed a bug in the GDB testsuite.


The most trivial issues have easy solutions, but the later issues are worse.

The first issue is a typo.  s/loadeding/loading/

The second issue mentions its own solution:  add a default value for the 
new argument.  I have used this approach in other places where I have 
wanted to add new behavior to existing procedures in DejaGnu.

The third and fourth issues have a simple solution:  how many different 
useful policies for loading files are there?  Likely very few -- DejaGnu 
currently only needs two of them.  Instead of a callback, pass a POLICY 
argument that is a simple string "load_always" or "load_once", with a 
default value of "load_always".  For the call sites that need load_once 
semantics, load_once can be passed as exactly that, with no need for 
quotes.  A simple switch(n) handles selecting a policy from its name, 
with all of them inline in the search_and_load_file procedure.

The fifth issue can be partially resolved at the cost of a significant 
amount of code.

The sixth issue can be partially resolved by moving the check to 
load_lib itself, eliminating most of the proposed new code and making 
most of these issues moot.  While this seems to require recording what 
tool init file was actually loaded somewhere, there is a trick that we 
could use:  the tool init file can only be mistaken for a library file 
if it is in testsuite/lib and the tool init file can only be in 
testsuite/lib if it is not in testsuite/lib/tool -- and this search path 
is part of the stable API for testsuites.  This means that load_lib 
could complain if $file is ${tool}.exp and 
$testsuitedir/lib/tool/${tool}.exp does not exist.  This could produce 
spurious warnings if an actual library module exists as ${tool}.exp 
somewhere else on the library search path, but no existing testsuite can 
have such a library module -- the previous bug mentioned in the eighth 
issue would have caused the loading of the tool init file to inhibit 
that library module.  It can also misfire if the testsuite changes the 
"tool" variable, but that should be an obvious bad idea and would 
clearly be a bug in that testsuite.

The seventh issue is a major problem, possibly blocking all by itself:  
this patch subtly changes API behavior.

The eighth issue reinforces that this is about bugs in testsuites rather 
than DejaGnu itself.  While producing useful diagnostic messages is a 
good idea, we should be cautious about increasing future maintenance 
burdens to do so for rare cases.

The seventh and ninth issues are the big problem:  this is a low-level 
change with uncertain consequences for future maintenance to work around 
a bug in another project that has already been fixed.  Detecting similar 
bugs elsewhere is an admirable goal, but I think that it might simply be 
better to document this caveat somewhere in the manual, perhaps the node 
for the load_lib procedure.  (There is an entire printed page worth of 
text that I added to the manual explaining Expect's priority rules and 
the caveats they produce after I found the same (Expect-misuse-related) 
bug lurking in DejaGnu's testsuite that I had independently introduced 
and fixed on another project.)  If this issue bites, load_lib will be in 
the backtrace, and a warning produced in load_lib ("WARNING:  Possible 
attempt to load tool init file as library module; see manual.") should 
be enough to point the way for the user.  The warning can always be 
removed if we find that it has too many spurious hits, but I think the 
heuristic I described for the sixth issue will be adequate.


Overall, I vote NAK on this patch in its current form.


-- Jacob