Re: [dpdk-dev] [PATCH v1 02/10] app/test: support resources externally linked

[Jan Viktorin <viktorin@rehivetech.com>]

Hello Thomas,

On Fri, 06 May 2016 16:32:53 +0200
Thomas Monjalon <thomas.monjalon@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@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic