On Thursday 31 December 2009 20:39:10 Joakim Tjernlund wrote: > Mike Frysinger <vap...@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 > > > --- 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() > > > --- 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) > 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 ... -mike
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot