[PATCH 0/2] kvmtool: allow CFLAGS and LDFLAGS override

[PATCH 0/2] kvmtool: allow CFLAGS and LDFLAGS override

[Andre Przywara]

In the past there have been some complaints from cross-compilation
users about missing libraries and include files. Fixing those issues
by pointing make to the proper directories via CFLAGS and LDFLAGS
proved to be complicated.
This series fixes this, so custom CFLAGS and LDFLAGS can be given on
the make command line and those will not overwrite kvmtool's vital
internal assignments.

Cheers,
Andre.

Andre Przywara (2):
  Makefile: allow overriding CFLAGS on the command line
  Makefile: consider LDFLAGS on feature tests and when linking executables

 Makefile | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

-- 
2.5.1

[PATCH 1/2] Makefile: allow overriding CFLAGS on the command line

[Andre Przywara]

When a Makefile variable is set on the make command line, all
Makefile-internal assignments to that very variable are _ignored_.
Since we add quite some essential values to CFLAGS internally,
specifying some CFLAGS on the command line will usually break the
build (and not fix any include file problems you hoped to overcome
with that).
Somewhat against intuition GNU make provides the "override" directive
to change this behavior; with that assignments in the Makefile get
_appended_ to the value given on the command line. [1]

Change any internal assignments to use that directive, so that a user
can use:
$ make CFLAGS=/path/to/my/include/dir
to teach kvmtool about non-standard header file locations (helpful
for cross-compilation) or to tweak other compiler options.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>

[1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html
---
 Makefile | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index f8f7cc4..77a7c9f 100644
--- a/Makefile
+++ b/Makefile
@@ -15,9 +15,7 @@ include config/utilities.mak
 include config/feature-tests.mak
 
 CC	:= $(CROSS_COMPILE)gcc
-CFLAGS	:=
 LD	:= $(CROSS_COMPILE)ld
-LDFLAGS	:=
 
 FIND	:= find
 CSCOPE	:= cscope
@@ -162,7 +160,7 @@ ifeq ($(ARCH), arm)
 	OBJS		+= arm/aarch32/kvm-cpu.o
 	ARCH_INCLUDE	:= $(HDRS_ARM_COMMON)
 	ARCH_INCLUDE	+= -Iarm/aarch32/include
-	CFLAGS		+= -march=armv7-a
+	override CFLAGS	+= -march=armv7-a
 
 	ARCH_WANT_LIBFDT := y
 endif
@@ -274,12 +272,12 @@ endif
 ifeq ($(LTO),1)
 	FLAGS_LTO := -flto
 	ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y)
-		CFLAGS		+= $(FLAGS_LTO)
+		override CFLAGS	+= $(FLAGS_LTO)
 	endif
 endif
 
 ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y)
-	CFLAGS		+= -DCONFIG_GUEST_INIT
+	override CFLAGS	+= -DCONFIG_GUEST_INIT
 	GUEST_INIT	:= guest/init
 	GUEST_OBJS	= guest/guest_init.o
 	ifeq ($(ARCH_PRE_INIT),)
@@ -331,7 +329,8 @@ DEFINES	+= -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
 DEFINES	+= -DBUILD_ARCH='"$(ARCH)"'
 
 KVM_INCLUDE := include
-CFLAGS	+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
+override CFLAGS	+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE)
+override CFLAGS	+= -O2 -fno-strict-aliasing -g
 
 WARNINGS += -Wall
 WARNINGS += -Wformat=2
@@ -349,10 +348,10 @@ WARNINGS += -Wvolatile-register-var
 WARNINGS += -Wwrite-strings
 WARNINGS += -Wno-format-nonliteral
 
-CFLAGS	+= $(WARNINGS)
+override CFLAGS	+= $(WARNINGS)
 
 ifneq ($(WERROR),0)
-	CFLAGS += -Werror
+	override CFLAGS += -Werror
 endif
 
 all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT)
-- 
2.5.1

[PATCH 2/2] Makefile: consider LDFLAGS on feature tests and when linking executables

[Andre Przywara]

While we have an LDFLAGS variable in kvmtool's Makefile, it's not
really used when both doing the feature tests and when finally linking
the lkvm executable.
Add that variable to all the linking steps to allow the user to
specify custom library directories or linker options on the command
line.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Makefile | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/Makefile b/Makefile
index 77a7c9f..eac1220 100644
--- a/Makefile
+++ b/Makefile
@@ -196,12 +196,12 @@ endif
 # On a given system, some libs may link statically, some may not; so, check
 # both and only build those that link!
 
