> -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce Richardson > Sent: Tuesday, March 03, 2015 1:03 PM > To: Marc Sune > Cc: dev at dpdk.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 at 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
