Hello Thomas, On Fri, 06 May 2016 16:32:53 +0200 Thomas Monjalon <thomas.monjalon at 6wind.com> wrote:
> It looks a lot too much tricky to be integrated without code comments ;) > Please make a documentation effort. I know it's not the brightly shining code... I focused mainly on the C API of this. Thanks a lot for comments. I will fix those and come back with something better. > > 2016-05-06 12:48, Jan Viktorin: > > --- a/app/test/Makefile > > +++ b/app/test/Makefile > > @@ -33,6 +33,37 @@ include $(RTE_SDK)/mk/rte.vars.mk > > > > ifeq ($(CONFIG_RTE_APP_TEST),y) > > A comment is needed here to explain the intent of the following code. Sure. > > > +ifeq ($(RTE_ARCH),arm) > > +RTE_OBJCOPY_O = elf32-littlearm > > +RTE_OBJCOPY_B = arm > > +else ifeq ($(RTE_ARCH),arm64) > > +RTE_OBJCOPY_O = elf64-littleaarch64 > > +RTE_OBJCOPY_B = aarch64 > > +else ifeq ($(RTE_ARCH),i686) > > +RTE_OBJCOPY_O = elf32-i386 > > +RTE_OBJCOPY_B = i386 > > +else ifeq ($(RTE_ARCH),x86_64) > > +RTE_OBJCOPY_O = elf64-x86-64 > > +RTE_OBJCOPY_B = i386:x86-64 > > +else ifeq ($(RTE_ARCH),x86_x32) > > +RTE_OBJCOPY_O = elf32-x86-64 > > +RTE_OBJCOPY_B = i386:x86-64 > > +else > > +$(error Unrecognized RTE_ARCH: $(RTE_ARCH)) > > +endif > > RTE_OBJCOPY_O could be renamed RTE_OBJCOPY_TARGET. > RTE_OBJCOPY_B could be renamed RTE_OBJCOPY_ARCH. > > These definitions could be done in mk/arch/ Cool, I will move those. However, I don't know all the cryptic objcopy names for all platforms. I've tested just arm, arm64 and x86_64. I guessed the rest (or copied from some web page). > > > + > > +define resource > > When defining a makefile macro, the arguments must be documented on > the previous line. Otherwise, nobody (including you) will be able > to read it without parsing the code below. Sure! What about a better name (RTE_TEST_RESOURCE, TEST_RESOURCE)? Should this macro stay in this Makefile? > > It is not a simple resource, so the name must avoid the confusion. > Why not linked_resource? > > > +SRCS-y += $(1).res.o > > +$(1).res.o: $(2) > > + $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_B) -O $(RTE_OBJCOPY_O) \ > > + --rename-section \ > > + .data=.rodata,alloc,load,data,contents,readonly \ > > + --redefine-sym _binary__dev_stdin_start=beg_$(1) \ > > + --redefine-sym _binary__dev_stdin_end=end_$(1) \ > > + --redefine-sym _binary__dev_stdin_size=siz_$(1) \ > > + /dev/stdin $$@ < $$< > > +endef > > [...] > > > +#define REGISTER_LINKED_RESOURCE(_n) \ > > +extern const char beg_ ##_n; \ > > +extern const char end_ ##_n; \ > > +REGISTER_RESOURCE(_n, &beg_ ##_n, &end_ ##_n); \ > > Please explain the begin/end trick. The objcopy creates an object file with our data located at _binary__dev_stdin_start until _binary__dev_stdin_end. The names are derrived from the input file name - this is so so stupid! I used /dev/stdin here to have a deterministic name of the *_start/_end labels. Otherwise, the code would be very messy (trying to santize the aboslute!!! path passed by make). In our C code, we declare extern const char _binary__dev_stdin_start; extern const char _binary__dev_stdin_end; ...variables placed exactly on addresses where our data were put by objcopy. This &_binary__dev_stdin_start is address of our data. Those names are however changed by --redefine-sym to be unique and to reflect the name we use to refer to the data from our C code by REGISTER_LINKED_RESOURCE(<name>). Regards Jan > Thanks -- Jan Viktorin E-mail: Viktorin at RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic
