Re: [Xen-devel] [PATCH 4/4] DEPS handling: Remove absolute paths from references to cwd

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.17 at 18:46, <ian.jackson@xxxxxxxxxxxxx> wrote:
> In some directories we use gcc on source files elsewhere, to generate
> a .o here in the current directory.  Eg in tools/libxl/,
>    gcc -I -o build.o /path/to/libacpi/build.c
> We pass -MMD and -MF options to generate a .d file right here.
> 
> In the general case this .c file might need to include things from the
> directory here, eg libacpi/build.c eventually #includes various
> *libxl*.h.  We pass gcc -I. for this, which means things from the cwd
> where we invoked gcc, not the directory of the #including file.
> 
> When we do this, gcc's -MMD output mentions /path/to/libxl/*libxl*.h,
> even though it could refer to simply *libxl*.h.  This is presumably
> because gcc has noticed that `.' in this context must mean relative to
> the invocation cwd, not relative to build.c, and gcc doesn't realise
> that references in the .d file are also wrt the invocation cwd.
> 
> make distinguishes targets purely textually.  It will canonicalise a
> target name by removing ./ before comparison (so _libxl_types.h and
> ./_libxl_types.h are considered the same target) but it won't examine
> the filesystem.  So _libxl_types.h and
> /path/to/tools/libxl/_libxl_types.h are different targets.
> 
> And, _libxl_types.h is generated from a pattern rule.  This pattern
> rule is therefore instatiated twice, and the two instances may be run
> concurrently - but use the same tempfiles and can therefore fail.
> 
> The thing that is wrong here is gcc's choice to output an absolute
> path.
> 
> We could work around it by adding a rule to teach make about a
> relationship between these `two different files'.  But this has to be
> done for every autogenerated file and is therefore fragile (leaving a
> race bug when we get it wrong).
> 
> Ideally we would fix the problem by fixing the .d file as it is
> generated.  But the .d files are generated by many many rules
> mentioning $(CC) and $(CFLAGS).  (We might in theory pass a bash
> process substitution to -MF, but 1. that's not portable to people who
> don't have bash and 2. it hangs, anyway.)
> 
> So instead we do this conversion at include time.  That is, we tell
> make to include not the raw .d files, but the sedded ones.
> 
> The sedding removes occurrences of ` $PWD/'.  We use the shell
> variable PWD because the make variable sometimes refers to the xen
> toplevel.  If gcc's output format should change, then this sed rune
> may not work any more, but that doesn't seem very likely.
> 
> The rune is only effective for dependencies on files which are exactly
> in the current directory, or a subdirectory of it named simply by its
> subdirectory name.  If there are autogenerated include files which
> exist in a sibling (or worse, somewhere completely else), this
> approach will not work, because we'd have to figure out what name this
> Makefile usually uses to refer to them.  Hopefully such things don't
> exist.
> 
> The indirect variables DEPS_RM and DEPS_INCLUDE are necessary to
> preserve the assumptions made in the various Makefiles.  Specifically,
> xen/ Makefiles assume that it is ok to say DEPS+=something (where
> something is in a subdirectory); tools/ Makefiles all used to include
> DEPS themselves (but now they include DEPS_INCLUDE); and many
> Makefiles tended to explictly rm DEPS (but now rm DEPS_RM).
> 
> In the new scheme of things: DEPS is the files that come out of gcc
> (or perhaps an assembler or something) and may be assigned to by
> Makefiles.  DEPS_INCLUDE is the processed form.  And DEPS_RM is both
> combined, so that they both get cleaned.
> 
> We need to explicitly use $(wildcard ) to do the wildcard expansion on
> DEPS a bit earlier.  If we didn't, then DEPS_INCLUDE would contain
> `.*.d2' which would not exist.
> 
> Evaluation order: DEPS_RM and DEPS_INCLUDE are recursively expanded
> variables, so that although they are defined early (in Config.mk),
> their actual values are computed at the time of use, using the value
> of DEPS that is prevailing at that time.
> 
> Reported-by: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
(also extending to the non-tools parts of patches 1-3) with
one question:

> --- a/Config.mk
> +++ b/Config.mk
> @@ -56,8 +56,11 @@ HOSTCC ?= clang
>  HOSTCXX ?= clang++
>  endif
>  
> -DEPS_RM = $(DEPS)
> -DEPS_INCLUDE = $(DEPS)
> +DEPS_INCLUDE = $(addsuffix .d2, $(basename $(wildcard $(DEPS))))
> +DEPS_RM = $(DEPS) $(DEPS_INCLUDE)
> +
> +%.d2: %.d

Wouldn't it be better to use .%.d2 and .%.d here?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel