[U-Boot] [PATCH 2/4] Use LINK_OFF to access global data

[Joakim Tjernlund]

Mike Frysinger <vapier at gentoo.org> wrote on 01/01/2010 07:18:44:

> From: Mike Frysinger <vapier at gentoo.org>
> To: Joakim Tjernlund <joakim.tjernlund at transmode.se>
> Cc: u-boot at lists.denx.de
> Date: 01/01/2010 07:19
> Subject: Re: [U-Boot] [PATCH 2/4] Use LINK_OFF to access global data
>
> On Thursday 31 December 2009 20:39:10 Joakim Tjernlund wrote:
> > Mike Frysinger <vapier at gentoo.org> wrote on 31/12/2009 19:44:40:
> > > On Wednesday 30 December 2009 10:08:30 Joakim Tjernlund wrote:
> > > > --- a/common/cmd_nvedit.c
> > > > +++ b/common/cmd_nvedit.c
> > > > @@ -512,6 +512,7 @@ char *getenv (char *name)
> > > >  {
> > > >     int i, nxt;
> > > >
> > > > +   name = LINK_OFF(name);
> > > >     WATCHDOG_RESET();
> > > >
> > > >     for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
> > > > @@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len)
> > > >  {
> > > >     int i, nxt;
> > > >
> > > > +   name = LINK_OFF(name);
> > > >     for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
> > > >        int val, n;
> > >
> > > you have no guarantee that getenv() is called with a const string which
> > > is in the .rodata section.  there's code that generates the env name in a
> > > buffer on the stack and gives that to getenv().  does LINK_OFF() still
> > > work then ?
> >
> > True. LINK_OFF will not work iff link addr != load addr and name isn't a
> >  const string. Basically if you want to use the LINK_OFF feature you have
> >  to use a const string.
>
> some of the other functions in these patch sets fall into the same issue ...
> like the output functions

Yes, I wish gcc would have an option to access string literals relative the
text segment so one would not access these via the GOT.

>
> > > > --- a/common/console.c
> > > > +++ b/common/console.c
> > > > @@ -346,7 +346,7 @@ void putc(const char c)
> > > >     }
> > > >  }
> > > >
> > > > -void puts(const char *s)
> > > > +static void printf_puts(const char *s)
> > > >  {
> > > >  #ifdef CONFIG_SILENT_CONSOLE
> > > >     if (gd->flags & GD_FLG_SILENT)
> > > > @@ -367,12 +367,18 @@ void puts(const char *s)
> > > >     }
> > > >  }
> > > >
> > > > +void puts(const char *s)
> > > > +{
> > > > +   printf_puts(LINK_OFF(s));
> > > > +}
> > >
> > > and if CONFIG_LINK_OFF isnt defined, does gcc correctly inline this ?  if
> > > not, i think there needs to be #ifdef CONFIG_LINK_OFF handling here.
> >
> > Possibly, however LINK_OFF is is a NOP if CONFIG_LINK_OFF isn't defined.
>
> yes, but that doesnt mean gcc takes care of inlining all of printf_puts() into
> the puts() and all the new call sites go to puts()

Sure, gcc might not inline the current code in this case. I guess
you don't want the extra size this would incur or is there some else you
are concerned about?

>
> > > > --- a/lib_generic/crc32.c
> > > > +++ b/lib_generic/crc32.c
> > > > @@ -156,6 +156,11 @@
> > > >  */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) {
> > > > +#ifdef LINK_OFF
> > > > +    const uint32_t *crc_tab = LINK_OFF(crc_table);
> > > > +#else
> > > > +    const uint32_t *crc_tab = crc_table;
> > > > +#endif
> > >
> > > the patch 1/4 you posted always defines LINK_OFF.  it's CONFIG_LINK_OFF
> > > which is dynamic.
> >
> > Yes, but LINK_OFF will work too. I can change this though, it looks better.
>
> my point is that it should either be checking CONFIG_LINK_OFF or always using
> LINK_OFF (since it nops when the config is off as you point out)

Yes, I will change this in the next spin.

>
> > The bigger question is if the LINN_OFF changes in general are acceptable to
> >  u-boot. Any board/arch that doesn't want this functionality should not
> >  notice I think.
>
> i dont have any plans on wanting this, and it seems pretty invasive ... and
> easy to introduce new code that breaks PIC people but no one else really
> notices ...

Yes, it is a bit invasive hence the question if this is acceptable to u-boot.
I have been looking for other ways to do this but there isn't one sans modifying
gcc. You are sort of an gcc guy, what do you think the odds are
that gcc would add a few new options mainly useful for smaller
embedded progs like u-boot (and possible the kernel too)?

If this is accepted a few boards could be changed to use this new function by
default and then one would detect breakage earlier.

    Jocke