[PATCH] mk: add support for gdb debug info generation

[PATCH] mk: add support for gdb debug info generation

[Cyril Chemparathy]

From: Cyril Chemparathy <cchemparathy@tilera.com>

It is often useful to build with debug enabled, we add a config
(CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.

Note: This patch does not include corresponding changes for ICC.  The
author pleads abject ignorance in this regard, and welcomes
recommendations. :-)

Change-Id: I499e591e1b7d71df751fd40d1fdcbe6975eeeb27
Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 mk/toolchain/gcc/rte.vars.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
index 0f51c66..22c4c1f 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -71,6 +71,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS)))
 endif
 endif
 
+ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
+TOOLCHAIN_CFLAGS += -g -ggdb
+TOOLCHAIN_LDFLAGS += -g -ggdb
+endif
+
 WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes
 WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith
 WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual
-- 
2.1.2

[PATCH] examples/l2fwd: Add forward count limit.

[Cyril Chemparathy]

This commit adds a forward count argument, which is used to terminate
the test once a certain number of packets have been forwarded.

Change-Id: Ia3e7ff5d41c3e947509b0653d53271b882fc04de
Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 examples/l2fwd/main.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
index 720fd5a..20c1dd2 100644
--- a/examples/l2fwd/main.c
+++ b/examples/l2fwd/main.c
@@ -137,7 +137,9 @@ struct l2fwd_port_statistics port_statistics[RTE_MAX_ETHPORTS];
 /* A tsc-based timer responsible for triggering statistics printout */
 #define TIMER_MILLISECOND 2000000ULL /* around 1ms at 2 Ghz */
 #define MAX_TIMER_PERIOD 86400 /* 1 day max */
+#define MAX_FORWARD_COUNT 1000000000 /* a cool billion :) */
 static int64_t timer_period = 10 * TIMER_MILLISECOND * 1000; /* default period is 10 seconds */
+static int64_t forward_count;
 
 /* Print out statistics on packets dropped */
 static void
@@ -262,6 +264,7 @@ l2fwd_main_loop(void)
 	unsigned i, j, portid, nb_rx;
 	struct lcore_queue_conf *qconf;
 	const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) / US_PER_S * BURST_TX_DRAIN_US;
+	uint64_t total_packets_tx;
 
 	prev_tsc = 0;
 	timer_tsc = 0;
@@ -321,6 +324,17 @@ l2fwd_main_loop(void)
 			}
 
 			prev_tsc = cur_tsc;
+
+			if (forward_count > 0) {
+				total_packets_tx = 0;
+				for (portid = 0; portid < RTE_MAX_ETHPORTS; portid++) {
+					/* skip disabled ports */
+					if ((l2fwd_enabled_port_mask & (1 << portid)) != 0)
+						total_packets_tx += port_statistics[portid].tx;
+				}
+				if (total_packets_tx >= forward_count)
+					break;
+			}
 		}
 
 		/*
@@ -357,7 +371,8 @@ l2fwd_usage(const char *prgname)
 	printf("%s [EAL options] -- -p PORTMASK [-q NQ]\n"
 	       "  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
 	       "  -q NQ: number of queue (=ports) per lcore (default is 1)\n"
-		   "  -T PERIOD: statistics will be refreshed each PERIOD seconds (0 to disable, 10 default, 86400 maximum)\n",
+	       "  -T PERIOD: statistics will be refreshed each PERIOD seconds (0 to disable, 10 default, 86400 maximum)\n"
+	       "  -C COUNT: exit after transmitting COUNT packets\n",
 	       prgname);
 }
 
@@ -412,6 +427,22 @@ l2fwd_parse_timer_period(const char *q_arg)
 	return n;
 }
 
+static int
+l2fwd_parse_forward_count(const char *q_arg)
+{
+	char *end = NULL;
+	int n;
+
+	/* parse number string */
+	n = strtol(q_arg, &end, 10);
+	if ((q_arg[0] == '\0') || (end == NULL) || (*end != '\0'))
+		return -1;
+	if (n >= MAX_FORWARD_COUNT)
+		return -1;
+
+	return n;
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 l2fwd_parse_args(int argc, char **argv)
@@ -426,7 +457,7 @@ l2fwd_parse_args(int argc, char **argv)
 
 	argvopt = argv;
 
-	while ((opt = getopt_long(argc, argvopt, "p:q:T:",
+	while ((opt = getopt_long(argc, argvopt, "p:q:T:C:",
 				  lgopts, &option_index)) != EOF) {
 
 		switch (opt) {
@@ -460,6 +491,16 @@ l2fwd_parse_args(int argc, char **argv)
 			}
 			break;
 
+		/* forward count */
+		case 'C':
+			forward_count = l2fwd_parse_forward_count(optarg);
+			if (forward_count < 0) {
+				printf("invalid forward count\n");
+				l2fwd_usage(prgname);
+				return -1;
+			}
+			break;
+
 		/* long options */
 		case 0:
 			l2fwd_usage(prgname);
@@ -702,6 +743,7 @@ main(int argc, char **argv)
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			return -1;
 	}
+	print_stats();
 
 	return 0;
 }
-- 
2.1.2

Re: [PATCH] examples/l2fwd: Add forward count limit.

[Thomas Monjalon]

NACK

2015-06-19 14:29, Cyril Chemparathy:
> This commit adds a forward count argument, which is used to terminate
> the test once a certain number of packets have been forwarded.

It is not a test but an example application.
This patch doesn't demonstrate a DPDK feature.
If you need to improve a test tool, please prefer testpmd.

Re: [PATCH] mk: add support for gdb debug info generation

[Gonzalez Monroy, Sergio]

On 19/06/2015 22:29, Cyril Chemparathy wrote:
> From: Cyril Chemparathy <cchemparathy@tilera.com>
>
> It is often useful to build with debug enabled, we add a config
> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
>
> Note: This patch does not include corresponding changes for ICC.  The
> author pleads abject ignorance in this regard, and welcomes
> recommendations. :-)
>
> Change-Id: I499e591e1b7d71df751fd40d1fdcbe6975eeeb27
> Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
> ---
>   mk/toolchain/gcc/rte.vars.mk | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
> index 0f51c66..22c4c1f 100644
> --- a/mk/toolchain/gcc/rte.vars.mk
> +++ b/mk/toolchain/gcc/rte.vars.mk
> @@ -71,6 +71,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS)))
>   endif
>   endif
>   
> +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
> +TOOLCHAIN_CFLAGS += -g -ggdb
> +TOOLCHAIN_LDFLAGS += -g -ggdb
> +endif
> +
>   WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes
>   WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith
>   WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual
I don't think you need to modify the makefiles and introduce a new 
compile time option for this.
The same result can be easily achieved by setting EXTRA_CFLAGS in the 
command line. ie:
     $ make install T=x86_64-native-linuxapp-gcc EXTRA_CFLAGS='-g -ggdb'

Sergio

Re: [PATCH] mk: add support for gdb debug info generation

[Simon Kågström]

On 2015-06-22 09:44, Gonzalez Monroy, Sergio wrote:
> On 19/06/2015 22:29, Cyril Chemparathy wrote:
>> From: Cyril Chemparathy <cchemparathy@tilera.com>
>>
>> It is often useful to build with debug enabled, we add a config
>> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
>>
>>   +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
>> +TOOLCHAIN_CFLAGS += -g -ggdb
>> +TOOLCHAIN_LDFLAGS += -g -ggdb
>> +endif

> I don't think you need to modify the makefiles and introduce a new
> compile time option for this.
> The same result can be easily achieved by setting EXTRA_CFLAGS in the
> command line. ie:
>     $ make install T=x86_64-native-linuxapp-gcc EXTRA_CFLAGS='-g -ggdb'

Why isn't -g standard though? The binaries should/will anyhow be
stripped when used for production - but debugging information should be
useful when analysing crashes.

// Simon

Re: [PATCH] mk: add support for gdb debug info generation

[Gonzalez Monroy, Sergio]

On 22/06/2015 08:56, Simon Kågström wrote:
> On 2015-06-22 09:44, Gonzalez Monroy, Sergio wrote:
>> On 19/06/2015 22:29, Cyril Chemparathy wrote:
>>> From: Cyril Chemparathy <cchemparathy@tilera.com>
>>>
>>> It is often useful to build with debug enabled, we add a config
>>> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
>>>
>>>    +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
>>> +TOOLCHAIN_CFLAGS += -g -ggdb
>>> +TOOLCHAIN_LDFLAGS += -g -ggdb
>>> +endif
>> I don't think you need to modify the makefiles and introduce a new
>> compile time option for this.
>> The same result can be easily achieved by setting EXTRA_CFLAGS in the
>> command line. ie:
>>      $ make install T=x86_64-native-linuxapp-gcc EXTRA_CFLAGS='-g -ggdb'
> Why isn't -g standard though? The binaries should/will anyhow be
> stripped when used for production - but debugging information should be
> useful when analysing crashes.
I guess you could argue that, to always build with debug info then strip 
it down.
You would need another flag to strip debug info for production, or leave 
it for debugging.

In my opinion is not worth it, but it you feel strongly about it you can 
submit patches and
let the community decide.

Sergio
> // Simon
>

Re: [PATCH] mk: add support for gdb debug info generation

[Thomas Monjalon]

2015-06-23 08:39, Gonzalez Monroy, Sergio:
> On 22/06/2015 08:56, Simon Kågström wrote:
> > On 2015-06-22 09:44, Gonzalez Monroy, Sergio wrote:
> >> On 19/06/2015 22:29, Cyril Chemparathy wrote:
> >>> From: Cyril Chemparathy <cchemparathy@tilera.com>
> >>>
> >>> It is often useful to build with debug enabled, we add a config
> >>> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
> >>>
> >>>    +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
> >>> +TOOLCHAIN_CFLAGS += -g -ggdb
> >>> +TOOLCHAIN_LDFLAGS += -g -ggdb
> >>> +endif
> >> I don't think you need to modify the makefiles and introduce a new
> >> compile time option for this.
> >> The same result can be easily achieved by setting EXTRA_CFLAGS in the
> >> command line. ie:
> >>      $ make install T=x86_64-native-linuxapp-gcc EXTRA_CFLAGS='-g -ggdb'
> > Why isn't -g standard though? The binaries should/will anyhow be
> > stripped when used for production - but debugging information should be
> > useful when analysing crashes.
> 
> I guess you could argue that, to always build with debug info then strip 
> it down.
> You would need another flag to strip debug info for production, or leave 
> it for debugging.
> 
> In my opinion is not worth it, but it you feel strongly about it you can 
> submit patches and
> let the community decide.

