Re: [PATCH 3/5] libxl: Generate golang bindings in libxl Makefile

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On May 26, 2020, at 2:53 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
> 
> George Dunlap writes ("[PATCH 3/5] libxl: Generate golang bindings in libxl 
> Makefile"):
>> +.PHONY: idl-external
>> +idl-external:
>> +    $(MAKE) -C $(XEN_ROOT)/tools/golang/xenlight idl-gen
> 
> Unfortunately this kind of thing is forbidden.  At least, without a
> rigorous proof that this isn't a concurrency hazard.
> 
> The problem is that with parallel make, the concurrency correctness
> principles are as follows:
> (1) different targets use nonoverlapping temporary and output files
>    (makefile authors' responsibiliy)
> (2) one invocation of make won't make the same target twice at the
>    same time (fundamental principle of operation for make)
> (3) the same makefile (or different makefiles with overlapping
>    targets) may not be entered multiple times in parallel
>    (build system authors' responsibility; preclucdes most use of
>    make -C to sibling directories rather than to children)
> 
> A correctness proof to make an exception would involve demonstrating
> that the tools/golang directories never touch this file when invoked
> as part of a recursive build.  NB, consider the clean targets too.

tools/golang/xenlight/Makefile:*.gen.go target will be triggered by 
xenlight/Makefile:idl-gen and xenlight/Makefile:build.

xenlight/Makefile:build is called from tools/golang/Makfile:subdirs-all, which 
is called from tools/Makefile:subdirs-all.

xenlight/Makefile:idl-gen is called from tools/libxl/Makefile:idl-external, 
which is called from tools/libxl/Makefile:all, which is called from 
tools/Makefile:subdirs-all.

tools/Makefile:subdirs-all is implemented as a non-parallel for loop executing 
over SUBDIRS-y; tools/golang comes after tools/libxl in that list, and so 
tools/golang:all will never be called until after tools/libxl:all has 
completed.  This invariant — that tools/golang/Makefile:all must not be called 
until tools/libxl/Makefile:all has completed must be kept regardless of this 
patch, since xenlight/Makefile:build depends on other output from 
tools/libxl/Makefile:all.

So as long as nothing else calls tools/libxl:all or tools/libxl:idl-external, 
this should be safe.  We could add a comments near xenlight/Makefile:idl-gen 
saying it must only be called from libxl/Makefile:idl-external; and to 
libxl/Makefile:idl-external saying it must not be called recursively from 
another Makefile.

> Alternatively, move the generated golang files to tools/libxl maybe,
> and perhaps leave symlinks behind.

Would that result in the files being accessible to the golang build tools at 
https://xenbits.xenproject.org/git-http/xen.git/tools/golang/xenlight ?  If 
not, it defeats the purpose of having the files checked into the tree.

> Or convert the whole (of tools/, maybe) to nonrecursive make using eg
> subdirmk :-).  https://diziet.dreamwidth.org/5763.html

This isn’t really a practical suggestion: I don’t have time to refactor the 
entire libxl Makefile tree, and certainly don’t have time by Friday.

 -George