libperf: Don't remove -g when EXTRA_CFLAGS are used

[PATCH] libperf: Don't remove -g when EXTRA_CFLAGS are used

Posted by James Clark 9 months ago

When using EXTRA_CFLAGS, for example "EXTRA_CFLAGS=-DREFCNT_CHECKING=1",
this construct stops setting -g which you'd expect would not be affected
by adding extra flags. Additionally, EXTRA_CFLAGS should be the last
thing to be appended so that it can be used to undo any defaults. And no
condition is required, just += appends to any existing CFLAGS and also
appends or doesn't append EXTRA_CFLAGS if they are or aren't set.

It's not clear why DEBUG=1 is required for -g in Perf when in libperf
it's always on, but I don't think we need to change that behavior now
because someone may be depending on it.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/lib/perf/Makefile | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/tools/lib/perf/Makefile b/tools/lib/perf/Makefile
index 3a9b2140aa04..478fe57bf8ce 100644
--- a/tools/lib/perf/Makefile
+++ b/tools/lib/perf/Makefile
@@ -54,13 +54,6 @@ endif
 
 TEST_ARGS := $(if $(V),-v)
 
-# Set compile option CFLAGS
-ifdef EXTRA_CFLAGS
-  CFLAGS := $(EXTRA_CFLAGS)
-else
-  CFLAGS := -g -Wall
-endif
-
 INCLUDES = \
 -I$(srctree)/tools/lib/perf/include \
 -I$(srctree)/tools/lib/ \
@@ -70,11 +63,12 @@ INCLUDES = \
 -I$(srctree)/tools/include/uapi
 
 # Append required CFLAGS
-override CFLAGS += $(EXTRA_WARNINGS)
-override CFLAGS += -Werror -Wall
+override CFLAGS += -g -Werror -Wall
 override CFLAGS += -fPIC
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
+override CFLAGS += $(EXTRA_WARNINGS)
+override CFLAGS += $(EXTRA_CFLAGS)
 
 all:
 
-- 
2.34.1

Re: [PATCH] libperf: Don't remove -g when EXTRA_CFLAGS are used

Posted by Namhyung Kim 8 months, 4 weeks ago

On Wed, 19 Mar 2025 11:40:09 +0000, James Clark wrote:
> When using EXTRA_CFLAGS, for example "EXTRA_CFLAGS=-DREFCNT_CHECKING=1",
> this construct stops setting -g which you'd expect would not be affected
> by adding extra flags. Additionally, EXTRA_CFLAGS should be the last
> thing to be appended so that it can be used to undo any defaults. And no
> condition is required, just += appends to any existing CFLAGS and also
> appends or doesn't append EXTRA_CFLAGS if they are or aren't set.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung

Re: [PATCH] libperf: Don't remove -g when EXTRA_CFLAGS are used

Posted by Ian Rogers 9 months ago

On Wed, Mar 19, 2025 at 4:40 AM James Clark <james.clark@linaro.org> wrote:
>
> When using EXTRA_CFLAGS, for example "EXTRA_CFLAGS=-DREFCNT_CHECKING=1",
> this construct stops setting -g which you'd expect would not be affected
> by adding extra flags. Additionally, EXTRA_CFLAGS should be the last
> thing to be appended so that it can be used to undo any defaults. And no
> condition is required, just += appends to any existing CFLAGS and also
> appends or doesn't append EXTRA_CFLAGS if they are or aren't set.
>
> It's not clear why DEBUG=1 is required for -g in Perf when in libperf
> it's always on, but I don't think we need to change that behavior now
> because someone may be depending on it.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/lib/perf/Makefile | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/tools/lib/perf/Makefile b/tools/lib/perf/Makefile
> index 3a9b2140aa04..478fe57bf8ce 100644
> --- a/tools/lib/perf/Makefile
> +++ b/tools/lib/perf/Makefile
> @@ -54,13 +54,6 @@ endif
>
>  TEST_ARGS := $(if $(V),-v)
>
> -# Set compile option CFLAGS
> -ifdef EXTRA_CFLAGS
> -  CFLAGS := $(EXTRA_CFLAGS)
> -else
> -  CFLAGS := -g -Wall
> -endif

I suspect this was cargo culted from:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/bpf/Makefile?h=perf-tools-next#n81
I'm not clear why it was ever that way, but perhaps a wider cleanup is
necessary than just this one patch.

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

>  INCLUDES = \
>  -I$(srctree)/tools/lib/perf/include \
>  -I$(srctree)/tools/lib/ \
> @@ -70,11 +63,12 @@ INCLUDES = \
>  -I$(srctree)/tools/include/uapi
>
>  # Append required CFLAGS
> -override CFLAGS += $(EXTRA_WARNINGS)
> -override CFLAGS += -Werror -Wall
> +override CFLAGS += -g -Werror -Wall
>  override CFLAGS += -fPIC
>  override CFLAGS += $(INCLUDES)
>  override CFLAGS += -fvisibility=hidden
> +override CFLAGS += $(EXTRA_WARNINGS)
> +override CFLAGS += $(EXTRA_CFLAGS)
>
>  all:
>
> --
> 2.34.1
>