I think stripping is a packaging responsibility.
It would be a good idea to always provide debugging symbols.

Re: [PATCH] mk: add support for gdb debug info generation

[Simon Kågström]

On 2015-06-23 09:47, Thomas Monjalon wrote:
> 2015-06-23 08:39, Gonzalez Monroy, Sergio:
>> I guess you could argue that, to always build with debug info then strip 
>> it down.
>> You would need another flag to strip debug info for production, or leave 
>> it for debugging.
>>
>> In my opinion is not worth it, but it you feel strongly about it you can 
>> submit patches and
>> let the community decide.
> 
> I think stripping is a packaging responsibility.
> It would be a good idea to always provide debugging symbols.

Yes, I think this would be the best way too, and should be pretty much
standard procedure.

DPDK is anyhow just a library - stripping should be up to the
application / packaging to do.

// Simon

Re: [PATCH] mk: add support for gdb debug info generation

[Cyril Chemparathy]

On Mon, 22 Jun 2015 08:44:41 +0100
"Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com> wrote:

> I don't think you need to modify the makefiles and introduce a new 
> compile time option for this.
> The same result can be easily achieved by setting EXTRA_CFLAGS in the 
> command line. ie:
>      $ make install T=x86_64-native-linuxapp-gcc EXTRA_CFLAGS='-g
> -ggdb'

Fair enough.  Please ignore this patch then.  Thanks!

[PATCH] mk: add support for gdb debug info generation

[Cyril Chemparathy]

It is often useful to build with debug enabled, we add a config
(CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.

Note: This patch does not include corresponding changes for ICC.  The author
pleads abject ignorance in this regard, and welcomes recommendations. :-)

Signed-off-by: Cyril Chemparathy <cchemparathy-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
---
 config/defconfig_x86_64-default-linuxapp-gcc | 1 +
 mk/toolchain/gcc/rte.vars.mk                 | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/config/defconfig_x86_64-default-linuxapp-gcc b/config/defconfig_x86_64-default-linuxapp-gcc
index f11ffbf..3b36efd 100644
--- a/config/defconfig_x86_64-default-linuxapp-gcc
+++ b/config/defconfig_x86_64-default-linuxapp-gcc
@@ -67,6 +67,7 @@ CONFIG_RTE_ARCH_X86_64=y
 #
 CONFIG_RTE_TOOLCHAIN="gcc"
 CONFIG_RTE_TOOLCHAIN_GCC=y
+CONFIG_RTE_TOOLCHAIN_DEBUG=n
 
 #
 # Use intrinsics or assembly code for key routines
diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
index 0edb93f..81ed3fa 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -68,6 +68,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS)))
 endif
 endif
 
+ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
+TOOLCHAIN_CFLAGS += -g -ggdb
+TOOLCHAIN_LDFLAGS += -g -ggdb
+endif
+
 WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes
 WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith
 WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual
-- 
1.8.3.1

Re: [PATCH] mk: add support for gdb debug info generation

[Ananyev, Konstantin]

Hi Cyril,
We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you can use to enable debug, or any other compiler/linker options you need.
Wonder, why that is not enough?
Thanks
Konstantin

-----Original Message-----
From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Cyril Chemparathy
Sent: Thursday, April 03, 2014 6:31 PM
To: dev-VfR2kkLFssw@public.gmane.org
Subject: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation

It is often useful to build with debug enabled, we add a config
(CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.

Note: This patch does not include corresponding changes for ICC.  The author pleads abject ignorance in this regard, and welcomes recommendations. :-)

Signed-off-by: Cyril Chemparathy <cchemparathy-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
---
 config/defconfig_x86_64-default-linuxapp-gcc | 1 +
 mk/toolchain/gcc/rte.vars.mk                 | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/config/defconfig_x86_64-default-linuxapp-gcc b/config/defconfig_x86_64-default-linuxapp-gcc
index f11ffbf..3b36efd 100644
--- a/config/defconfig_x86_64-default-linuxapp-gcc
+++ b/config/defconfig_x86_64-default-linuxapp-gcc
@@ -67,6 +67,7 @@ CONFIG_RTE_ARCH_X86_64=y  #  CONFIG_RTE_TOOLCHAIN="gcc"
 CONFIG_RTE_TOOLCHAIN_GCC=y
+CONFIG_RTE_TOOLCHAIN_DEBUG=n
 
 #
 # Use intrinsics or assembly code for key routines diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index 0edb93f..81ed3fa 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -68,6 +68,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS)))  endif  endif
 
+ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
+TOOLCHAIN_CFLAGS += -g -ggdb
+TOOLCHAIN_LDFLAGS += -g -ggdb
+endif
+
 WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes  WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith  WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual
--
1.8.3.1

Re: [PATCH] mk: add support for gdb debug info generation

[Marc Sune]

I don't like the proposed patch, but I am recovering this old thread 
because I agree on the problem statement.

On 04/04/14 11:57, Ananyev, Konstantin wrote:
> Hi Cyril,
> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you can use to enable debug, or any other compiler/linker options you need.
> Wonder, why that is not enough?

EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why 
setting individually:

diff --git a/config/common_linuxapp b/config/common_linuxapp
index 2f9643b..04adc0d 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
  # Compile generic ethernet library
  #
  CONFIG_RTE_LIBRTE_ETHER=y
-CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
+CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y


to put an example, does not set -g and -O0 in that particular module 
only. No one would ever use something compiled in DEBUG in production 
anyway.

I always end up modifying manually Makefiles in the lib library that I 
am interested in having insides, overriding CFLAGS=-O3, which is not 
that nice.

Marc

> Thanks
> Konstantin
>
> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Cyril Chemparathy
> Sent: Thursday, April 03, 2014 6:31 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation
>
> It is often useful to build with debug enabled, we add a config
> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
>
> Note: This patch does not include corresponding changes for ICC.  The author pleads abject ignorance in this regard, and welcomes recommendations. :-)
>
> Signed-off-by: Cyril Chemparathy <cchemparathy-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
> ---
>   config/defconfig_x86_64-default-linuxapp-gcc | 1 +
>   mk/toolchain/gcc/rte.vars.mk                 | 5 +++++
>   2 files changed, 6 insertions(+)
>
> diff --git a/config/defconfig_x86_64-default-linuxapp-gcc b/config/defconfig_x86_64-default-linuxapp-gcc
> index f11ffbf..3b36efd 100644
> --- a/config/defconfig_x86_64-default-linuxapp-gcc
> +++ b/config/defconfig_x86_64-default-linuxapp-gcc
> @@ -67,6 +67,7 @@ CONFIG_RTE_ARCH_X86_64=y  #  CONFIG_RTE_TOOLCHAIN="gcc"
>   CONFIG_RTE_TOOLCHAIN_GCC=y
> +CONFIG_RTE_TOOLCHAIN_DEBUG=n
>   
>   #
>   # Use intrinsics or assembly code for key routines diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index 0edb93f..81ed3fa 100644
> --- a/mk/toolchain/gcc/rte.vars.mk
> +++ b/mk/toolchain/gcc/rte.vars.mk
> @@ -68,6 +68,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS)))  endif  endif
>   
> +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
> +TOOLCHAIN_CFLAGS += -g -ggdb
> +TOOLCHAIN_LDFLAGS += -g -ggdb
> +endif
> +
>   WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes  WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith  WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual
> --
> 1.8.3.1
>

Re: [PATCH] mk: add support for gdb debug info generation

[Marc Sune]

On 22/02/15 12:51, Marc Sune wrote:
> I don't like the proposed patch, but I am recovering this old thread 
> because I agree on the problem statement.
>
> On 04/04/14 11:57, Ananyev, Konstantin wrote:
>> Hi Cyril,
>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you can 
>> use to enable debug, or any other compiler/linker options you need.
>> Wonder, why that is not enough?
>
> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why 
> setting individually:
>
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index 2f9643b..04adc0d 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>  # Compile generic ethernet library
>  #
>  CONFIG_RTE_LIBRTE_ETHER=y
> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
>
>
> to put an example, does not set -g and -O0 in that particular module 
> only. No one would ever use something compiled in DEBUG in production 
> anyway.
>
> I always end up modifying manually Makefiles in the lib library that I 
> am interested in having insides, overriding CFLAGS=-O3, which is not 
> that nice.

I would like some feedback on this idea. If the community sees benefit, 
I will work on a patch for this.

Marc

>
> Marc
>
>> Thanks
>> Konstantin
>>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Cyril Chemparathy
>> Sent: Thursday, April 03, 2014 6:31 PM
>> To: dev-VfR2kkLFssw@public.gmane.org
>> Subject: [dpdk-dev] [PATCH] mk: add support for gdb debug info 
>> generation
>>
>> It is often useful to build with debug enabled, we add a config
>> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
>>
>> Note: This patch does not include corresponding changes for ICC.  The 
>> author pleads abject ignorance in this regard, and welcomes 
>> recommendations. :-)
>>
>> Signed-off-by: Cyril Chemparathy <cchemparathy-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
>> ---
>>   config/defconfig_x86_64-default-linuxapp-gcc | 1 +
>>   mk/toolchain/gcc/rte.vars.mk                 | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/config/defconfig_x86_64-default-linuxapp-gcc 
>> b/config/defconfig_x86_64-default-linuxapp-gcc
>> index f11ffbf..3b36efd 100644
>> --- a/config/defconfig_x86_64-default-linuxapp-gcc
>> +++ b/config/defconfig_x86_64-default-linuxapp-gcc
>> @@ -67,6 +67,7 @@ CONFIG_RTE_ARCH_X86_64=y  # CONFIG_RTE_TOOLCHAIN="gcc"
>>   CONFIG_RTE_TOOLCHAIN_GCC=y
>> +CONFIG_RTE_TOOLCHAIN_DEBUG=n
>>     #
>>   # Use intrinsics or assembly code for key routines diff --git 
>> a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index 
>> 0edb93f..81ed3fa 100644
>> --- a/mk/toolchain/gcc/rte.vars.mk
>> +++ b/mk/toolchain/gcc/rte.vars.mk
>> @@ -68,6 +68,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS))) endif  
>> endif
>>   +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
>> +TOOLCHAIN_CFLAGS += -g -ggdb
>> +TOOLCHAIN_LDFLAGS += -g -ggdb
>> +endif
>> +
>>   WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes 
>> -Wmissing-prototypes  WERROR_FLAGS += -Wmissing-declarations 
>> -Wold-style-definition -Wpointer-arith  WERROR_FLAGS += -Wcast-align 
>> -Wnested-externs -Wcast-qual
>> -- 
>> 1.8.3.1
>>
>

Re: [PATCH] mk: add support for gdb debug info generation

[Bruce Richardson]

On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
> 
> On 22/02/15 12:51, Marc Sune wrote:
> >I don't like the proposed patch, but I am recovering this old thread
> >because I agree on the problem statement.
> >
> >On 04/04/14 11:57, Ananyev, Konstantin wrote:
> >>Hi Cyril,
> >>We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you can use
> >>to enable debug, or any other compiler/linker options you need.
> >>Wonder, why that is not enough?
> >
> >EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
> >setting individually:
> >
> >diff --git a/config/common_linuxapp b/config/common_linuxapp
> >index 2f9643b..04adc0d 100644
> >--- a/config/common_linuxapp
> >+++ b/config/common_linuxapp
> >@@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> > # Compile generic ethernet library
> > #
> > CONFIG_RTE_LIBRTE_ETHER=y
> >-CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> >+CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
> >
> >
> >to put an example, does not set -g and -O0 in that particular module only.
> >No one would ever use something compiled in DEBUG in production anyway.
> >
> >I always end up modifying manually Makefiles in the lib library that I am
> >interested in having insides, overriding CFLAGS=-O3, which is not that
> >nice.
> 
> I would like some feedback on this idea. If the community sees benefit, I
> will work on a patch for this.
> 
> Marc
> 

So, your proposal is to patch things so that any time one sets DEBUG=y in the
build-time config for a library, we change the '-O3' to '-O0' and set -g also.
Correct?

If that's the case, I see no harm in doing such a thing. I wonder how useful
the '-g' option would be, just limited to a single library, though. One would
suspect that it may be better applied globally, so that one can see the state
of the application as it makes the call into the library.

/Bruce

> >
> >Marc
> >
> >>Thanks
> >>Konstantin
> >>
> >>-----Original Message-----
> >>From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Cyril Chemparathy
> >>Sent: Thursday, April 03, 2014 6:31 PM
> >>To: dev-VfR2kkLFssw@public.gmane.org
> >>Subject: [dpdk-dev] [PATCH] mk: add support for gdb debug info
> >>generation
> >>
> >>It is often useful to build with debug enabled, we add a config
> >>(CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
> >>
> >>Note: This patch does not include corresponding changes for ICC.  The
> >>author pleads abject ignorance in this regard, and welcomes
> >>recommendations. :-)
> >>
> >>Signed-off-by: Cyril Chemparathy <cchemparathy-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
> >>---
> >>  config/defconfig_x86_64-default-linuxapp-gcc | 1 +
> >>  mk/toolchain/gcc/rte.vars.mk                 | 5 +++++
> >>  2 files changed, 6 insertions(+)
> >>
> >>diff --git a/config/defconfig_x86_64-default-linuxapp-gcc
> >>b/config/defconfig_x86_64-default-linuxapp-gcc
> >>index f11ffbf..3b36efd 100644
> >>--- a/config/defconfig_x86_64-default-linuxapp-gcc
> >>+++ b/config/defconfig_x86_64-default-linuxapp-gcc
> >>@@ -67,6 +67,7 @@ CONFIG_RTE_ARCH_X86_64=y  # CONFIG_RTE_TOOLCHAIN="gcc"
> >>  CONFIG_RTE_TOOLCHAIN_GCC=y
> >>+CONFIG_RTE_TOOLCHAIN_DEBUG=n
> >>    #
> >>  # Use intrinsics or assembly code for key routines diff --git
> >>a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index
> >>0edb93f..81ed3fa 100644
> >>--- a/mk/toolchain/gcc/rte.vars.mk
> >>+++ b/mk/toolchain/gcc/rte.vars.mk
> >>@@ -68,6 +68,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS))) endif
> >>endif
> >>  +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
> >>+TOOLCHAIN_CFLAGS += -g -ggdb
> >>+TOOLCHAIN_LDFLAGS += -g -ggdb
> >>+endif
> >>+
> >>  WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes
> >>-Wmissing-prototypes  WERROR_FLAGS += -Wmissing-declarations
> >>-Wold-style-definition -Wpointer-arith  WERROR_FLAGS += -Wcast-align
> >>-Wnested-externs -Wcast-qual
> >>-- 
> >>1.8.3.1
> >>
> >
> 

Re: [PATCH] mk: add support for gdb debug info generation

[Marc Sune]

On 03/03/15 10:33, Bruce Richardson wrote:
> On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
>> On 22/02/15 12:51, Marc Sune wrote:
>>> I don't like the proposed patch, but I am recovering this old thread
>>> because I agree on the problem statement.
>>>
>>> On 04/04/14 11:57, Ananyev, Konstantin wrote:
>>>> Hi Cyril,
>>>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you can use
>>>> to enable debug, or any other compiler/linker options you need.
>>>> Wonder, why that is not enough?
>>> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
>>> setting individually:
>>>
>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>> index 2f9643b..04adc0d 100644
>>> --- a/config/common_linuxapp
>>> +++ b/config/common_linuxapp
>>> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>>> # Compile generic ethernet library
>>> #
>>> CONFIG_RTE_LIBRTE_ETHER=y
>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
>>> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
>>>
>>>
>>> to put an example, does not set -g and -O0 in that particular module only.
>>> No one would ever use something compiled in DEBUG in production anyway.
>>>
>>> I always end up modifying manually Makefiles in the lib library that I am
>>> interested in having insides, overriding CFLAGS=-O3, which is not that
>>> nice.
>> I would like some feedback on this idea. If the community sees benefit, I
>> will work on a patch for this.
>>
>> Marc
>>
> So, your proposal is to patch things so that any time one sets DEBUG=y in the
> build-time config for a library, we change the '-O3' to '-O0' and set -g also.
> Correct?

I am not sure what you mean by 'patch things'. I would simply enable the 
build system to override the default compilation flags (now DPDK-wide, 
or specifically librte_ wide) when _DEBUG=y for a library, changing 
compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I 
have to check if -O0 already implicitly means -fno-inline (even for 
__attribute__((always_inline)) ).

I did a quick test. I chose KNI because it didn't have a DEBUG flag for 
the user-space library. For other libraries, the existing _DEBUG setting 
would be enough:

marc@dpdk:~/dpdk$ git diff HEAD
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 97f1c9e..8a3cef8 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
  #
  CONFIG_RTE_LIBRTE_KNI=y
  CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
+CONFIG_RTE_LIBRTE_KNI_DEBUG=y
  CONFIG_RTE_KNI_KO_DEBUG=n
  CONFIG_RTE_KNI_VHOST=n
  CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
index 7107832..895f64e 100644
--- a/lib/librte_kni/Makefile
+++ b/lib/librte_kni/Makefile
@@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
  # library name
  LIB = librte_kni.a

-CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
+CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)

  EXPORT_MAP := rte_kni_version.map

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 63a41e2..eee477d 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -78,6 +78,13 @@ endif
  ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
  ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
  LDLIBS += -lrte_kni
+
+ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
+CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
+else
+CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
+endif
+
  endif
  endif


Thoughts?

>
> If that's the case, I see no harm in doing such a thing. I wonder how useful
> the '-g' option would be, just limited to a single library, though. One would
> suspect that it may be better applied globally, so that one can see the state
> of the application as it makes the call into the library.

It is IMHO. I am usually doing it this way, specially for ethdev 
initialization and the specific pmd (we probably need more fine grained 
return codes there). If you enable the debug for that library, you 
generally want to check the code and the state there, and you can access 
the input parameters of the function and the state of that library 
itself, which is generally enough, I think.

Special cases are inline functions used by other libraries / user 
application, which are compiled either with the global DPDK flags (-O3) 
or by the user's application flags.

Perhaps it also makes sense, in addition, to have a global "DEBUG" knob, 
which would enable to compile the entire DPDK library code with -O0 -g 
and possibly also with -fno-inline. This would also help debugging the 
inline functions.

Marc

>
> /Bruce
>
>>> Marc
>>>
>>>> Thanks
>>>> Konstantin
>>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Cyril Chemparathy
>>>> Sent: Thursday, April 03, 2014 6:31 PM
>>>> To: dev-VfR2kkLFssw@public.gmane.org
>>>> Subject: [dpdk-dev] [PATCH] mk: add support for gdb debug info
>>>> generation
>>>>
>>>> It is often useful to build with debug enabled, we add a config
>>>> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so.
>>>>
>>>> Note: This patch does not include corresponding changes for ICC.  The
>>>> author pleads abject ignorance in this regard, and welcomes
>>>> recommendations. :-)
>>>>
>>>> Signed-off-by: Cyril Chemparathy <cchemparathy-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>   config/defconfig_x86_64-default-linuxapp-gcc | 1 +
>>>>   mk/toolchain/gcc/rte.vars.mk                 | 5 +++++
>>>>   2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/config/defconfig_x86_64-default-linuxapp-gcc
>>>> b/config/defconfig_x86_64-default-linuxapp-gcc
>>>> index f11ffbf..3b36efd 100644
>>>> --- a/config/defconfig_x86_64-default-linuxapp-gcc
>>>> +++ b/config/defconfig_x86_64-default-linuxapp-gcc
>>>> @@ -67,6 +67,7 @@ CONFIG_RTE_ARCH_X86_64=y  # CONFIG_RTE_TOOLCHAIN="gcc"
>>>>   CONFIG_RTE_TOOLCHAIN_GCC=y
>>>> +CONFIG_RTE_TOOLCHAIN_DEBUG=n
>>>>     #
>>>>   # Use intrinsics or assembly code for key routines diff --git
>>>> a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index
>>>> 0edb93f..81ed3fa 100644
>>>> --- a/mk/toolchain/gcc/rte.vars.mk
>>>> +++ b/mk/toolchain/gcc/rte.vars.mk
>>>> @@ -68,6 +68,11 @@ ifeq (,$(findstring -O0,$(EXTRA_CFLAGS))) endif
>>>> endif
>>>>   +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y)
>>>> +TOOLCHAIN_CFLAGS += -g -ggdb
>>>> +TOOLCHAIN_LDFLAGS += -g -ggdb
>>>> +endif
>>>> +
>>>>   WERROR_FLAGS := -W -Wall -Werror -Wstrict-prototypes
>>>> -Wmissing-prototypes  WERROR_FLAGS += -Wmissing-declarations
>>>> -Wold-style-definition -Wpointer-arith  WERROR_FLAGS += -Wcast-align
>>>> -Wnested-externs -Wcast-qual
>>>> -- 
>>>> 1.8.3.1
>>>>

Re: [PATCH] mk: add support for gdb debug info generation

[Panu Matilainen]

On 03/03/2015 02:19 PM, Marc Sune wrote:
>
> On 03/03/15 10:33, Bruce Richardson wrote:
>> On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
>>> On 22/02/15 12:51, Marc Sune wrote:
>>>> I don't like the proposed patch, but I am recovering this old thread
>>>> because I agree on the problem statement.
>>>>
>>>> On 04/04/14 11:57, Ananyev, Konstantin wrote:
>>>>> Hi Cyril,
>>>>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you can use
>>>>> to enable debug, or any other compiler/linker options you need.
>>>>> Wonder, why that is not enough?
>>>> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
>>>> setting individually:
>>>>
>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>>> index 2f9643b..04adc0d 100644
>>>> --- a/config/common_linuxapp
>>>> +++ b/config/common_linuxapp
>>>> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>>>> # Compile generic ethernet library
>>>> #
>>>> CONFIG_RTE_LIBRTE_ETHER=y
>>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
>>>> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
>>>>
>>>>
>>>> to put an example, does not set -g and -O0 in that particular module
>>>> only.
>>>> No one would ever use something compiled in DEBUG in production anyway.
>>>>
>>>> I always end up modifying manually Makefiles in the lib library that
>>>> I am
>>>> interested in having insides, overriding CFLAGS=-O3, which is not that
>>>> nice.
>>> I would like some feedback on this idea. If the community sees
>>> benefit, I
>>> will work on a patch for this.
>>>
>>> Marc
>>>
>> So, your proposal is to patch things so that any time one sets DEBUG=y
>> in the
>> build-time config for a library, we change the '-O3' to '-O0' and set
>> -g also.
>> Correct?
>
> I am not sure what you mean by 'patch things'. I would simply enable the
> build system to override the default compilation flags (now DPDK-wide,
> or specifically librte_ wide) when _DEBUG=y for a library, changing
> compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
> have to check if -O0 already implicitly means -fno-inline (even for
> __attribute__((always_inline)) ).
>
> I did a quick test. I chose KNI because it didn't have a DEBUG flag for
> the user-space library. For other libraries, the existing _DEBUG setting
> would be enough:
>
> marc@dpdk:~/dpdk$ git diff HEAD
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index 97f1c9e..8a3cef8 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
>   #
>   CONFIG_RTE_LIBRTE_KNI=y
>   CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
> +CONFIG_RTE_LIBRTE_KNI_DEBUG=y
>   CONFIG_RTE_KNI_KO_DEBUG=n
>   CONFIG_RTE_KNI_VHOST=n
>   CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
> diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
> index 7107832..895f64e 100644
> --- a/lib/librte_kni/Makefile
> +++ b/lib/librte_kni/Makefile
> @@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>   # library name
>   LIB = librte_kni.a
>
> -CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
>
>   EXPORT_MAP := rte_kni_version.map
>
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index 63a41e2..eee477d 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -78,6 +78,13 @@ endif
>   ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>   ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>   LDLIBS += -lrte_kni
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
> +else
> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
> +endif
> +
>   endif
>   endif
>
>
> Thoughts?

My 5c is that if anything, DPDK needs *less* places that muck around 
with compiler flags, not more. If you something like this for all the 
libraries in DPDK the number doesn't just increase a bit, it explodes.

I dont see that much point in this thing, but I'd approach it by 
defining the debug flags someplace central, say DEBUG_FLAGS, and append 
that to the common cflags when *_DEBUG config is enabled. At least with 
gcc the last option wins so if you just append -O0 when debugging then 
that's what wins, the earlier -O3 does not matter.

	- Panu -

	- Panu -

Re: [PATCH] mk: add support for gdb debug info generation

[Marc Sune]

On 03/03/15 13:40, Panu Matilainen wrote:
> On 03/03/2015 02:19 PM, Marc Sune wrote:
>>
>> On 03/03/15 10:33, Bruce Richardson wrote:
>>> On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
>>>> On 22/02/15 12:51, Marc Sune wrote:
>>>>> I don't like the proposed patch, but I am recovering this old thread
>>>>> because I agree on the problem statement.
>>>>>
>>>>> On 04/04/14 11:57, Ananyev, Konstantin wrote:
>>>>>> Hi Cyril,
>>>>>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you 
>>>>>> can use
>>>>>> to enable debug, or any other compiler/linker options you need.
>>>>>> Wonder, why that is not enough?
>>>>> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
>>>>> setting individually:
>>>>>
>>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>>>> index 2f9643b..04adc0d 100644
>>>>> --- a/config/common_linuxapp
>>>>> +++ b/config/common_linuxapp
>>>>> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>>>>> # Compile generic ethernet library
>>>>> #
>>>>> CONFIG_RTE_LIBRTE_ETHER=y
>>>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
>>>>> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
>>>>>
>>>>>
>>>>> to put an example, does not set -g and -O0 in that particular module
>>>>> only.
>>>>> No one would ever use something compiled in DEBUG in production 
>>>>> anyway.
>>>>>
>>>>> I always end up modifying manually Makefiles in the lib library that
>>>>> I am
>>>>> interested in having insides, overriding CFLAGS=-O3, which is not 
>>>>> that
>>>>> nice.
>>>> I would like some feedback on this idea. If the community sees
>>>> benefit, I
>>>> will work on a patch for this.
>>>>
>>>> Marc
>>>>
>>> So, your proposal is to patch things so that any time one sets DEBUG=y
>>> in the
>>> build-time config for a library, we change the '-O3' to '-O0' and set
>>> -g also.
>>> Correct?
>>
>> I am not sure what you mean by 'patch things'. I would simply enable the
>> build system to override the default compilation flags (now DPDK-wide,
>> or specifically librte_ wide) when _DEBUG=y for a library, changing
>> compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
>> have to check if -O0 already implicitly means -fno-inline (even for
>> __attribute__((always_inline)) ).
>>
>> I did a quick test. I chose KNI because it didn't have a DEBUG flag for
>> the user-space library. For other libraries, the existing _DEBUG setting
>> would be enough:
>>
>> marc@dpdk:~/dpdk$ git diff HEAD
>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>> index 97f1c9e..8a3cef8 100644
>> --- a/config/common_linuxapp
>> +++ b/config/common_linuxapp
>> @@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
>>   #
>>   CONFIG_RTE_LIBRTE_KNI=y
>>   CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
>> +CONFIG_RTE_LIBRTE_KNI_DEBUG=y
>>   CONFIG_RTE_KNI_KO_DEBUG=n
>>   CONFIG_RTE_KNI_VHOST=n
>>   CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
>> diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
>> index 7107832..895f64e 100644
>> --- a/lib/librte_kni/Makefile
>> +++ b/lib/librte_kni/Makefile
>> @@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>   # library name
>>   LIB = librte_kni.a
>>
>> -CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
>> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
>>
>>   EXPORT_MAP := rte_kni_version.map
>>
>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
>> index 63a41e2..eee477d 100644
>> --- a/mk/rte.app.mk
>> +++ b/mk/rte.app.mk
>> @@ -78,6 +78,13 @@ endif
>>   ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>>   ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>>   LDLIBS += -lrte_kni
>> +
>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
>> +else
>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
>> +endif
>> +
>>   endif
>>   endif
>>
>>
>> Thoughts?
>
> My 5c is that if anything, DPDK needs *less* places that muck around 
> with compiler flags, not more. If you something like this for all the 
> libraries in DPDK the number doesn't just increase a bit, it explodes.

If you check the part below this one in my original email, that you 
stripped out (without notice), the suggestion was also to add a global 
_DEBUG parameter for the entire DPDK set of libraries, to change all the 
CFLAGS at once (not in the attached PATCH).

>
> I dont see that much point in this thing, but I'd approach it by 
> defining the debug flags someplace central, say DEBUG_FLAGS, and 
> append that to the common cflags when *_DEBUG config is enabled. At 
> least with gcc the last option wins so if you just append -O0 when 
> debugging then that's what wins, the earlier -O3 does not matter.

The original problem is the one you expose; libraries hardcode the 
CFLAGS, ignoring user-flags. There is no way to change this unless you 
change the Makefiles directly.

But right now, each library does hardcode its *own* flags (check 
Makefiles for the libraries), so there is already not a unified approach 
here. I see for instance KNI having -fno-strict-aliasing while other 
libraries don't.

Having said that, there are moments, specially with -O3, in which to be 
able to reproduce a bug, you need to compile certain parts of code with 
-O3 and the rest with -O0 -g (the ones to be debugged). The approach 
proposed (both a global *and* a lib specific) allows that.

Marc

>
>     - Panu -
>
>     - Panu -
>

Re: [PATCH] mk: add support for gdb debug info generation

[Bruce Richardson]

On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
> 
> On 03/03/15 13:40, Panu Matilainen wrote:
> >On 03/03/2015 02:19 PM, Marc Sune wrote:
> >>
> >>On 03/03/15 10:33, Bruce Richardson wrote:
> >>>On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
> >>>>On 22/02/15 12:51, Marc Sune wrote:
> >>>>>I don't like the proposed patch, but I am recovering this old thread
> >>>>>because I agree on the problem statement.
> >>>>>
> >>>>>On 04/04/14 11:57, Ananyev, Konstantin wrote:
> >>>>>>Hi Cyril,
> >>>>>>We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you
> >>>>>>can use
> >>>>>>to enable debug, or any other compiler/linker options you need.
> >>>>>>Wonder, why that is not enough?
> >>>>>EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
> >>>>>setting individually:
> >>>>>
> >>>>>diff --git a/config/common_linuxapp b/config/common_linuxapp
> >>>>>index 2f9643b..04adc0d 100644
> >>>>>--- a/config/common_linuxapp
> >>>>>+++ b/config/common_linuxapp
> >>>>>@@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> >>>>># Compile generic ethernet library
> >>>>>#
> >>>>>CONFIG_RTE_LIBRTE_ETHER=y
> >>>>>-CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> >>>>>+CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
> >>>>>
> >>>>>
> >>>>>to put an example, does not set -g and -O0 in that particular module
> >>>>>only.
> >>>>>No one would ever use something compiled in DEBUG in production
> >>>>>anyway.
> >>>>>
> >>>>>I always end up modifying manually Makefiles in the lib library that
> >>>>>I am
> >>>>>interested in having insides, overriding CFLAGS=-O3, which is not
> >>>>>that
> >>>>>nice.
> >>>>I would like some feedback on this idea. If the community sees
> >>>>benefit, I
> >>>>will work on a patch for this.
> >>>>
> >>>>Marc
> >>>>
> >>>So, your proposal is to patch things so that any time one sets DEBUG=y
> >>>in the
> >>>build-time config for a library, we change the '-O3' to '-O0' and set
> >>>-g also.
> >>>Correct?
> >>
> >>I am not sure what you mean by 'patch things'. I would simply enable the
> >>build system to override the default compilation flags (now DPDK-wide,
> >>or specifically librte_ wide) when _DEBUG=y for a library, changing
> >>compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
> >>have to check if -O0 already implicitly means -fno-inline (even for
> >>__attribute__((always_inline)) ).
> >>
> >>I did a quick test. I chose KNI because it didn't have a DEBUG flag for
> >>the user-space library. For other libraries, the existing _DEBUG setting
> >>would be enough:
> >>
> >>marc@dpdk:~/dpdk$ git diff HEAD
> >>diff --git a/config/common_linuxapp b/config/common_linuxapp
> >>index 97f1c9e..8a3cef8 100644
> >>--- a/config/common_linuxapp
> >>+++ b/config/common_linuxapp
> >>@@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
> >>  #
> >>  CONFIG_RTE_LIBRTE_KNI=y
> >>  CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
> >>+CONFIG_RTE_LIBRTE_KNI_DEBUG=y
> >>  CONFIG_RTE_KNI_KO_DEBUG=n
> >>  CONFIG_RTE_KNI_VHOST=n
> >>  CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
> >>diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
> >>index 7107832..895f64e 100644
> >>--- a/lib/librte_kni/Makefile
> >>+++ b/lib/librte_kni/Makefile
> >>@@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >>  # library name
> >>  LIB = librte_kni.a
> >>
> >>-CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
> >>+CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
> >>
> >>  EXPORT_MAP := rte_kni_version.map
> >>
> >>diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> >>index 63a41e2..eee477d 100644
> >>--- a/mk/rte.app.mk
> >>+++ b/mk/rte.app.mk
> >>@@ -78,6 +78,13 @@ endif
> >>  ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> >>  ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> >>  LDLIBS += -lrte_kni
> >>+
> >>+ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
> >>+CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
> >>+else
> >>+CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
> >>+endif
> >>+
> >>  endif
> >>  endif
> >>
> >>
> >>Thoughts?
> >
> >My 5c is that if anything, DPDK needs *less* places that muck around with
> >compiler flags, not more. If you something like this for all the libraries
> >in DPDK the number doesn't just increase a bit, it explodes.
> 
> If you check the part below this one in my original email, that you stripped
> out (without notice), the suggestion was also to add a global _DEBUG
> parameter for the entire DPDK set of libraries, to change all the CFLAGS at
> once (not in the attached PATCH).
> 
> >
> >I dont see that much point in this thing, but I'd approach it by defining
> >the debug flags someplace central, say DEBUG_FLAGS, and append that to the
> >common cflags when *_DEBUG config is enabled. At least with gcc the last
> >option wins so if you just append -O0 when debugging then that's what
> >wins, the earlier -O3 does not matter.
> 
> The original problem is the one you expose; libraries hardcode the CFLAGS,
> ignoring user-flags. There is no way to change this unless you change the
> Makefiles directly.
> 
> But right now, each library does hardcode its *own* flags (check Makefiles
> for the libraries), so there is already not a unified approach here. I see
> for instance KNI having -fno-strict-aliasing while other libraries don't.
> 
> Having said that, there are moments, specially with -O3, in which to be able
> to reproduce a bug, you need to compile certain parts of code with -O3 and
> the rest with -O0 -g (the ones to be debugged). The approach proposed (both
> a global *and* a lib specific) allows that.
> 
> Marc
> 

I believe that the global option of overriding the CFLAGS is already sufficiently
covered - including being documented in programmers guide - by EXTRA_CFLAGS. The
ability to turn off optimization support for a single library is not covered
anywhere, and that suggestion seems reasonable to me. For each library, we can
just append '-O0 -g' to the CFLAGS in that libraries makefile if the debug option
is set. I don't see that as significantly complicating things [though I wouldn't
make any changes to the rte.app.mk to allow this, just have it per lib in the
lib's makefile]

/Bruce

> >    - Panu -
> >
> >    - Panu -
> >
> 

Re: [PATCH] mk: add support for gdb debug info generation

[Marc Sune]

On 03/03/15 14:03, Bruce Richardson wrote:
> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
>> On 03/03/15 13:40, Panu Matilainen wrote:
>>> On 03/03/2015 02:19 PM, Marc Sune wrote:
>>>> On 03/03/15 10:33, Bruce Richardson wrote:
>>>>> On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
>>>>>> On 22/02/15 12:51, Marc Sune wrote:
>>>>>>> I don't like the proposed patch, but I am recovering this old thread
>>>>>>> because I agree on the problem statement.
>>>>>>>
>>>>>>> On 04/04/14 11:57, Ananyev, Konstantin wrote:
>>>>>>>> Hi Cyril,
>>>>>>>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you
>>>>>>>> can use
>>>>>>>> to enable debug, or any other compiler/linker options you need.
>>>>>>>> Wonder, why that is not enough?
>>>>>>> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
>>>>>>> setting individually:
>>>>>>>
>>>>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>>>>>> index 2f9643b..04adc0d 100644
>>>>>>> --- a/config/common_linuxapp
>>>>>>> +++ b/config/common_linuxapp
>>>>>>> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>>>>>>> # Compile generic ethernet library
>>>>>>> #
>>>>>>> CONFIG_RTE_LIBRTE_ETHER=y
>>>>>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
>>>>>>> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
>>>>>>>
>>>>>>>
>>>>>>> to put an example, does not set -g and -O0 in that particular module
>>>>>>> only.
>>>>>>> No one would ever use something compiled in DEBUG in production
>>>>>>> anyway.
>>>>>>>
>>>>>>> I always end up modifying manually Makefiles in the lib library that
>>>>>>> I am
>>>>>>> interested in having insides, overriding CFLAGS=-O3, which is not
>>>>>>> that
>>>>>>> nice.
>>>>>> I would like some feedback on this idea. If the community sees
>>>>>> benefit, I
>>>>>> will work on a patch for this.
>>>>>>
>>>>>> Marc
>>>>>>
>>>>> So, your proposal is to patch things so that any time one sets DEBUG=y
>>>>> in the
>>>>> build-time config for a library, we change the '-O3' to '-O0' and set
>>>>> -g also.
>>>>> Correct?
>>>> I am not sure what you mean by 'patch things'. I would simply enable the
>>>> build system to override the default compilation flags (now DPDK-wide,
>>>> or specifically librte_ wide) when _DEBUG=y for a library, changing
>>>> compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
>>>> have to check if -O0 already implicitly means -fno-inline (even for
>>>> __attribute__((always_inline)) ).
>>>>
>>>> I did a quick test. I chose KNI because it didn't have a DEBUG flag for
>>>> the user-space library. For other libraries, the existing _DEBUG setting
>>>> would be enough:
>>>>
>>>> marc@dpdk:~/dpdk$ git diff HEAD
>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>>> index 97f1c9e..8a3cef8 100644
>>>> --- a/config/common_linuxapp
>>>> +++ b/config/common_linuxapp
>>>> @@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
>>>>   #
>>>>   CONFIG_RTE_LIBRTE_KNI=y
>>>>   CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
>>>> +CONFIG_RTE_LIBRTE_KNI_DEBUG=y
>>>>   CONFIG_RTE_KNI_KO_DEBUG=n
>>>>   CONFIG_RTE_KNI_VHOST=n
>>>>   CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
>>>> diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
>>>> index 7107832..895f64e 100644
>>>> --- a/lib/librte_kni/Makefile
>>>> +++ b/lib/librte_kni/Makefile
>>>> @@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>>>   # library name
>>>>   LIB = librte_kni.a
>>>>
>>>> -CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
>>>> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
>>>>
>>>>   EXPORT_MAP := rte_kni_version.map
>>>>
>>>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
>>>> index 63a41e2..eee477d 100644
>>>> --- a/mk/rte.app.mk
>>>> +++ b/mk/rte.app.mk
>>>> @@ -78,6 +78,13 @@ endif
>>>>   ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>>>>   ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>>>>   LDLIBS += -lrte_kni
>>>> +
>>>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
>>>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
>>>> +else
>>>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
>>>> +endif
>>>> +
>>>>   endif
>>>>   endif
>>>>
>>>>
>>>> Thoughts?
>>> My 5c is that if anything, DPDK needs *less* places that muck around with
>>> compiler flags, not more. If you something like this for all the libraries
>>> in DPDK the number doesn't just increase a bit, it explodes.
>> If you check the part below this one in my original email, that you stripped
>> out (without notice), the suggestion was also to add a global _DEBUG
>> parameter for the entire DPDK set of libraries, to change all the CFLAGS at
>> once (not in the attached PATCH).
>>
>>> I dont see that much point in this thing, but I'd approach it by defining
>>> the debug flags someplace central, say DEBUG_FLAGS, and append that to the
>>> common cflags when *_DEBUG config is enabled. At least with gcc the last
>>> option wins so if you just append -O0 when debugging then that's what
>>> wins, the earlier -O3 does not matter.
>> The original problem is the one you expose; libraries hardcode the CFLAGS,
>> ignoring user-flags. There is no way to change this unless you change the
>> Makefiles directly.
>>
>> But right now, each library does hardcode its *own* flags (check Makefiles
>> for the libraries), so there is already not a unified approach here. I see
>> for instance KNI having -fno-strict-aliasing while other libraries don't.
>>
>> Having said that, there are moments, specially with -O3, in which to be able
>> to reproduce a bug, you need to compile certain parts of code with -O3 and
>> the rest with -O0 -g (the ones to be debugged). The approach proposed (both
>> a global *and* a lib specific) allows that.
>>
>> Marc
>>
> I believe that the global option of overriding the CFLAGS is already sufficiently
> covered - including being documented in programmers guide - by EXTRA_CFLAGS.

To be honest, I tried EXTRA_CFLAGS at some point in time (probably 1.5 
or 1.6, but maybe not stable releases) and it did not work, so I ended 
up doing it manually, and never tried again.

It does work now with CFLAGS, I didn't try LDFLAGS, but it does not for 
EXTRA_CPPFLAGS apparently (unless I made some stupid mistake):

marc@dpdk:~/dpdk$ git diff
diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 4e70fa0..4a1e538 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -61,6 +61,10 @@

  #define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)

+#ifdef TEST_CPPFLAGS
+       #error TEST_CPPFLAGS defined
+#endif
+
  /**
   * KNI context
   */

marc@dpdk:~/dpdk$ export EXTRA_CPPFLAGS='-DTEST_CPPFLAGS'
marc@dpdk:~/dpdk$ make install T=x86_64-native-linuxapp-gcc
...
Build complete
marc@dpdk:~/dpdk$

> The
> ability to turn off optimization support for a single library is not covered
> anywhere, and that suggestion seems reasonable to me. For each library, we can
> just append '-O0 -g' to the CFLAGS in that libraries makefile if the debug option
> is set. I don't see that as significantly complicating things [though I wouldn't
> make any changes to the rte.app.mk to allow this, just have it per lib in the
> lib's makefile]

Right. It makes more sense there.

Marc

>
> /Bruce
>
>>>     - Panu -
>>>
>>>     - Panu -
>>>

Re: [PATCH] mk: add support for gdb debug info generation

[Olivier MATZ]

Hi Marc,

On 03/03/2015 02:27 PM, Marc Sune wrote:
>
> On 03/03/15 14:03, Bruce Richardson wrote:
>> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
>> [...]
>> I believe that the global option of overriding the CFLAGS is already
>> sufficiently
>> covered - including being documented in programmers guide - by
>> EXTRA_CFLAGS.
>
> To be honest, I tried EXTRA_CFLAGS at some point in time (probably 1.5
> or 1.6, but maybe not stable releases) and it did not work, so I ended
> up doing it manually, and never tried again.
>
> It does work now with CFLAGS, I didn't try LDFLAGS, but it does not for
> EXTRA_CPPFLAGS apparently (unless I made some stupid mistake):
>
> marc@dpdk:~/dpdk$ git diff
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index 4e70fa0..4a1e538 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -61,6 +61,10 @@
>
>   #define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)
>
> +#ifdef TEST_CPPFLAGS
> +       #error TEST_CPPFLAGS defined
> +#endif
> +
>   /**
>    * KNI context
>    */
>
> marc@dpdk:~/dpdk$ export EXTRA_CPPFLAGS='-DTEST_CPPFLAGS'
> marc@dpdk:~/dpdk$ make install T=x86_64-native-linuxapp-gcc
> ...
> Build complete

The reason why it does not work is described in the documentation:

./doc/guides/prog_guide/build_app.rst:*   CPPFLAGS: The flags to use to 
provide flags to the C preprocessor (only useful when assembling .S files)
./doc/guides/prog_guide/dev_kit_build_system.rst:*   CPPFLAGS: Flags to 
use to give flags to C preprocessor (only useful when assembling .S files).
./doc/guides/prog_guide/dev_kit_build_system.rst:*   EXTRA_CPPFLAGS: The 
content of this variable is appended after CPPFLAGS when using a C 
preprocessor on assembly files.

I think your test would work with EXTRA_CFLAGS.

I don't say it's the proper behavior, but at least it's coherent with
the documentation.

Regards,
Olivier

Re: [PATCH] mk: add support for gdb debug info generation

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Bruce Richardson
> Sent: Tuesday, March 03, 2015 1:03 PM
> To: Marc Sune
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation
> 
> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
> >
> > On 03/03/15 13:40, Panu Matilainen wrote:
> > >On 03/03/2015 02:19 PM, Marc Sune wrote:
> > >>
> > >>On 03/03/15 10:33, Bruce Richardson wrote:
> > >>>On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
> > >>>>On 22/02/15 12:51, Marc Sune wrote:
> > >>>>>I don't like the proposed patch, but I am recovering this old thread
> > >>>>>because I agree on the problem statement.
> > >>>>>
> > >>>>>On 04/04/14 11:57, Ananyev, Konstantin wrote:
> > >>>>>>Hi Cyril,
> > >>>>>>We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you
> > >>>>>>can use
> > >>>>>>to enable debug, or any other compiler/linker options you need.
> > >>>>>>Wonder, why that is not enough?
> > >>>>>EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
> > >>>>>setting individually:
> > >>>>>
> > >>>>>diff --git a/config/common_linuxapp b/config/common_linuxapp
> > >>>>>index 2f9643b..04adc0d 100644
> > >>>>>--- a/config/common_linuxapp
> > >>>>>+++ b/config/common_linuxapp
> > >>>>>@@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> > >>>>># Compile generic ethernet library
> > >>>>>#
> > >>>>>CONFIG_RTE_LIBRTE_ETHER=y
> > >>>>>-CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> > >>>>>+CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
> > >>>>>
> > >>>>>
> > >>>>>to put an example, does not set -g and -O0 in that particular module
> > >>>>>only.
> > >>>>>No one would ever use something compiled in DEBUG in production
> > >>>>>anyway.
> > >>>>>
> > >>>>>I always end up modifying manually Makefiles in the lib library that
> > >>>>>I am
> > >>>>>interested in having insides, overriding CFLAGS=-O3, which is not
> > >>>>>that
> > >>>>>nice.
> > >>>>I would like some feedback on this idea. If the community sees
> > >>>>benefit, I
> > >>>>will work on a patch for this.
> > >>>>
> > >>>>Marc
> > >>>>
> > >>>So, your proposal is to patch things so that any time one sets DEBUG=y
> > >>>in the
> > >>>build-time config for a library, we change the '-O3' to '-O0' and set
> > >>>-g also.
> > >>>Correct?
> > >>
> > >>I am not sure what you mean by 'patch things'. I would simply enable the
> > >>build system to override the default compilation flags (now DPDK-wide,
> > >>or specifically librte_ wide) when _DEBUG=y for a library, changing
> > >>compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
> > >>have to check if -O0 already implicitly means -fno-inline (even for
> > >>__attribute__((always_inline)) ).
> > >>
> > >>I did a quick test. I chose KNI because it didn't have a DEBUG flag for
> > >>the user-space library. For other libraries, the existing _DEBUG setting
> > >>would be enough:
> > >>
> > >>marc@dpdk:~/dpdk$ git diff HEAD
> > >>diff --git a/config/common_linuxapp b/config/common_linuxapp
> > >>index 97f1c9e..8a3cef8 100644
> > >>--- a/config/common_linuxapp
> > >>+++ b/config/common_linuxapp
> > >>@@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
> > >>  #
> > >>  CONFIG_RTE_LIBRTE_KNI=y
> > >>  CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
> > >>+CONFIG_RTE_LIBRTE_KNI_DEBUG=y
> > >>  CONFIG_RTE_KNI_KO_DEBUG=n
> > >>  CONFIG_RTE_KNI_VHOST=n
> > >>  CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
> > >>diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
> > >>index 7107832..895f64e 100644
> > >>--- a/lib/librte_kni/Makefile
> > >>+++ b/lib/librte_kni/Makefile
> > >>@@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > >>  # library name
> > >>  LIB = librte_kni.a
> > >>
> > >>-CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
> > >>+CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
> > >>
> > >>  EXPORT_MAP := rte_kni_version.map
> > >>
> > >>diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> > >>index 63a41e2..eee477d 100644
> > >>--- a/mk/rte.app.mk
> > >>+++ b/mk/rte.app.mk
> > >>@@ -78,6 +78,13 @@ endif
> > >>  ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> > >>  ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> > >>  LDLIBS += -lrte_kni
> > >>+
> > >>+ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
> > >>+CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
> > >>+else
> > >>+CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
> > >>+endif
> > >>+
> > >>  endif
> > >>  endif
> > >>
> > >>
> > >>Thoughts?
> > >
> > >My 5c is that if anything, DPDK needs *less* places that muck around with
> > >compiler flags, not more. If you something like this for all the libraries
> > >in DPDK the number doesn't just increase a bit, it explodes.
> >
> > If you check the part below this one in my original email, that you stripped
> > out (without notice), the suggestion was also to add a global _DEBUG
> > parameter for the entire DPDK set of libraries, to change all the CFLAGS at
> > once (not in the attached PATCH).
> >
> > >
> > >I dont see that much point in this thing, but I'd approach it by defining
> > >the debug flags someplace central, say DEBUG_FLAGS, and append that to the
> > >common cflags when *_DEBUG config is enabled. At least with gcc the last
> > >option wins so if you just append -O0 when debugging then that's what
> > >wins, the earlier -O3 does not matter.
> >
> > The original problem is the one you expose; libraries hardcode the CFLAGS,
> > ignoring user-flags. There is no way to change this unless you change the
> > Makefiles directly.
> >
> > But right now, each library does hardcode its *own* flags (check Makefiles
> > for the libraries), so there is already not a unified approach here. I see
> > for instance KNI having -fno-strict-aliasing while other libraries don't.
> >
> > Having said that, there are moments, specially with -O3, in which to be able
> > to reproduce a bug, you need to compile certain parts of code with -O3 and
> > the rest with -O0 -g (the ones to be debugged). The approach proposed (both
> > a global *and* a lib specific) allows that.
> >
> > Marc
> >
> 
> I believe that the global option of overriding the CFLAGS is already sufficiently
> covered - including being documented in programmers guide - by EXTRA_CFLAGS. The
> ability to turn off optimization support for a single library is not covered
> anywhere, and that suggestion seems reasonable to me. For each library, we can
> just append '-O0 -g' to the CFLAGS in that libraries makefile if the debug option
> is set. I don't see that as significantly complicating things [though I wouldn't
> make any changes to the rte.app.mk to allow this, just have it per lib in the
> lib's makefile]
> 

