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] /Bruce > > - Panu - > > > > - Panu - > > >
