[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jacob Bachmeyer
Subject: Re: [PATCH 2/2] Move loaded_libs check into search_and_load_file
Date: Thu, 07 Mar 2019 23:07:57 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20090807 MultiZilla/ SeaMonkey/1.1.17 Mnenhy/

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]