Well, but then people would say 'why only -g -O0' for _DEBUG config option?
And would ask for ability to add/change other compile options to add/change for particular library.
If we need that, probably we can introduce RTE_LIBRTE_<LIBNAME>_EXTRA_CFLAGS config macros for each library?
So people can set them in their config files to whatever they like? 

Konstantin

Re: [PATCH] mk: add support for gdb debug info generation

[Marc Sune]

On 03/03/15 14:31, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Bruce Richardson
>> Sent: Tuesday, March 03, 2015 1:03 PM
>> To: Marc Sune
>> Cc: dev-VfR2kkLFssw@public.gmane.org
>> Subject: Re: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation
>>
>> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
>>> On 03/03/15 13:40, Panu Matilainen wrote:
>>>> On 03/03/2015 02:19 PM, Marc Sune wrote:
>>>>> On 03/03/15 10:33, Bruce Richardson wrote:
>>>>>> On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
>>>>>>> On 22/02/15 12:51, Marc Sune wrote:
>>>>>>>> I don't like the proposed patch, but I am recovering this old thread
>>>>>>>> because I agree on the problem statement.
>>>>>>>>
>>>>>>>> On 04/04/14 11:57, Ananyev, Konstantin wrote:
>>>>>>>>> Hi Cyril,
>>>>>>>>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you
>>>>>>>>> can use
>>>>>>>>> to enable debug, or any other compiler/linker options you need.
>>>>>>>>> Wonder, why that is not enough?
>>>>>>>> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
>>>>>>>> setting individually:
>>>>>>>>
>>>>>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>>>>>>> index 2f9643b..04adc0d 100644
>>>>>>>> --- a/config/common_linuxapp
>>>>>>>> +++ b/config/common_linuxapp
>>>>>>>> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>>>>>>>> # Compile generic ethernet library
>>>>>>>> #
>>>>>>>> CONFIG_RTE_LIBRTE_ETHER=y
>>>>>>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
>>>>>>>> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
>>>>>>>>
>>>>>>>>
>>>>>>>> to put an example, does not set -g and -O0 in that particular module
>>>>>>>> only.
>>>>>>>> No one would ever use something compiled in DEBUG in production
>>>>>>>> anyway.
>>>>>>>>
>>>>>>>> I always end up modifying manually Makefiles in the lib library that
>>>>>>>> I am
>>>>>>>> interested in having insides, overriding CFLAGS=-O3, which is not
>>>>>>>> that
>>>>>>>> nice.
>>>>>>> I would like some feedback on this idea. If the community sees
>>>>>>> benefit, I
>>>>>>> will work on a patch for this.
>>>>>>>
>>>>>>> Marc
>>>>>>>
>>>>>> So, your proposal is to patch things so that any time one sets DEBUG=y
>>>>>> in the
>>>>>> build-time config for a library, we change the '-O3' to '-O0' and set
>>>>>> -g also.
>>>>>> Correct?
>>>>> I am not sure what you mean by 'patch things'. I would simply enable the
>>>>> build system to override the default compilation flags (now DPDK-wide,
>>>>> or specifically librte_ wide) when _DEBUG=y for a library, changing
>>>>> compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
>>>>> have to check if -O0 already implicitly means -fno-inline (even for
>>>>> __attribute__((always_inline)) ).
>>>>>
>>>>> I did a quick test. I chose KNI because it didn't have a DEBUG flag for
>>>>> the user-space library. For other libraries, the existing _DEBUG setting
>>>>> would be enough:
>>>>>
>>>>> marc@dpdk:~/dpdk$ git diff HEAD
>>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>>>> index 97f1c9e..8a3cef8 100644
>>>>> --- a/config/common_linuxapp
>>>>> +++ b/config/common_linuxapp
>>>>> @@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
>>>>>   #
>>>>>   CONFIG_RTE_LIBRTE_KNI=y
>>>>>   CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
>>>>> +CONFIG_RTE_LIBRTE_KNI_DEBUG=y
>>>>>   CONFIG_RTE_KNI_KO_DEBUG=n
>>>>>   CONFIG_RTE_KNI_VHOST=n
>>>>>   CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
>>>>> diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
>>>>> index 7107832..895f64e 100644
>>>>> --- a/lib/librte_kni/Makefile
>>>>> +++ b/lib/librte_kni/Makefile
>>>>> @@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>>>>   # library name
>>>>>   LIB = librte_kni.a
>>>>>
>>>>> -CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
>>>>> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
>>>>>
>>>>>   EXPORT_MAP := rte_kni_version.map
>>>>>
>>>>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
>>>>> index 63a41e2..eee477d 100644
>>>>> --- a/mk/rte.app.mk
>>>>> +++ b/mk/rte.app.mk
>>>>> @@ -78,6 +78,13 @@ endif
>>>>>   ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>>>>>   ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>>>>>   LDLIBS += -lrte_kni
>>>>> +
>>>>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
>>>>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
>>>>> +else
>>>>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
>>>>> +endif
>>>>> +
>>>>>   endif
>>>>>   endif
>>>>>
>>>>>
>>>>> Thoughts?
>>>> My 5c is that if anything, DPDK needs *less* places that muck around with
>>>> compiler flags, not more. If you something like this for all the libraries
>>>> in DPDK the number doesn't just increase a bit, it explodes.
>>> If you check the part below this one in my original email, that you stripped
>>> out (without notice), the suggestion was also to add a global _DEBUG
>>> parameter for the entire DPDK set of libraries, to change all the CFLAGS at
>>> once (not in the attached PATCH).
>>>
>>>> I dont see that much point in this thing, but I'd approach it by defining
>>>> the debug flags someplace central, say DEBUG_FLAGS, and append that to the
>>>> common cflags when *_DEBUG config is enabled. At least with gcc the last
>>>> option wins so if you just append -O0 when debugging then that's what
>>>> wins, the earlier -O3 does not matter.
>>> The original problem is the one you expose; libraries hardcode the CFLAGS,
>>> ignoring user-flags. There is no way to change this unless you change the
>>> Makefiles directly.
>>>
>>> But right now, each library does hardcode its *own* flags (check Makefiles
>>> for the libraries), so there is already not a unified approach here. I see
>>> for instance KNI having -fno-strict-aliasing while other libraries don't.
>>>
>>> Having said that, there are moments, specially with -O3, in which to be able
>>> to reproduce a bug, you need to compile certain parts of code with -O3 and
>>> the rest with -O0 -g (the ones to be debugged). The approach proposed (both
>>> a global *and* a lib specific) allows that.
>>>
>>> Marc
>>>
>> I believe that the global option of overriding the CFLAGS is already sufficiently
>> covered - including being documented in programmers guide - by EXTRA_CFLAGS. The
>> ability to turn off optimization support for a single library is not covered
>> anywhere, and that suggestion seems reasonable to me. For each library, we can
>> just append '-O0 -g' to the CFLAGS in that libraries makefile if the debug option
>> is set. I don't see that as significantly complicating things [though I wouldn't
>> make any changes to the rte.app.mk to allow this, just have it per lib in the
>> lib's makefile]
>>
> Well, but then people would say 'why only -g -O0' for _DEBUG config option?
> And would ask for ability to add/change other compile options to add/change for particular library.
> If we need that, probably we can introduce RTE_LIBRTE_<LIBNAME>_EXTRA_CFLAGS config macros for each library?
> So people can set them in their config files to whatever they like?

It is not a bad idea either. However, there are already those _DEBUG 
config elements (the KNI is a special case).

Maybe both can be combined (i.e. RTE_LIBRTE_<LIBNAME>_EXTRA_CFLAGS 
should always override _DEBUG). It would probably get a bit 
over-engineered then, though.

marc
>
> Konstantin

Re: [PATCH] mk: add support for gdb debug info generation

[Ananyev, Konstantin]

