Message ID | 54F5A6CF.2090203@bisdn.de (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 708EF5A84; Tue, 3 Mar 2015 13:19:31 +0100 (CET) Received: from mx.bisdn.de (mx.bisdn.de [185.27.182.31]) by dpdk.org (Postfix) with ESMTP id 6CF9C5A83 for <dev@dpdk.org>; Tue, 3 Mar 2015 13:19:30 +0100 (CET) Received: from [192.168.1.35] (11.Red-79-144-249.dynamicIP.rima-tde.net [79.144.249.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx.bisdn.de (Postfix) with ESMTPSA id DA880A3174; Tue, 3 Mar 2015 13:19:29 +0100 (CET) Message-ID: <54F5A6CF.2090203@bisdn.de> Date: Tue, 03 Mar 2015 13:19:27 +0100 From: Marc Sune <marc.sune@bisdn.de> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.4.0 MIME-Version: 1.0 To: Bruce Richardson <bruce.richardson@intel.com> References: <1396546285-29972-1-git-send-email-cchemparathy@tilera.com> <2601191342CEEE43887BDE71AB9772580EF948F7@IRSMSX105.ger.corp.intel.com> <54E9C2C0.3090108@bisdn.de> <54F49E9D.6070201@bisdn.de> <20150303093355.GA7300@bricha3-MOBL3> In-Reply-To: <20150303093355.GA7300@bricha3-MOBL3> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] mk: add support for gdb debug info generation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Marc Sune
March 3, 2015, 12:19 p.m. UTC
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 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@dpdk.org] On Behalf Of Cyril Chemparathy >>>> Sent: Thursday, April 03, 2014 6:31 PM >>>> To: dev@dpdk.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@tilera.com> >>>> --- >>>> 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 >>>>
Comments
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 -
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 - >
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 - > > >
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson > Sent: Tuesday, March 03, 2015 1:03 PM > To: Marc Sune > Cc: dev@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@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
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.
On 03/03/15 14:31, Ananyev, Konstantin wrote: > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson >> Sent: Tuesday, March 03, 2015 1:03 PM >> To: Marc Sune >> Cc: dev@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@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
> -----Original Message----- > From: Marc Sune [mailto:marc.sune@bisdn.de] > Sent: Tuesday, March 03, 2015 2:39 PM > To: Ananyev, Konstantin; Richardson, Bruce > Cc: dev@dpdk.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@dpdk.org] On Behalf Of Bruce Richardson > >> Sent: Tuesday, March 03, 2015 1:03 PM > >> To: Marc Sune > >> Cc: dev@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@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
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