Re: [Qemu-devel] [PATCH v3 1/8] make: move top level dir to end of include search path

[Daniel P. Berrange]

On Tue, Jan 24, 2017 at 02:11:29PM -0600, Eric Blake wrote:
> On 01/24/2017 05:01 AM, Daniel P. Berrange wrote:
> > Currently the search path is
> > 
> >   1. source dir corresponding to input file (implicit by compiler)
> >   2. top level build dir
> >   3. top level source dir
> >   4. top level source include/ dir
> >   5. source dir corresponding to input file
> >   6. build dir corresponding to output file
> > 
> > This causes a semantic difference in behaviour for builds
> > where srcdir == builddir vs srcdir != builddir, because
> > item 5 moves from end to start, when srcdir == builddir.
> 
> Rather, item 5 is a no-op (because it duplicated 1), and item 6 moves
> from the end to the beginning when srcdir == builddir

Yes

> > As a general rule we also want to move to have all shared
> > headers in the include/ dir, so move that ahead of the
> > top level dirs in the search order.
> 
> Wait - are you proposing that you swap 4 to occur earlier than 2/3?...

Sigh, left over from an earlier version of this patch. I was trying
todo a more general cleanup as described here, but decided to cut
back to the bare minimum I needed for trace work.

> > Thus we now have:
> > 
> >   1. source dir corresponding to input file
> >   2. build dir corresponding to output file
> >   3. top level build dir
> >   4. top level source dir
> >   5. top level source include/ dir
> 
> ...because this doesn't match that swap, and I don't see it in the patch
> body (but I may have missed it; I'm not as strong at reviewing make as I
> am at C)

Yeah, you're right.

> > 
> > and items 1+2 and 4+5 collapse into a single dir when srcdir==builddir
> 
> Isn't that items 3+4 (not 4+5) that collapse?

Yes, typo.

> > so overall order remains the same.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  rules.mak | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> 
> > 
> > diff --git a/rules.mak b/rules.mak
> > index d5c516c..e09aabe 100644
> > --- a/rules.mak
> > +++ b/rules.mak
> > @@ -26,8 +26,10 @@ QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out 
> > -Wstrict-prototypes -Wmissing
> >  # Flags for dependency generation
> >  QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
> >  
> > -# Same as -I$(SRC_PATH) -I., but for the nested source/object directories
> > -QEMU_INCLUDES += -I$(<D) -I$(@D)
> 
> In particular, this is the old code for 5 and 6,
> 
> > +# Compiler searches the source file dir first, but in vpath builds
> > +# we need to make it search the build dir too, before any other
> > +# explicit search paths.
> > +QEMU_LOCAL_INCLUDES = -I$(BUILD_DIR)/$(@D)
> 
> while this is the new code for 2, plus documentation that 1 is implicit.

So there's a subtle difference I didn't explain, and which I'm going to
tweak again.

 -I$(@D)

is a relative include. eg for a file 'migration/ram.o' it will resolve
to '-Imigration' relative to the build dir. Except there's a subtle
difference between target-dependent and target-independent files. For
example, migration/migration.o will be built in $BUILD_DIR/migration
but migration/ram.o will be built in $BUILD_DIR/x86_64-softmmu/migration
so this reslative include points to two different places potentially.

Hence, I changed to -I$(BUILD_DIR)/$(@D), so it is gauranteed to
point to $BUILD_DIR/migration, even for target-dependant files.

In retrospect, I think it is more correct to include both directories
ie  -I$(BUILD_DIR)/$(@D) and -I$(@D). Even if not technically needed
by this patch series I think its clearer.

> >  WL_U := -Wl,-u,
> >  find-symbols = $(if $1, $(sort $(shell $(NM) -P -g $1 | $2)))
> > @@ -61,7 +63,7 @@ expand-objs = $(strip $(sort $(filter %.o,$1)) \
> >                    $(filter-out %.o %.mo,$1))
> >  
> >  %.o: %.c
> > -   $(call quiet-command,$(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) 
> > $(QEMU_DGFLAGS) $(CFLAGS) $(address@hidden) -c -o $@ 
> > $<,"CC","$(TARGET_DIR)$@")
> > +   $(call quiet-command,$(CC) $(QEMU_LOCAL_INCLUDES) $(QEMU_INCLUDES) 
> > $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) $(address@hidden) -c -o $@ 
> > $<,"CC","$(TARGET_DIR)$@")
> 
> And the pre-pending of QEMU_LOCAL_INCLUDES is what changes the position
> of the local directory from last to first, thus delaying the top level
> dir to the end, but I don't see top/include/ moving.
> 
> These are now some long lines; is it worth taking the time to add \ line
> splitting for legibility, either in this patch or as an add-on?
> 
> > @@ -359,6 +361,7 @@ define unnest-vars
> >                  $(eval $(o:%.mo=%$(DSOSUF)): module-common.o $($o-objs)),
> >                  $(error $o added in $v but $o-objs is not set)))
> >          $(shell mkdir -p ./ $(sort $(dir $($v))))
> > +        $(shell cd $(BUILD_DIR) && mkdir -p ./ $(sort $(dir $($v))))
> >          # Include all the .d files
> 
> Okay, this change makes sense (make sure all the build directories exist
> in time; no-op for in-tree build, but helpful for VPATH), but seems
> unrelated to the commit message.  Rebase snafu?

The existing mkdir line there ensures that the -I$(@D) always points to a
directory that exists.

The new mkdir line does the same for -I$(BUILD_DIR)/$(@D).

This is to deal with fact that the compiler warns if you give a -I directory
that does not exist

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|