From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx.bisdn.de (mx.bisdn.de [185.27.182.31]) by dpdk.org (Postfix) with ESMTP id 9BFE05A83 for ; Tue, 3 Mar 2015 14:27:44 +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 1E6A1A3174; Tue, 3 Mar 2015 14:27:44 +0100 (CET) Message-ID: <54F5B6CD.7090209@bisdn.de> Date: Tue, 03 Mar 2015 14:27:41 +0100 From: Marc Sune 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 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> <54F5A6CF.2090203@bisdn.de> <54F5ABCA.4010707@redhat.com> <54F5AF73.1060109@bisdn.de> <20150303130301.GA11084@bricha3-MOBL3> In-Reply-To: <20150303130301.GA11084@bricha3-MOBL3> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Mar 2015 13:27:44 -0000 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 - >>>