-ifeq ($(call try-build,$(SOURCE_STRLCPY),$(CFLAGS),),y)
+ifeq ($(call try-build,$(SOURCE_STRLCPY),$(CFLAGS),$(LDFLAGS)),y)
 	CFLAGS_DYNOPT	+= -DHAVE_STRLCPY
 	CFLAGS_STATOPT	+= -DHAVE_STRLCPY
 endif
 
-ifeq ($(call try-build,$(SOURCE_BFD),$(CFLAGS),-lbfd -static),y)
+ifeq ($(call try-build,$(SOURCE_BFD),$(CFLAGS),$(LDFLAGS) -lbfd -static),y)
 	CFLAGS_STATOPT	+= -DCONFIG_HAS_BFD
 	OBJS_STATOPT	+= symbol.o
 	LIBS_STATOPT	+= -lbfd
@@ -212,7 +212,7 @@ endif
 ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
 	CFLAGS_GTK3 := $(shell pkg-config --cflags gtk+-3.0 2>/dev/null)
 	LDFLAGS_GTK3 := $(shell pkg-config --libs gtk+-3.0 2>/dev/null)
-	ifeq ($(call try-build,$(SOURCE_GTK3),$(CFLAGS) $(CFLAGS_GTK3),$(LDFLAGS_GTK3)),y)
+	ifeq ($(call try-build,$(SOURCE_GTK3),$(CFLAGS) $(CFLAGS_GTK3),$(LDFLAGS) $(LDFLAGS_GTK3)),y)
 		OBJS_DYNOPT	+= ui/gtk3.o
 		CFLAGS_DYNOPT	+= -DCONFIG_HAS_GTK3 $(CFLAGS_GTK3)
 		LIBS_DYNOPT	+= $(LDFLAGS_GTK3)
@@ -220,63 +220,63 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
 		NOTFOUND	+= GTK3
 	endif
 
-	ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),-lvncserver),y)
+	ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),$(LDFLAGS) -lvncserver),y)
 		OBJS_DYNOPT	+= ui/vnc.o
 		CFLAGS_DYNOPT	+= -DCONFIG_HAS_VNCSERVER
 		LIBS_DYNOPT	+= -lvncserver
 	else
 		NOTFOUND	+= vncserver
 	endif
-	ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),-lvncserver -static),y)
+	ifeq ($(call try-build,$(SOURCE_VNCSERVER),$(CFLAGS),$(LDFLAGS) -lvncserver -static),y)
 		OBJS_STATOPT	+= ui/vnc.o
 		CFLAGS_STATOPT	+= -DCONFIG_HAS_VNCSERVER
 		LIBS_STATOPT	+= -lvncserver
 	endif
 
-	ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),-lSDL),y)
+	ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),$(LDFLAGS) -lSDL),y)
 		OBJS_DYNOPT	+= ui/sdl.o
 		CFLAGS_DYNOPT	+= -DCONFIG_HAS_SDL
 		LIBS_DYNOPT	+= -lSDL
 	else
 		NOTFOUND	+= SDL
 	endif
-	ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),-lSDL -static), y)
+	ifeq ($(call try-build,$(SOURCE_SDL),$(CFLAGS),$(LDFLAGS) -lSDL -static), y)
 		OBJS_STATOPT	+= ui/sdl.o
 		CFLAGS_STATOPT	+= -DCONFIG_HAS_SDL
 		LIBS_STATOPT	+= -lSDL
 	endif
 endif
 
-ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),-lz),y)
+ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
 	CFLAGS_DYNOPT	+= -DCONFIG_HAS_ZLIB
 	LIBS_DYNOPT	+= -lz
 else
 	NOTFOUND	+= zlib
 endif
-ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),-lz -static),y)
+ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz -static),y)
 	CFLAGS_STATOPT	+= -DCONFIG_HAS_ZLIB
 	LIBS_STATOPT	+= -lz
 endif
 
-ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),-laio),y)
+ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y)
 	CFLAGS_DYNOPT	+= -DCONFIG_HAS_AIO
 	LIBS_DYNOPT	+= -laio
 else
 	NOTFOUND	+= aio
 endif
-ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),-laio -static),y)
+ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y)
 	CFLAGS_STATOPT	+= -DCONFIG_HAS_AIO
 	LIBS_STATOPT	+= -laio
 endif
 
 ifeq ($(LTO),1)
 	FLAGS_LTO := -flto
-	ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y)
+	ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(LDFLAGS) $(FLAGS_LTO)),y)
 		override CFLAGS	+= $(FLAGS_LTO)
 	endif
 endif
 
-ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y)
+ifeq ($(call try-build,$(SOURCE_STATIC),$(CFLAGS),$(LDFLAGS) -static),y)
 	override CFLAGS	+= -DCONFIG_GUEST_INIT
 	GUEST_INIT	:= guest/init
 	GUEST_OBJS	= guest/guest_init.o
@@ -370,11 +370,11 @@ STATIC_OBJS = $(patsubst %.o,%.static.o,$(OBJS) $(OBJS_STATOPT))
 
 $(PROGRAM)-static:  $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT) $(GUEST_PRE_INIT)
 	$(E) "  LINK    " $@
-	$(Q) $(CC) -static $(CFLAGS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_STATOPT) -o $@
+	$(Q) $(CC) -static $(CFLAGS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_OBJS) $(LDFLAGS) $(LIBS) $(LIBS_STATOPT) -o $@
 
 $(PROGRAM): $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_INIT) $(GUEST_PRE_INIT)
 	$(E) "  LINK    " $@
-	$(Q) $(CC) $(CFLAGS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_DYNOPT) -o $@
+	$(Q) $(CC) $(CFLAGS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS) $(LDFLAGS) $(LIBS) $(LIBS_DYNOPT) -o $@
 
 $(PROGRAM_ALIAS): $(PROGRAM)
 	$(E) "  LN      " $@
-- 
2.5.1

Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line

[Riku Voipio]

On 30 October 2015 at 19:20, Andre Przywara <andre.przywara@arm.com> wrote:
> When a Makefile variable is set on the make command line, all
> Makefile-internal assignments to that very variable are _ignored_.
> Since we add quite some essential values to CFLAGS internally,
> specifying some CFLAGS on the command line will usually break the
> build (and not fix any include file problems you hoped to overcome
> with that).
> Somewhat against intuition GNU make provides the "override" directive
> to change this behavior; with that assignments in the Makefile get
> _appended_ to the value given on the command line. [1]
>
> Change any internal assignments to use that directive, so that a user
> can use:
> $ make CFLAGS=/path/to/my/include/dir
> to teach kvmtool about non-standard header file locations (helpful
> for cross-compilation) or to tweak other compiler options.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
> [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html
> ---
>  Makefile | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f8f7cc4..77a7c9f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,9 +15,7 @@ include config/utilities.mak
>  include config/feature-tests.mak
>
>  CC     := $(CROSS_COMPILE)gcc
> -CFLAGS :=
>  LD     := $(CROSS_COMPILE)ld
> -LDFLAGS        :=

This breaks builds of debian packages as dpkg-buildpackage sets LDFLAGS
to something unsuitable for guest init.

Looks like this has been an issue before:

commit 57fa349a9792a629e4ed2d89e1309cc96dcc39af
Author: Will Deacon <will.deacon@arm.com>
Date:   Thu Jun 4 16:25:36 2015 +0100

    Don't inherit CFLAGS and LDFLAGS from the environment

    kvmtool doesn't build with arbitrary flags, so let's clear CFLAGS and
    LDFLAGS by default at the top of the Makefile, allowing people to add
    additional options there if they really want to.

    Reported by Dave Jones, who ended up passing -std=gnu99 by mistake.

I think it's better to have EXTRA_CFLAGS and EXTRA_LDFLAGS like the kernel
has.

>  FIND   := find
>  CSCOPE := cscope
> @@ -162,7 +160,7 @@ ifeq ($(ARCH), arm)
>         OBJS            += arm/aarch32/kvm-cpu.o
>         ARCH_INCLUDE    := $(HDRS_ARM_COMMON)
>         ARCH_INCLUDE    += -Iarm/aarch32/include
> -       CFLAGS          += -march=armv7-a
> +       override CFLAGS += -march=armv7-a
>
>         ARCH_WANT_LIBFDT := y
>  endif
> @@ -274,12 +272,12 @@ endif
>  ifeq ($(LTO),1)
>         FLAGS_LTO := -flto
>         ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y)
> -               CFLAGS          += $(FLAGS_LTO)
> +               override CFLAGS += $(FLAGS_LTO)
>         endif
>  endif
>
>  ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y)
> -       CFLAGS          += -DCONFIG_GUEST_INIT
> +       override CFLAGS += -DCONFIG_GUEST_INIT
>         GUEST_INIT      := guest/init
>         GUEST_OBJS      = guest/guest_init.o
>         ifeq ($(ARCH_PRE_INIT),)
> @@ -331,7 +329,8 @@ DEFINES     += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
>  DEFINES        += -DBUILD_ARCH='"$(ARCH)"'
>
>  KVM_INCLUDE := include
> -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
> +override CFLAGS        += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE)
> +override CFLAGS        += -O2 -fno-strict-aliasing -g
>
>  WARNINGS += -Wall
>  WARNINGS += -Wformat=2
> @@ -349,10 +348,10 @@ WARNINGS += -Wvolatile-register-var
>  WARNINGS += -Wwrite-strings
>  WARNINGS += -Wno-format-nonliteral
>
> -CFLAGS += $(WARNINGS)
> +override CFLAGS        += $(WARNINGS)
>
>  ifneq ($(WERROR),0)
> -       CFLAGS += -Werror
> +       override CFLAGS += -Werror
>  endif
>
>  all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT)
> --
> 2.5.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line

[Andre Przywara]

Hi Riku,

On 04/11/15 10:02, Riku Voipio wrote:
> On 30 October 2015 at 19:20, Andre Przywara <andre.przywara@arm.com> wrote:
>> When a Makefile variable is set on the make command line, all
>> Makefile-internal assignments to that very variable are _ignored_.
>> Since we add quite some essential values to CFLAGS internally,
>> specifying some CFLAGS on the command line will usually break the
>> build (and not fix any include file problems you hoped to overcome
>> with that).
>> Somewhat against intuition GNU make provides the "override" directive
>> to change this behavior; with that assignments in the Makefile get
>> _appended_ to the value given on the command line. [1]
>>
>> Change any internal assignments to use that directive, so that a user
>> can use:
>> $ make CFLAGS=/path/to/my/include/dir
>> to teach kvmtool about non-standard header file locations (helpful
>> for cross-compilation) or to tweak other compiler options.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>
>> [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html
>> ---
>>  Makefile | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index f8f7cc4..77a7c9f 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -15,9 +15,7 @@ include config/utilities.mak
>>  include config/feature-tests.mak
>>
>>  CC     := $(CROSS_COMPILE)gcc
>> -CFLAGS :=
>>  LD     := $(CROSS_COMPILE)ld
>> -LDFLAGS        :=
> 
> This breaks builds of debian packages as dpkg-buildpackage sets LDFLAGS
> to something unsuitable for guest init.
> 
> Looks like this has been an issue before:

> 
> commit 57fa349a9792a629e4ed2d89e1309cc96dcc39af
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Thu Jun 4 16:25:36 2015 +0100
> 
>     Don't inherit CFLAGS and LDFLAGS from the environment
> 
>     kvmtool doesn't build with arbitrary flags, so let's clear CFLAGS and
>     LDFLAGS by default at the top of the Makefile, allowing people to add
>     additional options there if they really want to.
> 
>     Reported by Dave Jones, who ended up passing -std=gnu99 by mistake.

Well, I fixed this issue later with making kvmtool compilation more
robust when using modern compiler standards.
That's why I wanted this kludge to go away.

> I think it's better to have EXTRA_CFLAGS and EXTRA_LDFLAGS like the kernel
> has.

Mmmh, I'd rather see guest_init creation use their own flags for it,
since it is so special and actually independent from the target userland.
Let me check this out and send out my guest_init Makefile fix I have
lying around here on the way.

What LDFLAGS are actually causing your issues?

Cheers,
Andre.

> 
>>  FIND   := find
>>  CSCOPE := cscope
>> @@ -162,7 +160,7 @@ ifeq ($(ARCH), arm)
>>         OBJS            += arm/aarch32/kvm-cpu.o
>>         ARCH_INCLUDE    := $(HDRS_ARM_COMMON)
>>         ARCH_INCLUDE    += -Iarm/aarch32/include
>> -       CFLAGS          += -march=armv7-a
>> +       override CFLAGS += -march=armv7-a
>>
>>         ARCH_WANT_LIBFDT := y
>>  endif
>> @@ -274,12 +272,12 @@ endif
>>  ifeq ($(LTO),1)
>>         FLAGS_LTO := -flto
>>         ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y)
>> -               CFLAGS          += $(FLAGS_LTO)
>> +               override CFLAGS += $(FLAGS_LTO)
>>         endif
>>  endif
>>
>>  ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y)
>> -       CFLAGS          += -DCONFIG_GUEST_INIT
>> +       override CFLAGS += -DCONFIG_GUEST_INIT
>>         GUEST_INIT      := guest/init
>>         GUEST_OBJS      = guest/guest_init.o
>>         ifeq ($(ARCH_PRE_INIT),)
>> @@ -331,7 +329,8 @@ DEFINES     += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
>>  DEFINES        += -DBUILD_ARCH='"$(ARCH)"'
>>
>>  KVM_INCLUDE := include
>> -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
>> +override CFLAGS        += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE)
>> +override CFLAGS        += -O2 -fno-strict-aliasing -g
>>
>>  WARNINGS += -Wall
>>  WARNINGS += -Wformat=2
>> @@ -349,10 +348,10 @@ WARNINGS += -Wvolatile-register-var
>>  WARNINGS += -Wwrite-strings
>>  WARNINGS += -Wno-format-nonliteral
>>
>> -CFLAGS += $(WARNINGS)
>> +override CFLAGS        += $(WARNINGS)
>>
>>  ifneq ($(WERROR),0)
>> -       CFLAGS += -Werror
>> +       override CFLAGS += -Werror
>>  endif
>>
>>  all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT)
>> --
>> 2.5.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line

[Riku Voipio]

On 4 November 2015 at 12:13, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Riku,
>
> On 04/11/15 10:02, Riku Voipio wrote:
>> On 30 October 2015 at 19:20, Andre Przywara <andre.przywara@arm.com> wrote:
>>> When a Makefile variable is set on the make command line, all
>>> Makefile-internal assignments to that very variable are _ignored_.
>>> Since we add quite some essential values to CFLAGS internally,
>>> specifying some CFLAGS on the command line will usually break the
>>> build (and not fix any include file problems you hoped to overcome
>>> with that).
>>> Somewhat against intuition GNU make provides the "override" directive
>>> to change this behavior; with that assignments in the Makefile get
>>> _appended_ to the value given on the command line. [1]
>>>
>>> Change any internal assignments to use that directive, so that a user
>>> can use:
>>> $ make CFLAGS=/path/to/my/include/dir
>>> to teach kvmtool about non-standard header file locations (helpful
>>> for cross-compilation) or to tweak other compiler options.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html
>>> ---
>>>  Makefile | 15 +++++++--------
>>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index f8f7cc4..77a7c9f 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -15,9 +15,7 @@ include config/utilities.mak
>>>  include config/feature-tests.mak
>>>
>>>  CC     := $(CROSS_COMPILE)gcc
>>> -CFLAGS :=
>>>  LD     := $(CROSS_COMPILE)ld
>>> -LDFLAGS        :=
>>
>> This breaks builds of debian packages as dpkg-buildpackage sets LDFLAGS
>> to something unsuitable for guest init.
>>
>> Looks like this has been an issue before:
>
>>
>> commit 57fa349a9792a629e4ed2d89e1309cc96dcc39af
>> Author: Will Deacon <will.deacon@arm.com>
>> Date:   Thu Jun 4 16:25:36 2015 +0100
>>
>>     Don't inherit CFLAGS and LDFLAGS from the environment
>>
>>     kvmtool doesn't build with arbitrary flags, so let's clear CFLAGS and
>>     LDFLAGS by default at the top of the Makefile, allowing people to add
>>     additional options there if they really want to.
>>
>>     Reported by Dave Jones, who ended up passing -std=gnu99 by mistake.
>
> Well, I fixed this issue later with making kvmtool compilation more
> robust when using modern compiler standards.
> That's why I wanted this kludge to go away.
>
>> I think it's better to have EXTRA_CFLAGS and EXTRA_LDFLAGS like the kernel
>> has.
>
> Mmmh, I'd rather see guest_init creation use their own flags for it,
> since it is so special and actually independent from the target userland.
> Let me check this out and send out my guest_init Makefile fix I have
> lying around here on the way.
>
> What LDFLAGS are actually causing your issues?

  LINK     guest/init
ld: unrecognized option '-Wl,-z,relro'

That's another issue - kvmtool passes LDFLAGS to both LD and CC yet they
actually take different command line options.

> Cheers,
> Andre.
>
>>
>>>  FIND   := find
>>>  CSCOPE := cscope
>>> @@ -162,7 +160,7 @@ ifeq ($(ARCH), arm)
>>>         OBJS            += arm/aarch32/kvm-cpu.o
>>>         ARCH_INCLUDE    := $(HDRS_ARM_COMMON)
>>>         ARCH_INCLUDE    += -Iarm/aarch32/include
>>> -       CFLAGS          += -march=armv7-a
>>> +       override CFLAGS += -march=armv7-a
>>>
>>>         ARCH_WANT_LIBFDT := y
>>>  endif
>>> @@ -274,12 +272,12 @@ endif
>>>  ifeq ($(LTO),1)
>>>         FLAGS_LTO := -flto
>>>         ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y)
>>> -               CFLAGS          += $(FLAGS_LTO)
>>> +               override CFLAGS += $(FLAGS_LTO)
>>>         endif
>>>  endif
>>>
>>>  ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y)
>>> -       CFLAGS          += -DCONFIG_GUEST_INIT
>>> +       override CFLAGS += -DCONFIG_GUEST_INIT
>>>         GUEST_INIT      := guest/init
>>>         GUEST_OBJS      = guest/guest_init.o
>>>         ifeq ($(ARCH_PRE_INIT),)
>>> @@ -331,7 +329,8 @@ DEFINES     += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
>>>  DEFINES        += -DBUILD_ARCH='"$(ARCH)"'
>>>
>>>  KVM_INCLUDE := include
>>> -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
>>> +override CFLAGS        += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE)
>>> +override CFLAGS        += -O2 -fno-strict-aliasing -g
>>>
>>>  WARNINGS += -Wall
>>>  WARNINGS += -Wformat=2
>>> @@ -349,10 +348,10 @@ WARNINGS += -Wvolatile-register-var
>>>  WARNINGS += -Wwrite-strings
>>>  WARNINGS += -Wno-format-nonliteral
>>>
>>> -CFLAGS += $(WARNINGS)
>>> +override CFLAGS        += $(WARNINGS)
>>>
>>>  ifneq ($(WERROR),0)
>>> -       CFLAGS += -Werror
>>> +       override CFLAGS += -Werror
>>>  endif
>>>
>>>  all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT)
>>> --
>>> 2.5.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line

[Will Deacon]

On Wed, Nov 04, 2015 at 12:02:23PM +0200, Riku Voipio wrote:
> On 30 October 2015 at 19:20, Andre Przywara <andre.przywara@arm.com> wrote:
> > When a Makefile variable is set on the make command line, all
> > Makefile-internal assignments to that very variable are _ignored_.
> > Since we add quite some essential values to CFLAGS internally,
> > specifying some CFLAGS on the command line will usually break the
> > build (and not fix any include file problems you hoped to overcome
> > with that).
> > Somewhat against intuition GNU make provides the "override" directive
> > to change this behavior; with that assignments in the Makefile get
> > _appended_ to the value given on the command line. [1]
> >
> > Change any internal assignments to use that directive, so that a user
> > can use:
> > $ make CFLAGS=/path/to/my/include/dir
> > to teach kvmtool about non-standard header file locations (helpful
> > for cross-compilation) or to tweak other compiler options.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >
> > [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html
> > ---
> >  Makefile | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index f8f7cc4..77a7c9f 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -15,9 +15,7 @@ include config/utilities.mak
> >  include config/feature-tests.mak
> >
> >  CC     := $(CROSS_COMPILE)gcc
> > -CFLAGS :=
> >  LD     := $(CROSS_COMPILE)ld
> > -LDFLAGS        :=
> 
> This breaks builds of debian packages as dpkg-buildpackage sets LDFLAGS
> to something unsuitable for guest init.

Thanks for the report, Riku. I'll revert this patch while we rethink how
to support user-supplied toolchain flags.

Will