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

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

[Jan Beulich]

Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
userspace test harnesses") didn't account for the dependencies of
cpuid-autogen.h to potentially change between incremental builds.
Putting the make invocation to produce the header together with the
directory tree creation therefore does not work. Introduce a separate
goal.

Furthermore the harness has a "run" goal which is supposed to be usable
independently of the rest of the tools sub-tree building, and both the
harness and the fuzzer code are also supposed to be buildable
independently. Therefore they need to recursivley invoke make to re-
build the generated header if needed.

Finally cpuid.o did not have any dependencies added for it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -26,13 +26,15 @@ GCOV_FLAGS := --coverage
 	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $< -o $@
 
 x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h)
+                     x86-vendors.h x86-defns.h msr-index.h) \
+         $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
+                     cpuid.h cpuid-autogen.h)
 x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h $(x86.h)
 
 # x86-emulate.c will be implicit for both
 x86-emulate.o x86-emulate-cov.o: x86_emulate/x86_emulate.c $(x86_emulate.h)
 
-fuzz-emul.o fuzz-emulate-cov.o wrappers.o: $(x86_emulate.h)
+fuzz-emul.o fuzz-emulate-cov.o cpuid.o wrappers.o: $(x86_emulate.h)
 
 x86-insn-fuzzer.a: fuzz-emul.o x86-emulate.o cpuid.o
 	$(AR) rc $@ $^
@@ -43,6 +45,9 @@ afl-harness: afl-harness.o fuzz-emul.o x
 afl-harness-cov: afl-harness-cov.o fuzz-emul-cov.o x86-emulate-cov.o cpuid.o wrappers.o
 	$(CC) $(CFLAGS) $(GCOV_FLAGS) $^ -o $@
 
+$(XEN_ROOT)/tools/include/xen/lib/x86/cpuid-autogen.h: FORCE
+	$(MAKE) -C $(XEN_ROOT)/tools/include build
+
 # Common targets
 .PHONY: all
 all: x86-insn-fuzz-all
@@ -60,6 +65,9 @@ install: all
 
 .PHONY: uninstall
 
+.PHONY: FORCE
+FORCE:
+
 .PHONY: afl
 afl: afl-harness
 
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -4,8 +4,9 @@ include $(XEN_ROOT)/tools/Rules.mk
 # Relative to $(XEN_ROOT)/xen/xsm/flask
 FLASK_H_DEPEND := policy/initial_sids
 
-.PHONY: all build
-all build: xen-foreign xen/.dir xen-xsm/.dir
+.PHONY: all all-y build
+all build: all-y xen-foreign xen/.dir xen-xsm/.dir
+all-y:
 
 .PHONY: xen-foreign
 xen-foreign:
@@ -27,10 +28,12 @@ ifeq ($(CONFIG_X86),y)
 	for f in $(filter-out %autogen.h,$(patsubst $(XEN_ROOT)/xen/include/xen/lib/x86/%,%,Makefile $(wildcard $(XEN_ROOT)/xen/include/xen/lib/x86/*.h))); do \
 		ln -sf $(XEN_ROOT)/xen/include/xen/lib/x86/$$f xen/lib/x86/$$f; \
 	done
-	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT)
 endif
 	touch $@
 
+all-$(CONFIG_X86): xen/.dir
+	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT)
+
 # Not xen/xsm as that clashes with link to
 # $(XEN_ROOT)/xen/include/public/xsm above.
 xen-xsm/.dir: $(XEN_ROOT)/xen/xsm/flask/policy/mkflask.sh \
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -11,6 +11,9 @@ all:
 run: $(TARGET)
 	./$(TARGET)
 
+.PHONY: FORCE
+FORCE:
+
 # Add libx86 to the build
 vpath %.c $(XEN_ROOT)/xen/lib/x86
 
@@ -208,13 +211,18 @@ $(call cc-option-add,HOSTCFLAGS-x86_64,H
 HOSTCFLAGS += $(CFLAGS_xeninclude) -I. $(HOSTCFLAGS-$(XEN_COMPILE_ARCH))
 
 x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h)
+                     x86-vendors.h x86-defns.h msr-index.h) \
+         $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
+                     cpuid.h cpuid-autogen.h)
 x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86-emulate.o test_x86_emulator.o evex-disp8.o wrappers.o: %.o: %.c $(x86_emulate.h)
+x86-emulate.o cpuid.o test_x86_emulator.o evex-disp8.o wrappers.o: %.o: %.c $(x86_emulate.h)
 	$(HOSTCC) $(HOSTCFLAGS) -c -g -o $@ $<
 
 x86-emulate.o: x86_emulate/x86_emulate.c
 x86-emulate.o: HOSTCFLAGS += -D__XEN_TOOLS__
 
 test_x86_emulator.o: $(addsuffix .h,$(TESTCASES)) $(addsuffix -opmask.h,$(OPMASK))
+
+$(XEN_ROOT)/tools/include/xen/lib/x86/cpuid-autogen.h: FORCE
+	$(MAKE) -C $(XEN_ROOT)/tools/include build




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

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

[Andrew Cooper]

On 14/12/2018 08:49, Jan Beulich wrote:
> Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
> userspace test harnesses") didn't account for the dependencies of
> cpuid-autogen.h to potentially change between incremental builds.
> Putting the make invocation to produce the header together with the
> directory tree creation therefore does not work. Introduce a separate
> goal.
>
> Furthermore the harness has a "run" goal which is supposed to be usable
> independently of the rest of the tools sub-tree building, and both the
> harness and the fuzzer code are also supposed to be buildable
> independently. Therefore they need to recursivley invoke make to re-
> build the generated header if needed.
>
> Finally cpuid.o did not have any dependencies added for it.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[Ian Jackson]

Jan Beulich writes ("[PATCH] x86emul: fix test harness and fuzzer build dependencies"):
> --- a/tools/fuzz/x86_instruction_emulator/Makefile
> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
...
> +$(XEN_ROOT)/tools/include/xen/lib/x86/cpuid-autogen.h: FORCE
> +	$(MAKE) -C $(XEN_ROOT)/tools/include build

I think this introduces a `make -j' hazard ?  The problem is that this
branch of the Makefiles tree might enter tools/include while
another branch is also doing so, resulting in two simultaneous
executions in the same directory.

As I have written before:

  With recursive make, it is necessary for the overall structure of the
  makefiles to sequence things so that each directory is entered exactly
  once, before its dependent directories are entered.  (It is possible
  to violate this rule without creating races but it is tricky and
  inadvisable.)

>  .PHONY: xen-foreign
>  xen-foreign:
> @@ -27,10 +28,12 @@ ifeq ($(CONFIG_X86),y)
>  	for f in $(filter-out %autogen.h,$(patsubst $(XEN_ROOT)/xen/include/xen/lib/x86/%,%,Makefile $(wildcard $(XEN_ROOT)/xen/include/xen/lib/x86/*.h))); do \
>  		ln -sf $(XEN_ROOT)/xen/include/xen/lib/x86/$$f xen/lib/x86/$$f; \
>  	done
> -	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT)
>  endif
>  	touch $@
>  
> +all-$(CONFIG_X86): xen/.dir
> +	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT)

And here we have a pre-existing instance of the same bug, I think ?

With recursive make you can't just
   $(MAKE) -C .../this
   $(MAKE) -C .../that
because of this race problem.  You need to sequence the subdirectories
correctly in the parent Makefiles so that by the time `here' is
entered, its dependency directories are already finished.

Ian.

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

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

[Jan Beulich]

>>> On 20.12.18 at 15:46, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("[PATCH] x86emul: fix test harness and fuzzer build 
> dependencies"):
>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
> ...
>> +$(XEN_ROOT)/tools/include/xen/lib/x86/cpuid-autogen.h: FORCE
>> +	$(MAKE) -C $(XEN_ROOT)/tools/include build
> 
> I think this introduces a `make -j' hazard ?  The problem is that this
> branch of the Makefiles tree might enter tools/include while
> another branch is also doing so, resulting in two simultaneous
> executions in the same directory.

What is "another branch" here? So far I was under the impression
that the ability of building x86 emulator fuzzer and test harness
independently is an exception, and that all other parts of the
tools/ subtree are supposed to be built by going through the top
level. Otherwise further dependency issues might arise, due to
top level Makefile's %-tools-public-headers rule.

Hence whether there's a "make -j" hazard here depends on what
that top level rule's purpose is.

>> @@ -27,10 +28,12 @@ ifeq ($(CONFIG_X86),y)
>>  	for f in $(filter-out %autogen.h,$(patsubst 
> $(XEN_ROOT)/xen/include/xen/lib/x86/%,%,Makefile $(wildcard 
> $(XEN_ROOT)/xen/include/xen/lib/x86/*.h))); do \
>>  		ln -sf $(XEN_ROOT)/xen/include/xen/lib/x86/$$f xen/lib/x86/$$f; \
>>  	done
>> -	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT)
>>  endif
>>  	touch $@
>>  
>> +all-$(CONFIG_X86): xen/.dir
>> +	$(MAKE) -C xen/lib/x86 all XEN_ROOT=$(XEN_ROOT)
> 
> And here we have a pre-existing instance of the same bug, I think ?
> 
> With recursive make you can't just
>    $(MAKE) -C .../this
>    $(MAKE) -C .../that
> because of this race problem.  You need to sequence the subdirectories
> correctly in the parent Makefiles so that by the time `here' is
> entered, its dependency directories are already finished.

In general - yes. But with the way the tools/include/ tree gets
populated, I don't think so.

Jan



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

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] x86emul: fix test harness and fuzzer build dependencies"):
> On 20.12.18 at 15:46, <ian.jackson@citrix.com> wrote:
> > I think this introduces a `make -j' hazard ?  The problem is that this
> > branch of the Makefiles tree might enter tools/include while
> > another branch is also doing so, resulting in two simultaneous
> > executions in the same directory.
> 
> What is "another branch" here? So far I was under the impression
> that the ability of building x86 emulator fuzzer and test harness
> independently is an exception, and that all other parts of the
> tools/ subtree are supposed to be built by going through the top
> level. Otherwise further dependency issues might arise, due to
> top level Makefile's %-tools-public-headers rule.
> 
> Hence whether there's a "make -j" hazard here depends on what
> that top level rule's purpose is.

I don't follow.

The top-level %-tools-public-headers rule is there to be something
that you can write in the dependencies of other subdirs, to arrange
that $(MAKE) -C tools/include is run before that other subdir.

Ie, it is there to satisfy the requirement I mention above, that the
dependency directory is built first.


If one wants to be build the x86 emulator fuzzer but not rebuild other
things, it is OK to run make in just that subdirectory.  That is a
general rule with recursive make: you can run make in a subdirectory,
at the cost of perhaps not rebuilding everything that was changed.

But when one does that, one must have built the rest of the things
already because otherwise the x86emul directory has to $(MAKE) -C back
to tools/include, rentering tools/include.

In the general case if one is changing things in different places and
rebuilding frequently, one may have to
   make -C place1 && make -C place2
in one's ad-hoc command line.

On that basis,

    On 14/12/2018 08:49, Jan Beulich wrote:
    > Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in
    > the userspace test harnesses") didn't account for the
    > dependencies of cpuid-autogen.h to potentially change between
    > incremental builds.  Putting the make invocation to produce the
    > header together with the directory tree creation therefore does
    > not work. Introduce a separate goal.

I think that may have been a misconceived attempt to improve on the
usual UI rule for recursive make, which I describe above.


I wrote:

   (It is possible to violate this rule without creating races but it
  is tricky and inadvisable.)

If we are determined that it must be possible to run make in the x86
emulator fuzzer directory *without having previously built the rest of
the tree normally*, then perhaps it is necessary to do this
$(MAKE) -C thing.

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.)

    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.)

 OR

 B. The tools/include Makefile gains explicit synchronisation; it
    would have to re-invoke itself.  I have never tries this, but it
    seems like it would be possible.  We would gain a new built-time
    dependency on a shell synchronisation/locking utility which would
    perhaps not be available on ten-year-old enterprise Linuces.

Ian.

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

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

[Jan Beulich]

>>> On 20.12.18 at 16:23, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH] x86emul: fix test harness and fuzzer build 
> dependencies"):
>> On 20.12.18 at 15:46, <ian.jackson@citrix.com> wrote:
>> > I think this introduces a `make -j' hazard ?  The problem is that this
>> > branch of the Makefiles tree might enter tools/include while
>> > another branch is also doing so, resulting in two simultaneous
>> > executions in the same directory.
>> 
>> What is "another branch" here? So far I was under the impression
>> that the ability of building x86 emulator fuzzer and test harness
>> independently is an exception, and that all other parts of the
>> tools/ subtree are supposed to be built by going through the top
>> level. Otherwise further dependency issues might arise, due to
>> top level Makefile's %-tools-public-headers rule.
>> 
>> Hence whether there's a "make -j" hazard here depends on what
>> that top level rule's purpose is.
> 
> I don't follow.
> 
> The top-level %-tools-public-headers rule is there to be something
> that you can write in the dependencies of other subdirs, to arrange
> that $(MAKE) -C tools/include is run before that other subdir.
> 
> 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.

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 wrote:
> 
>    (It is possible to violate this rule without creating races but it
>   is tricky and inadvisable.)
> 
> If we are determined that it must be possible to run make in the x86
> emulator fuzzer directory *without having previously built the rest of
> the tree normally*, then perhaps it is necessary to do this
> $(MAKE) -C thing.
> 
> 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.

>     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).

Bottom line - can you please be concrete as to what I need to change
in the patch (and in particular in the change to tools/include/Makefile,
as the other pieces are under x86 maintainership anyway) to get your
ack, without requiring me to address issues I don't (currently) care
about, and that have been there before?

Jan



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

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

[Jan Beulich]

>>> On 20.12.18 at 17:05, <JBeulich@suse.com> wrote:
>>>> On 20.12.18 at 16:23, <ian.jackson@citrix.com> wrote:
>> Jan Beulich writes ("Re: [PATCH] x86emul: fix test harness and fuzzer build 
>> dependencies"):
>>> On 20.12.18 at 15:46, <ian.jackson@citrix.com> wrote:
>>> > I think this introduces a `make -j' hazard ?  The problem is that this
>>> > branch of the Makefiles tree might enter tools/include while
>>> > another branch is also doing so, resulting in two simultaneous
>>> > executions in the same directory.
>>> 
>>> What is "another branch" here? So far I was under the impression
>>> that the ability of building x86 emulator fuzzer and test harness
>>> independently is an exception, and that all other parts of the
>>> tools/ subtree are supposed to be built by going through the top
>>> level. Otherwise further dependency issues might arise, due to
>>> top level Makefile's %-tools-public-headers rule.
>>> 
>>> Hence whether there's a "make -j" hazard here depends on what
>>> that top level rule's purpose is.
>> 
>> I don't follow.
>> 
>> The top-level %-tools-public-headers rule is there to be something
>> that you can write in the dependencies of other subdirs, to arrange
>> that $(MAKE) -C tools/include is run before that other subdir.
>> 
>> 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.
> 
> 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 wrote:
>> 
>>    (It is possible to violate this rule without creating races but it
>>   is tricky and inadvisable.)
>> 
>> If we are determined that it must be possible to run make in the x86
>> emulator fuzzer directory *without having previously built the rest of
>> the tree normally*, then perhaps it is necessary to do this
>> $(MAKE) -C thing.
>> 
>> 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.
> 
>>     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.

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?

Jan


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

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

[Ian Jackson]

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.

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.

> >> 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 ?

Ian.

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

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

[Jan Beulich]

>>> 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

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

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH] x86emul: fix test harness and fuzzer build dependencies"):
> On 21.12.18 at 15:16, <ian.jackson@citrix.com> wrote:
> > 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.

This interface intent is inconsistent with the design principles for
recursive make, and can not, in general, be realised.

It can in some special cases, including I think this one, be realised
using MAKELEVEL, but only at the cost of significant extra complexity.
I see you've sent another patch based on MAKELEVEL.  I don't have a
strong enough objection to this change in this one place to block it.

However, I worry is that this is setting a precedent.
What is wrong is the idea that in general it is OK in xen.git to
start with a clean tree, or modify an existing build tree,, run
   make -C some/direct/ory
and expect everything to be (re)built as needed.  The result of
trying to implement that everywhere via MAKELEVEL would be awful.

Can you promise me not to send any more patches based on using
MAKELEVEL this way ?  If not, where does it end ?  Noting that even
your updated patch does not work for (eg):
   make -C tools/fuzz

Sorry,
Ian.

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

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

[Jan Beulich]

>>> On 14.01.19 at 16:09, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH] x86emul: fix test harness and 
> fuzzer build dependencies"):
>> On 21.12.18 at 15:16, <ian.jackson@citrix.com> wrote:
>> > 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.
> 
> This interface intent is inconsistent with the design principles for
> recursive make, and can not, in general, be realised.
> 
> It can in some special cases, including I think this one, be realised
> using MAKELEVEL, but only at the cost of significant extra complexity.
> I see you've sent another patch based on MAKELEVEL.  I don't have a
> strong enough objection to this change in this one place to block it.

But from the rest of your reply it sounds as if you don't want to
see it go in either. I'm feeling left in the dark as to how to make
progress here.

> However, I worry is that this is setting a precedent.
> What is wrong is the idea that in general it is OK in xen.git to
> start with a clean tree, or modify an existing build tree,, run
>    make -C some/direct/ory
> and expect everything to be (re)built as needed.  The result of
> trying to implement that everywhere via MAKELEVEL would be awful.
> 
> Can you promise me not to send any more patches based on using
> MAKELEVEL this way ?

I don't intend to, but promise? No. For the fuzzer target I could
accept a doc change making the re-build of the include directory
a necessary prereq step. For the test harness'es "run" target,
though, I don't view this as a viable alternative. The name of the
goal is very clearly (to me at least) indicating that this is
(supposed to be) a standalone one. Any other harnesses
under tools/test/ may want to have something similar (some
already have a "run" target).

>  If not, where does it end ?  Noting that even
> your updated patch does not work for (eg):
>    make -C tools/fuzz

Of course it doesn't; it's not meant to be. I don't see how this
relates here though - it wouldn't work without the change here
either. I'm not even convinced "make -C tools" works, as
opposed to "make tools", due to the very prereq step to (re-)
make tools/include/.

So how do we make progress here? For the two changes that
you dislike I don't formally need your ack, and I have Andrew's.
I would (have to) respect an active NAK of yours, of course.

For the one change that I need your (or Wei's) ack on, I didn't
see any strong objection so far, and this fixes an actual issue
with the overall tools build, i.e. _outside_ of the area you're
concerned about. The $(MAKE) invocation there is not overly
nice, but I thought I did convince you that - with the way
tools/include/ gets dealt with from the top level - this should
not be an issue. Plus I'm just moving it.

Jan



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

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

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH] x86emul: fix test harness and fuzzer build dependencies"):
> So how do we make progress here? For the two changes that
> you dislike I don't formally need your ack, and I have Andrew's.
> I would (have to) respect an active NAK of yours, of course.

As I say, I think you are trying to something that is not well
supported by recursive make.  It follows that if any situations cause
particular trouble, they should be documented rather than fixed; or
maybe the general case should be documented too.  ("Only top-level
make targets are fully supported; you may use deeper targets directly
with make -C or by invoking make in a subdir, but at risk of
erroneously not (re)building other parts of the tree".)

But I don't think this is important enough for a NAK.

I'm not sure what you mean by "make progress".  Do you mean "how can
we dispose of this disagreement?"  I think our views are
irreconcilable.

If you and other committers have listened to me and still want to
commit this then so be it.

> For the one change that I need your (or Wei's) ack on, I didn't
> see any strong objection so far, and this fixes an actual issue
> with the overall tools build, i.e. _outside_ of the area you're
> concerned about. The $(MAKE) invocation there is not overly
> nice, but I thought I did convince you that - with the way
> tools/include/ gets dealt with from the top level - this should
> not be an issue. Plus I'm just moving it.

FAOD, the only thing I am objecting to is this kind of thing:

   +ifeq ($(MAKELEVEL),0)
   +$(XEN_ROOT)/tools/include/xen/lib/x86/cpuid-autogen.h: FORCE
   +       $(MAKE) -C $(XEN_ROOT)/tools/include build
   +endif

This appears twice in your v2.  The rest I have no difficulty with.

I don't think it is sensible to turn this into an argument about whose
bailiwick various hunks are in.  I am not going to cry foul if this
gets committed, particularly if Andrew has reviewed this conversation
and tells us that his ack stands.  I have said my piece.

HTH.

Ian.

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

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

[Jan Beulich]

>>> On 14.01.19 at 17:59, <ian.jackson@citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH] x86emul: fix test harness and 
> fuzzer build dependencies"):
>> So how do we make progress here? For the two changes that
>> you dislike I don't formally need your ack, and I have Andrew's.
>> I would (have to) respect an active NAK of yours, of course.
> 
> As I say, I think you are trying to something that is not well
> supported by recursive make.  It follows that if any situations cause
> particular trouble, they should be documented rather than fixed; or
> maybe the general case should be documented too.  ("Only top-level
> make targets are fully supported; you may use deeper targets directly
> with make -C or by invoking make in a subdir, but at risk of
> erroneously not (re)building other parts of the tree".)
> 
> But I don't think this is important enough for a NAK.
> 
> I'm not sure what you mean by "make progress".  Do you mean "how can
> we dispose of this disagreement?"  I think our views are
> irreconcilable.

Not really, no. I'd be fine to accept an alternative solution,
provided one can still invoke at least "run" targets on their own
(and without the disclaimer you propose above). A possible
solution could be to introduce wrapper targets in the top level
Makefile, properly sequencing the two steps from there. I didn't
suggest that because I'm not eager to see a proliferation of
new top level targets.

> If you and other committers have listened to me and still want to
> commit this then so be it.
> 
>> For the one change that I need your (or Wei's) ack on, I didn't
>> see any strong objection so far, and this fixes an actual issue
>> with the overall tools build, i.e. _outside_ of the area you're
>> concerned about. The $(MAKE) invocation there is not overly
>> nice, but I thought I did convince you that - with the way
>> tools/include/ gets dealt with from the top level - this should
>> not be an issue. Plus I'm just moving it.
> 
> FAOD, the only thing I am objecting to is this kind of thing:
> 
>    +ifeq ($(MAKELEVEL),0)
>    +$(XEN_ROOT)/tools/include/xen/lib/x86/cpuid-autogen.h: FORCE
>    +       $(MAKE) -C $(XEN_ROOT)/tools/include build
>    +endif
> 
> This appears twice in your v2.  The rest I have no difficulty with.
> 
> I don't think it is sensible to turn this into an argument about whose
> bailiwick various hunks are in.  I am not going to cry foul if this
> gets committed, particularly if Andrew has reviewed this conversation
> and tells us that his ack stands.  I have said my piece.

Thanks, this clarifies matters enough from a process perspective.
Nevertheless I'd prefer if I could commit something which you do
not disagree with. Therefore, rather than asking Andrew whether
his ack stands despite the discussion, I'd prefer if we could come
to a workable solution both of us can live with.

Jan



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