| Message ID | 1464118912-19658-5-git-send-email-nhorman@tuxdriver.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| 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 79EEF5398; Tue, 24 May 2016 21:42:50 +0200 (CEST) Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 2F1975398 for <dev@dpdk.org>; Tue, 24 May 2016 21:42:49 +0200 (CEST) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from <nhorman@tuxdriver.com>) id 1b5IE0-0001Ch-4B; Tue, 24 May 2016 15:42:46 -0400 From: Neil Horman <nhorman@tuxdriver.com> To: dev@dpdk.org Cc: Neil Horman <nhorman@tuxdriver.com>, Bruce Richardson <bruce.richardson@intel.com>, Thomas Monjalon <thomas.monjalon@6wind.com>, Stephen Hemminger <stephen@networkplumber.org>, Panu Matilainen <pmatilai@redhat.com> Date: Tue, 24 May 2016 15:41:51 -0400 Message-Id: <1464118912-19658-5-git-send-email-nhorman@tuxdriver.com> X-Mailer: git-send-email 2.5.5 In-Reply-To: <1464118912-19658-1-git-send-email-nhorman@tuxdriver.com> References: <1463431287-4551-1-git-send-email-nhorman@tuxdriver.com> <1464118912-19658-1-git-send-email-nhorman@tuxdriver.com> X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: [dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver 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
Neil Horman
May 24, 2016, 7:41 p.m. UTC
Modify the compilation makefile to identify C files that export PMD information,
and use that to trigger execution of the pmdinfo binary. If the execution of
pmdinfo is successful, compile the output C file to an object, and use the
linker to do relocatable linking on the resultant object file into the parent
object that it came from. This effectively just adds the json string into the
string table of the object that defines the PMD to the outside world.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Bruce Richardson <bruce.richardson@intel.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: Panu Matilainen <pmatilai@redhat.com>
---
mk/internal/rte.compile-pre.mk | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
Comments
2016-05-24 15:41, Neil Horman: > --- a/mk/internal/rte.compile-pre.mk > +++ b/mk/internal/rte.compile-pre.mk > @@ -80,7 +80,8 @@ C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight > C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)"," HOSTCC $(@)") > else > C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CFLAGS) \ > - $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $< > + $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $< > + whitespace change? > C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight > C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)"," CC $(@)") > endif > @@ -88,10 +89,26 @@ C_TO_O_CMD = 'cmd_$@ = $(C_TO_O_STR)' > C_TO_O_DO = @set -e; \ > echo $(C_TO_O_DISP); \ > $(C_TO_O) && \ > + sh -c "grep -q \"PMD_REGISTER_DRIVER(.*)\" $<; \ > + if [ \$$? -eq 0 ]; \ > + then \ It is preferred to keep "then" at the end of the previous line. > + echo MODGEN $@; \ > + OBJF=`readlink -f $@`; \ > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ Maybe .pmd.c would be more appropriate than .mod.c? What means mod/MODGEN/MODBUILD? > + if [ \$$? -eq 0 ]; \ > + then \ > + echo MODBUILD $@; \ > + $(CC) -c -o \$$OBJF.mod.o \$$OBJF.mod.c; \ > + $(CROSS)ld -r -o \$$OBJF.o \$$OBJF.mod.o \$$OBJF; \ > + mv -f \$$OBJF.o \$$OBJF; \ > + fi; \ > + fi; \ > + true" && \ Why "true"? It deserves to be in a shell script, at least to ease testing.
On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote: > 2016-05-24 15:41, Neil Horman: > > --- a/mk/internal/rte.compile-pre.mk > > +++ b/mk/internal/rte.compile-pre.mk > > @@ -80,7 +80,8 @@ C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight > > C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)"," HOSTCC $(@)") > > else > > C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CFLAGS) \ > > - $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $< > > + $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $< > > + > > whitespace change? > Looks like, I'll remove it > > C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight > > C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)"," CC $(@)") > > endif > > @@ -88,10 +89,26 @@ C_TO_O_CMD = 'cmd_$@ = $(C_TO_O_STR)' > > C_TO_O_DO = @set -e; \ > > echo $(C_TO_O_DISP); \ > > $(C_TO_O) && \ > > + sh -c "grep -q \"PMD_REGISTER_DRIVER(.*)\" $<; \ > > + if [ \$$? -eq 0 ]; \ > > + then \ > > It is preferred to keep "then" at the end of the previous line. Very well. > > > + echo MODGEN $@; \ > > + OBJF=`readlink -f $@`; \ > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ > > Maybe .pmd.c would be more appropriate than .mod.c? fine > What means mod/MODGEN/MODBUILD? GENerate Module information & BUILD module information. > > > + if [ \$$? -eq 0 ]; \ > > + then \ > > + echo MODBUILD $@; \ > > + $(CC) -c -o \$$OBJF.mod.o \$$OBJF.mod.c; \ > > + $(CROSS)ld -r -o \$$OBJF.o \$$OBJF.mod.o \$$OBJF; \ > > + mv -f \$$OBJF.o \$$OBJF; \ > > + fi; \ > > + fi; \ > > + true" && \ > > Why "true"? Debugging statement, I'll remove it. > > It deserves to be in a shell script, at least to ease testing. What do you mean by "it" and why would it be easier to test in a shell script? > >
2016-05-25 13:40, Neil Horman: > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote: > > 2016-05-24 15:41, Neil Horman: > > > + echo MODGEN $@; \ > > > + OBJF=`readlink -f $@`; \ > > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ > > > > Maybe .pmd.c would be more appropriate than .mod.c? > fine > > What means mod/MODGEN/MODBUILD? > GENerate Module information & BUILD module information. I think "module" is not appropriate here. > > It deserves to be in a shell script, at least to ease testing. > What do you mean by "it" and why would it be easier to test in a shell script? "it" is mostly this whole patch. With a shell script, we can test the behaviour on one file easily. Maybe I'm wrong, but I don't like having too much lines in a Makefile rule. We probably need more opinions.
On Wed, May 25, 2016 at 08:56:25PM +0200, Thomas Monjalon wrote: > 2016-05-25 13:40, Neil Horman: > > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote: > > > 2016-05-24 15:41, Neil Horman: > > > > + echo MODGEN $@; \ > > > > + OBJF=`readlink -f $@`; \ > > > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ > > > > > > Maybe .pmd.c would be more appropriate than .mod.c? > > fine > > > What means mod/MODGEN/MODBUILD? > > GENerate Module information & BUILD module information. > > I think "module" is not appropriate here. > This is starting to feel very much like bikeshedding. What do you think would be more appropriate here? > > > It deserves to be in a shell script, at least to ease testing. > > What do you mean by "it" and why would it be easier to test in a shell script? > > "it" is mostly this whole patch. > With a shell script, we can test the behaviour on one file easily. > Maybe I'm wrong, but I don't like having too much lines in a Makefile rule. > We probably need more opinions. > That makes no sense to me. Any such script would need to receive two arguments: 1) The path to a C file for a pmd 2) The path to the corresponding object file for that pmd Running any such script is then usless unles its predecated on first building all the object files in the pmd. And if you want to run something by hand on the object files, it seems pretty straightforward to do so, just run: build/builttools/pmdinfogen /path/to/pmd/object/file The rest of that code is really just a test to avoid having to run pmdinfo gen on any files other than the ones that contain the PMD_REGISTER_DRIVER macro Neil
2016-05-25 15:43, Neil Horman: > On Wed, May 25, 2016 at 08:56:25PM +0200, Thomas Monjalon wrote: > > 2016-05-25 13:40, Neil Horman: > > > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote: > > > > 2016-05-24 15:41, Neil Horman: > > > > > + echo MODGEN $@; \ > > > > > + OBJF=`readlink -f $@`; \ > > > > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ > > > > > > > > Maybe .pmd.c would be more appropriate than .mod.c? > > > fine > > > > What means mod/MODGEN/MODBUILD? > > > GENerate Module information & BUILD module information. > > > > I think "module" is not appropriate here. > > > This is starting to feel very much like bikeshedding. What do you think would > be more appropriate here? pmd/PMDINFO// > > > > It deserves to be in a shell script, at least to ease testing. > > > What do you mean by "it" and why would it be easier to test in a shell script? > > > > "it" is mostly this whole patch. > > With a shell script, we can test the behaviour on one file easily. > > Maybe I'm wrong, but I don't like having too much lines in a Makefile rule. > > We probably need more opinions. > > > That makes no sense to me. Any such script would need to receive two arguments: > 1) The path to a C file for a pmd > 2) The path to the corresponding object file for that pmd > > Running any such script is then usless unles its predecated on first building > all the object files in the pmd. And if you want to run something by hand on > the object files, it seems pretty straightforward to do so, just run: > build/builttools/pmdinfogen /path/to/pmd/object/file > > The rest of that code is really just a test to avoid having to run pmdinfo gen > on any files other than the ones that contain the PMD_REGISTER_DRIVER macro OK, no strong opinion here. If you feel comfortable with multi-lines "sh -c" and escaping, up to you. If I discover something wrong in this part and needs to do some maintenance work, I'll probably think differently.
On Wed, May 25, 2016 at 10:04:11PM +0200, Thomas Monjalon wrote: > 2016-05-25 15:43, Neil Horman: > > On Wed, May 25, 2016 at 08:56:25PM +0200, Thomas Monjalon wrote: > > > 2016-05-25 13:40, Neil Horman: > > > > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote: > > > > > 2016-05-24 15:41, Neil Horman: > > > > > > + echo MODGEN $@; \ > > > > > > + OBJF=`readlink -f $@`; \ > > > > > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ > > > > > > > > > > Maybe .pmd.c would be more appropriate than .mod.c? > > > > fine > > > > > What means mod/MODGEN/MODBUILD? > > > > GENerate Module information & BUILD module information. > > > > > > I think "module" is not appropriate here. > > > > > This is starting to feel very much like bikeshedding. What do you think would > > be more appropriate here? > > pmd/PMDINFO// > > > > > > It deserves to be in a shell script, at least to ease testing. > > > > What do you mean by "it" and why would it be easier to test in a shell script? > > > > > > "it" is mostly this whole patch. > > > With a shell script, we can test the behaviour on one file easily. > > > Maybe I'm wrong, but I don't like having too much lines in a Makefile rule. > > > We probably need more opinions. > > > > > That makes no sense to me. Any such script would need to receive two arguments: > > 1) The path to a C file for a pmd > > 2) The path to the corresponding object file for that pmd > > > > Running any such script is then usless unles its predecated on first building > > all the object files in the pmd. And if you want to run something by hand on > > the object files, it seems pretty straightforward to do so, just run: > > build/builttools/pmdinfogen /path/to/pmd/object/file > > > > The rest of that code is really just a test to avoid having to run pmdinfo gen > > on any files other than the ones that contain the PMD_REGISTER_DRIVER macro > > OK, no strong opinion here. > If you feel comfortable with multi-lines "sh -c" and escaping, up to you. > If I discover something wrong in this part and needs to do some maintenance > work, I'll probably think differently. > You're welcome to assign the bug to me :) Neil
diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk index b9bff4a..6fa4042 100644 --- a/mk/internal/rte.compile-pre.mk +++ b/mk/internal/rte.compile-pre.mk @@ -80,7 +80,8 @@ C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)"," HOSTCC $(@)") else C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CFLAGS) \ - $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $< + $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $< + C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)"," CC $(@)") endif @@ -88,10 +89,26 @@ C_TO_O_CMD = 'cmd_$@ = $(C_TO_O_STR)' C_TO_O_DO = @set -e; \ echo $(C_TO_O_DISP); \ $(C_TO_O) && \ + sh -c "grep -q \"PMD_REGISTER_DRIVER(.*)\" $<; \ + if [ \$$? -eq 0 ]; \ + then \ + echo MODGEN $@; \ + OBJF=`readlink -f $@`; \ + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \ + if [ \$$? -eq 0 ]; \ + then \ + echo MODBUILD $@; \ + $(CC) -c -o \$$OBJF.mod.o \$$OBJF.mod.c; \ + $(CROSS)ld -r -o \$$OBJF.o \$$OBJF.mod.o \$$OBJF; \ + mv -f \$$OBJF.o \$$OBJF; \ + fi; \ + fi; \ + true" && \ echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \ sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \ rm -f $(call obj2dep,$(@)).tmp + # return an empty string if string are equal compare = $(strip $(subst $(1),,$(2)) $(subst $(2),,$(1)))