Re: [PATCH] x86emul: fix test harness and fuzzer build dependencies

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.12.18 at 15:16, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH] x86emul: fix test harness and 
> fuzzer build dependencies"):
>> On 20.12.18 at 17:05, <JBeulich@suse.com> wrote:
>>> On 20.12.18 at 16:23, <ian.jackson@citrix.com> wrote:
>> >> Ie, it is there to satisfy the requirement I mention above, that the
>> >> dependency directory is built first.
>> > 
>> > Which effectively means anything underneath tools/ (and whatever
>> > else subdir which depend on one of said rules) is liable to fail to
>> > build without having come through this (top level) rule.
> 
> Yes.
> 
>> > But this is not a property this patch introduces or changes. It just
>> > re-arranges how things get done. That is, I'd like to ask for the
>> > change to be acked (or a concrete proposal be made for what
>> > needs to change) _without_ fixing breakage that might be there
>> > and the introduction of which you may have missed, or else I'm
>> > sure you would have commented on what is now eddf9559c9.
> 
> I'm afraid I don't follow this line of argument.  The uncommitted
> patch we are now discussing introduce a new -j-unsafe call, which
> could cause stochastic build failures.

Well, depends on how you look at it: The patch moves such a
make invocation, which was simply misplaced by the original
change introducing it, and which would have - if properly
placed - introduced the -j issue right away.

> And AFAICT the only thing the patch-under-discussion improves is to
> relax (in some situations) the requirement to run
>   make -C ..../tools/include && make
> instead of just make, in x86emul.  That requirement is the one which I
> am saying is entirely normal when using recursive make so it should
> not be a surprise.
> 
> AFAICT eddf9559c9 doesn't seem to have this problem, although I could
> be wrong.

Not sure which of the problems "this problem" is. In any event
that change resulted in "make -C ..../tools/include" (if that
works as a standalone command in the first place, which I did
not try) _not_ having the intended effect during an incremental
invocation, i.e. after the tree (or merely the sub-tree) was
already built once.

>> >> But in that case we need to make sure that either:
>> >> 
>> >>  A. 1. The top-level Makefiles ensure that *a* build of
>> >>        tools/include completes *before* starting to enter
>> >>        tools/fuzz/x86_instruction_emulator.  (Which I
>> >>        think is the case.)
>> > 
>> > Yes, by said dependency on %-tools-public-headers.
> 
> Right.
> 
>> >>     AND
>> >> 
>> >>     2. make -C tools/include is definitely completely read-only if the
>> >>        thing has already been built.  (This is somewhat hard to check
>> >>        and maintain, and would need a comment in that Makefile to
>> >>        ensure that this property is preserved.)
>> > 
>> > Isn't that a property that's supposed to hold for almost all (sub)trees,
>> > i.e. it's rather the exception that an already built tree will see further
>> > changes when re-built incrementally without the sources having
>> > changed? That said, we have to consider such a case here, due to
>> > the use of move-if-changed in xen/include/xen/lib/x86/Makefile. But
>> > this still doesn't violate the fully-read-only requirement, as the rule's
>> > commands won't be executed again when the dependencies are
>> > older than the target (as is going to be the case after the invocation
>> > of the build from the top level Makefile).
>> 
>> So the conclusion was wrong here. If the dependencies changed
>> (as in are newer than the target) but the target file is the same as
>> before, move-if-changed will hide the effect from consumers of the
>> produced file, but the temporary file created gets in the way of -j
>> here.
> 
> Indeed.
> 
>> There being just one invocation of the build in tools/include/ prior
>> to the patch here (and hence no race afaict), would you consider
>> it reasonable to make the two new invocations dependent upon
>> $(MAKELEVEL), thus protecting things in the recursive case?
> 
> I don't think this is particularly nice, although I wouldn't block it.
> 
> Why is this particular inter-directory dependency unusual ?  Do we
> plan to introduce similar MAKELEVEL-based invocation of dependency
> directory makefiles everyhere ?

If need be, anywhere where independent building of the sub-tree
is specifically meant to work as a standalone operation. That
invocation of "make -C ..../tools/include && ..." is in particular not
meant to be necessary for the test harness'es "run" target. It is
also not intended to be used for the fuzzer builds (as per
tools/fuzz/README.afl), albeit there one might as well call it a doc
omission.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel