Re: [Qemu-devel] [PATCH for-2.12 2/2] make: switch from -I to -iquote

[Michael S. Tsirkin]

On Wed, Mar 21, 2018 at 06:40:23PM +0200, Marcel Apfelbaum wrote:
> On 21/03/2018 18:02, Michael S. Tsirkin wrote:
> > On Wed, Mar 21, 2018 at 03:36:51PM +0000, Daniel P. Berrangé wrote:
> >> On Wed, Mar 21, 2018 at 05:22:07PM +0200, Michael S. Tsirkin wrote:
> >>> Our rule right now is to use <> for external headers,
> >>> "" for internal ones. The idea was to avoid conflicts
> >>> between e.g. a system file named <trace.h> and an
> >>> internal one by the same name.
> >>>
> >>> Unfortunately we use -I compiler flag so it does not
> >>> help: a system file doing #include <trace.h> will
> >>> still pick up ours first.
> >>
> >> Has that actually hit you practice ?  I'm wondering if this
> >> is just theoretical in which case it could wait till 2.13
> >> since this scenario has existed in QEMU along time, or
> >> a real problem right now requiring fix in 2.12.
> > 
> > It's borderline.
> > 
> > In particular there is at least one instance where we already have a
> > conflict (yes, there are probably other ways to address this, but
> > I consider them less robust):
> > 
> > util/getauxval.c:#include <sys/auxv.h>
> > /usr/include/sys/auxv.h:#include <elf.h>
> > 
> > So it really does build by luck.
> > 
> > 
> >>> To fix, switch to -iquote which is supported by both
> >>> gcc and clang and only affects #include "" directives.
> >>
> >> Fine since we don't support anything other than
> >> gcc and clang. I'm assuming -iquote has been supported
> >> by these two compilers for a long time though ? The GCC
> >> docs annoyingly don't ever mention what release features
> >> appear in :-(
> > 
> > Judging by git log output:
> > 
> > For clang it appears to be there since 2007 that's pretty
> > close to day 1.
> > 
> > gcc's log is better documented, it's been there since 2004-05-03
> > 
> >>>
> >>> As a side effect, this catches any future uses of
> >>>  #include <> for internal headers.
> >>>
> >>> Suggested-by: Stefan Weil <address@hidden>
> >>> Signed-off-by: Michael S. Tsirkin <address@hidden>
> >>> ---
> >>
> >> Reviewed-by: Daniel P. Berrangé <address@hidden>
> > 
> > 
> > Thanks!
> > 
> 
> Hi Michael,
> Should I add this patch to the RDMA pull request?
> 
> Thanks,
> Marcel

Go ahead and add both of them pls.

> 
> >>
> >>>
> >>> I still think we want to switch to a more formal rule such as qemu/
> >>> prefix for all includes down the road, but this will at least catch any
> >>> scheme violations from creeping in meanwhile.
> >>>
> >>>
> >>>  configure       | 16 ++++++++--------
> >>>  rules.mak       |  2 +-
> >>>  Makefile.target |  4 ++--
> >>>  3 files changed, 11 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/configure b/configure
> >>> index af72fc8..23a4f3b 100755
> >>> --- a/configure
> >>> +++ b/configure
> >>> @@ -534,7 +534,7 @@ QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv 
> >>> $QEMU_CFLAGS"
> >>>  QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
> >>> $QEMU_CFLAGS"
> >>>  QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
> >>>  QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> >>> $QEMU_CFLAGS"
> >>> -QEMU_INCLUDES="-I. -I\$(SRC_PATH) -I\$(SRC_PATH)/accel/tcg 
> >>> -I\$(SRC_PATH)/include"
> >>> +QEMU_INCLUDES="-iquote . -iquote \$(SRC_PATH) -iquote 
> >>> \$(SRC_PATH)/accel/tcg -iquote \$(SRC_PATH)/include"
> >>>  if test "$debug_info" = "yes"; then
> >>>      CFLAGS="-g $CFLAGS"
> >>>      LDFLAGS="-g $LDFLAGS"
> >>> @@ -6560,19 +6560,19 @@ if test "$vxhs" = "yes" ; then
> >>>  fi
> >>>  
> >>>  if test "$tcg_interpreter" = "yes"; then
> >>> -  QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
> >>> +  QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
> >>>  elif test "$ARCH" = "sparc64" ; then
> >>> -  QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/sparc $QEMU_INCLUDES"
> >>> +  QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/sparc $QEMU_INCLUDES"
> >>>  elif test "$ARCH" = "s390x" ; then
> >>> -  QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/s390 $QEMU_INCLUDES"
> >>> +  QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/s390 $QEMU_INCLUDES"
> >>>  elif test "$ARCH" = "x86_64" -o "$ARCH" = "x32" ; then
> >>> -  QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/i386 $QEMU_INCLUDES"
> >>> +  QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/i386 $QEMU_INCLUDES"
> >>>  elif test "$ARCH" = "ppc64" ; then
> >>> -  QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/ppc $QEMU_INCLUDES"
> >>> +  QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/ppc $QEMU_INCLUDES"
> >>>  else
> >>> -  QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/\$(ARCH) $QEMU_INCLUDES"
> >>> +  QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/\$(ARCH) $QEMU_INCLUDES"
> >>>  fi
> >>> -QEMU_INCLUDES="-I\$(SRC_PATH)/tcg $QEMU_INCLUDES"
> >>> +QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg $QEMU_INCLUDES"
> >>>  
> >>>  echo "TOOLS=$tools" >> $config_host_mak
> >>>  echo "ROMS=$roms" >> $config_host_mak
> >>> diff --git a/rules.mak b/rules.mak
> >>> index 6e94333..93a0702 100644
> >>> --- a/rules.mak
> >>> +++ b/rules.mak
> >>> @@ -29,7 +29,7 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
> >>>  # dir, one absolute and the other relative to the compiler working
> >>>  # directory. These are the same for target-independent files, but
> >>>  # different for target-dependent ones.
> >>> -QEMU_LOCAL_INCLUDES = -I$(BUILD_DIR)/$(@D) -I$(@D)
> >>> +QEMU_LOCAL_INCLUDES = -iquote $(BUILD_DIR)/$(@D) -iquote $(@D)
> >>>  
> >>>  WL_U := -Wl,-u,
> >>>  find-symbols = $(if $1, $(sort $(shell $(NM) -P -g $1 | $2)))
> >>> diff --git a/Makefile.target b/Makefile.target
> >>> index 6549481..d0ec77a 100644
> >>> --- a/Makefile.target
> >>> +++ b/Makefile.target
> >>> @@ -11,9 +11,9 @@ $(call set-vpath, $(SRC_PATH):$(BUILD_DIR))
> >>>  ifdef CONFIG_LINUX
> >>>  QEMU_CFLAGS += -I../linux-headers
> >>>  endif
> >>> -QEMU_CFLAGS += -I.. -I$(SRC_PATH)/target/$(TARGET_BASE_ARCH) -DNEED_CPU_H
> >>> +QEMU_CFLAGS += -iquote .. -iquote $(SRC_PATH)/target/$(TARGET_BASE_ARCH) 
> >>> -DNEED_CPU_H
> >>>  
> >>> -QEMU_CFLAGS+=-I$(SRC_PATH)/include
> >>> +QEMU_CFLAGS+=-iquote $(SRC_PATH)/include
> >>>  
> >>>  ifdef CONFIG_USER_ONLY
> >>>  # user emulator name
> >>> -- 
> >>> MST
> >>>
> >>
> >> Regards,
> >> Daniel
> >> -- 
> >> |: https://berrange.com      -o-    
> >> https://www.flickr.com/photos/dberrange :|
> >> |: https://libvirt.org         -o-            
> >> https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-    
> >> https://www.instagram.com/dberrange :|