> -----Original Message-----
> From: Marc Sune [mailto:marc.sune-kpkqNMk1I7M@public.gmane.org]
> Sent: Tuesday, March 03, 2015 2:39 PM
> To: Ananyev, Konstantin; Richardson, Bruce
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation
> 
> 
> On 03/03/15 14:31, Ananyev, Konstantin wrote:
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Bruce Richardson
> >> Sent: Tuesday, March 03, 2015 1:03 PM
> >> To: Marc Sune
> >> Cc: dev-VfR2kkLFssw@public.gmane.org
> >> Subject: Re: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation
> >>
> >> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
> >>> On 03/03/15 13:40, Panu Matilainen wrote:
> >>>> On 03/03/2015 02:19 PM, Marc Sune wrote:
> >>>>> On 03/03/15 10:33, Bruce Richardson wrote:
> >>>>>> On Mon, Mar 02, 2015 at 06:32:13PM +0100, Marc Sune wrote:
> >>>>>>> On 22/02/15 12:51, Marc Sune wrote:
> >>>>>>>> I don't like the proposed patch, but I am recovering this old thread
> >>>>>>>> because I agree on the problem statement.
> >>>>>>>>
> >>>>>>>> On 04/04/14 11:57, Ananyev, Konstantin wrote:
> >>>>>>>>> Hi Cyril,
> >>>>>>>>> We already do have 'EXTRA_CFLAGS' and 'EXTRA_LDFLAGS' that you
> >>>>>>>>> can use
> >>>>>>>>> to enable debug, or any other compiler/linker options you need.
> >>>>>>>>> Wonder, why that is not enough?
> >>>>>>>> EXTRA_FLAGS var affects all the DPDK libraries. I was wondering why
> >>>>>>>> setting individually:
> >>>>>>>>
> >>>>>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
> >>>>>>>> index 2f9643b..04adc0d 100644
> >>>>>>>> --- a/config/common_linuxapp
> >>>>>>>> +++ b/config/common_linuxapp
> >>>>>>>> @@ -127,7 +127,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> >>>>>>>> # Compile generic ethernet library
> >>>>>>>> #
> >>>>>>>> CONFIG_RTE_LIBRTE_ETHER=y
> >>>>>>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> >>>>>>>> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> to put an example, does not set -g and -O0 in that particular module
> >>>>>>>> only.
> >>>>>>>> No one would ever use something compiled in DEBUG in production
> >>>>>>>> anyway.
> >>>>>>>>
> >>>>>>>> I always end up modifying manually Makefiles in the lib library that
> >>>>>>>> I am
> >>>>>>>> interested in having insides, overriding CFLAGS=-O3, which is not
> >>>>>>>> that
> >>>>>>>> nice.
> >>>>>>> I would like some feedback on this idea. If the community sees
> >>>>>>> benefit, I
> >>>>>>> will work on a patch for this.
> >>>>>>>
> >>>>>>> Marc
> >>>>>>>
> >>>>>> So, your proposal is to patch things so that any time one sets DEBUG=y
> >>>>>> in the
> >>>>>> build-time config for a library, we change the '-O3' to '-O0' and set
> >>>>>> -g also.
> >>>>>> Correct?
> >>>>> I am not sure what you mean by 'patch things'. I would simply enable the
> >>>>> build system to override the default compilation flags (now DPDK-wide,
> >>>>> or specifically librte_ wide) when _DEBUG=y for a library, changing
> >>>>> compilation flags from -O3 to -O0 -g and possibly also -fno-inline. I
> >>>>> have to check if -O0 already implicitly means -fno-inline (even for
> >>>>> __attribute__((always_inline)) ).
> >>>>>
> >>>>> I did a quick test. I chose KNI because it didn't have a DEBUG flag for
> >>>>> the user-space library. For other libraries, the existing _DEBUG setting
> >>>>> would be enough:
> >>>>>
> >>>>> marc@dpdk:~/dpdk$ git diff HEAD
> >>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
> >>>>> index 97f1c9e..8a3cef8 100644
> >>>>> --- a/config/common_linuxapp
> >>>>> +++ b/config/common_linuxapp
> >>>>> @@ -405,6 +405,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
> >>>>>   #
> >>>>>   CONFIG_RTE_LIBRTE_KNI=y
> >>>>>   CONFIG_RTE_KNI_PREEMPT_DEFAULT=y
> >>>>> +CONFIG_RTE_LIBRTE_KNI_DEBUG=y
> >>>>>   CONFIG_RTE_KNI_KO_DEBUG=n
> >>>>>   CONFIG_RTE_KNI_VHOST=n
> >>>>>   CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
> >>>>> diff --git a/lib/librte_kni/Makefile b/lib/librte_kni/Makefile
> >>>>> index 7107832..895f64e 100644
> >>>>> --- a/lib/librte_kni/Makefile
> >>>>> +++ b/lib/librte_kni/Makefile
> >>>>> @@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >>>>>   # library name
> >>>>>   LIB = librte_kni.a
> >>>>>
> >>>>> -CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
> >>>>> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) $(CONFIG_RTE_LIBRTE_KNI_CFLAGS)
> >>>>>
> >>>>>   EXPORT_MAP := rte_kni_version.map
> >>>>>
> >>>>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> >>>>> index 63a41e2..eee477d 100644
> >>>>> --- a/mk/rte.app.mk
> >>>>> +++ b/mk/rte.app.mk
> >>>>> @@ -78,6 +78,13 @@ endif
> >>>>>   ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> >>>>>   ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> >>>>>   LDLIBS += -lrte_kni
> >>>>> +
> >>>>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI_DEBUG),y)
> >>>>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O0 -g -fno-strict-aliasing -fno-inline
> >>>>> +else
> >>>>> +CONFIG_RTE_LIBRTE_KNI_CFLAGS = -O3 -fno-strict-aliasing
> >>>>> +endif
> >>>>> +
> >>>>>   endif
> >>>>>   endif
> >>>>>
> >>>>>
> >>>>> Thoughts?
> >>>> My 5c is that if anything, DPDK needs *less* places that muck around with
> >>>> compiler flags, not more. If you something like this for all the libraries
> >>>> in DPDK the number doesn't just increase a bit, it explodes.
> >>> If you check the part below this one in my original email, that you stripped
> >>> out (without notice), the suggestion was also to add a global _DEBUG
> >>> parameter for the entire DPDK set of libraries, to change all the CFLAGS at
> >>> once (not in the attached PATCH).
> >>>
> >>>> I dont see that much point in this thing, but I'd approach it by defining
> >>>> the debug flags someplace central, say DEBUG_FLAGS, and append that to the
> >>>> common cflags when *_DEBUG config is enabled. At least with gcc the last
> >>>> option wins so if you just append -O0 when debugging then that's what
> >>>> wins, the earlier -O3 does not matter.
> >>> The original problem is the one you expose; libraries hardcode the CFLAGS,
> >>> ignoring user-flags. There is no way to change this unless you change the
> >>> Makefiles directly.
> >>>
> >>> But right now, each library does hardcode its *own* flags (check Makefiles
> >>> for the libraries), so there is already not a unified approach here. I see
> >>> for instance KNI having -fno-strict-aliasing while other libraries don't.
> >>>
> >>> Having said that, there are moments, specially with -O3, in which to be able
> >>> to reproduce a bug, you need to compile certain parts of code with -O3 and
> >>> the rest with -O0 -g (the ones to be debugged). The approach proposed (both
> >>> a global *and* a lib specific) allows that.
> >>>
> >>> Marc
> >>>
> >> I believe that the global option of overriding the CFLAGS is already sufficiently
> >> covered - including being documented in programmers guide - by EXTRA_CFLAGS. The
> >> ability to turn off optimization support for a single library is not covered
> >> anywhere, and that suggestion seems reasonable to me. For each library, we can
> >> just append '-O0 -g' to the CFLAGS in that libraries makefile if the debug option
> >> is set. I don't see that as significantly complicating things [though I wouldn't
> >> make any changes to the rte.app.mk to allow this, just have it per lib in the
> >> lib's makefile]
> >>
> > Well, but then people would say 'why only -g -O0' for _DEBUG config option?
> > And would ask for ability to add/change other compile options to add/change for particular library.
> > If we need that, probably we can introduce RTE_LIBRTE_<LIBNAME>_EXTRA_CFLAGS config macros for each library?
> > So people can set them in their config files to whatever they like?
> 
> It is not a bad idea either. However, there are already those _DEBUG
> config elements (the KNI is a special case).

> 
> Maybe both can be combined (i.e. RTE_LIBRTE_<LIBNAME>_EXTRA_CFLAGS
> should always override _DEBUG). It would probably get a bit
> over-engineered then, though.

I would probably leave _DEBUG config options with what they are doing right now:
perform extra checks and generate logs.
After all, people might want their program to be more verbose, but they don't want compile options to be changed.  

Konstantin

> 
> marc
> >
> > Konstantin

Re: [PATCH] mk: add support for gdb debug info generation

[Thomas Monjalon]

2015-03-03 13:03, Bruce Richardson:
> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
> > On 03/03/15 13:40, Panu Matilainen wrote:
> > >My 5c is that if anything, DPDK needs *less* places that muck around with
> > >compiler flags, not more. If you something like this for all the libraries
> > >in DPDK the number doesn't just increase a bit, it explodes.
> > 
> > If you check the part below this one in my original email, that you stripped
> > out (without notice), the suggestion was also to add a global _DEBUG
> > parameter for the entire DPDK set of libraries, to change all the CFLAGS at
> > once (not in the attached PATCH).
> > 
> > >I dont see that much point in this thing, but I'd approach it by defining
> > >the debug flags someplace central, say DEBUG_FLAGS, and append that to the
> > >common cflags when *_DEBUG config is enabled. At least with gcc the last
> > >option wins so if you just append -O0 when debugging then that's what
> > >wins, the earlier -O3 does not matter.
> > 
> > The original problem is the one you expose; libraries hardcode the CFLAGS,
> > ignoring user-flags. There is no way to change this unless you change the
> > Makefiles directly.
> > 
> > But right now, each library does hardcode its *own* flags (check Makefiles
> > for the libraries), so there is already not a unified approach here. I see
> > for instance KNI having -fno-strict-aliasing while other libraries don't.
> > 
> > Having said that, there are moments, specially with -O3, in which to be able
> > to reproduce a bug, you need to compile certain parts of code with -O3 and
> > the rest with -O0 -g (the ones to be debugged). The approach proposed (both
> > a global *and* a lib specific) allows that.
> > 
> > Marc
> 
> I believe that the global option of overriding the CFLAGS is already sufficiently
> covered - including being documented in programmers guide - by EXTRA_CFLAGS. The
> ability to turn off optimization support for a single library is not covered
> anywhere, and that suggestion seems reasonable to me. For each library, we can
> just append '-O0 -g' to the CFLAGS in that libraries makefile if the debug option
> is set. I don't see that as significantly complicating things [though I wouldn't
> make any changes to the rte.app.mk to allow this, just have it per lib in the
> lib's makefile]

The difficult thing with a build system, is to know which options and use cases
we should support. Today you are suggesting some debug options for gdb.
Tomorrow someone would like to have a valgrind support and someone else would
like more options for a static analyzer.
I think that these usages are restricted to developers use and they already
can tune the Makefiles of the libs they are working on.