[U-Boot] Location of jump table in global_data structure

[U-Boot] Location of jump table in global_data structure

[Peter Tyser]

Hello,
I've noticed that the jump table pointer (**jt) in the global_data
structure is always the last field in the structure.  When standalone
applications are compiled, they hard code the jump table pointer offset
into the global_data structure.  When new versions of U-Boot come out
which add/remove a field from the global_data structure, old standalone
applications will no longer work as the location of the jt pointer has
changed.  I've noticed this issue when updating U-Boot from 1.3.0 to
1.3.4.

As an example from include/asm-avr32/global_data.h:

FROM VERSION 1.3.4:
typedef struct  global_data {
        bd_t            *bd;
        unsigned long   flags;
        unsigned long   baudrate;
        unsigned long   stack_end;      /* highest stack address */
        unsigned long   have_console;   /* serial_init() was called */
        unsigned long   reloc_off;      /* Relocation Offset */
        unsigned long   env_addr;       /* Address of env struct */
        unsigned long   env_valid;      /* Checksum of env valid? */
        unsigned long   cpu_hz;         /* cpu core clock frequency */
        void            **jt;           /* jump table */
} gd_t;

?FROM FUTURE VERSION 1.3.5:
?typedef struct  global_data {
        bd_t            *bd;
        unsigned long   flags;
        unsigned long   baudrate;
        unsigned long   stack_end;      /* highest stack address */
        unsigned long   have_console;   /* serial_init() was called */
        unsigned long   reloc_off;      /* Relocation Offset */
        unsigned long   env_addr;       /* Address of env struct */
        unsigned long   env_valid;      /* Checksum of env valid? */
        unsigned long   cpu_hz;         /* cpu core clock frequency */
====>   unsigned long	fancy_value;	/* FANCY NEW VALUE ADDED!! */
        void            **jt;           /* jump table */
} gd_t;


Adding fancy_value to global_data would break any standalone
applications compiled for 1.3.4 since the app is now using fancy_value
as its jump table pointer (where jt used to be).


One possible fix would be to move **jt to the 2nd item in global_data to
prevent it moving in the future.  This would break everyone's current
standalone apps however:) eg:
?typedef struct  global_data {
        bd_t            *bd;
====>   void            **jt;           /* jump table */
        unsigned long   flags;
        unsigned long   baudrate;
        unsigned long   stack_end;      /* highest stack address */
        unsigned long   have_console;   /* serial_init() was called */
        unsigned long   reloc_off;      /* Relocation Offset */
        unsigned long   env_addr;       /* Address of env struct */
        unsigned long   env_valid;      /* Checksum of env valid? */
        unsigned long   cpu_hz;         /* cpu core clock frequency */
} gd_t;


Another option would be to mandate that new fields only be added after
the **jt field to prevent it from moving, although this would be hard to
enforce and seems a bit hokey.


Do others view this issue as a problem that should be fixed?


If others feel that the jt pointer should be moved to the 2nd item in
global_data structure let me know and I can generate a patch.

Best,
Peter

[U-Boot] Location of jump table in global_data structure

[Jerry Van Baren]

Peter Tyser wrote:
> Hello,
> I've noticed that the jump table pointer (**jt) in the global_data
> structure is always the last field in the structure.  When standalone
> applications are compiled, they hard code the jump table pointer offset
> into the global_data structure.  When new versions of U-Boot come out
> which add/remove a field from the global_data structure, old standalone
> applications will no longer work as the location of the jt pointer has
> changed.  I've noticed this issue when updating U-Boot from 1.3.0 to
> 1.3.4.

It seems to me to be very broken that the contents an interface 
definition would shift from version to version.  IMHO, unless there are 
unassailable reasons, new values should *always* be appended to the 
struct so that the struct is backwards compatible to previous versions.

Maybe we need to upgrade our interface to a flattened device tree to 
avoid the horrible interface-as-a-struct layout problem.
   <http://www.brainyquote.com/quotes/quotes/b/bernardbar181387.html> ;-)

[snip]

> ?FROM FUTURE VERSION 1.3.5:
> ?typedef struct  global_data {
>         bd_t            *bd;
>         unsigned long   flags;
>         unsigned long   baudrate;
>         unsigned long   stack_end;      /* highest stack address */
>         unsigned long   have_console;   /* serial_init() was called */
>         unsigned long   reloc_off;      /* Relocation Offset */
>         unsigned long   env_addr;       /* Address of env struct */
>         unsigned long   env_valid;      /* Checksum of env valid? */
>         unsigned long   cpu_hz;         /* cpu core clock frequency */
> ====>   unsigned long	fancy_value;	/* FANCY NEW VALUE ADDED!! */
>         void            **jt;           /* jump table */
> } gd_t;

This addition is broken IMHO.

> One possible fix would be to move **jt to the 2nd item in global_data to
> prevent it moving in the future.  This would break everyone's current
> standalone apps however:) eg:
> ?typedef struct  global_data {
>         bd_t            *bd;
> ====>   void            **jt;           /* jump table */
>         unsigned long   flags;
>         unsigned long   baudrate;
>         unsigned long   stack_end;      /* highest stack address */
>         unsigned long   have_console;   /* serial_init() was called */
>         unsigned long   reloc_off;      /* Relocation Offset */
>         unsigned long   env_addr;       /* Address of env struct */
>         unsigned long   env_valid;      /* Checksum of env valid? */
>         unsigned long   cpu_hz;         /* cpu core clock frequency */
> } gd_t;

That only "fixes" the jump table reference.  If someone adds fancy_value 
after baudrate, it still will break backwards compatibility (maybe not 
visibly, maybe not immediately, maybe not for a given application, but 
it still is broken).

> Another option would be to mandate that new fields only be added after
> the **jt field to prevent it from moving, although this would be hard to
> enforce and seems a bit hokey.

No, only append new fields to the end of the struct (adding fields after 
**jt only fixes the problem for the first new field ;-).  The correct 
rule is to never add fields in the middle of the struct.

An instructive comment should go a long way and we have some pretty 
eagle-eyed code reviewers on the mailing list that should go the rest of 
the way.

> Do others view this issue as a problem that should be fixed?

Yes.

> If others feel that the jt pointer should be moved to the 2nd item in
> global_data structure let me know and I can generate a patch.

Add a comment and police it is my vote.

> Best,
> Peter

Thanks,
gvb

[U-Boot] Location of jump table in global_data structure

[Peter Tyser]

> > I've noticed that the jump table pointer (**jt) in the global_data
> > structure is always the last field in the structure.  When standalone
> > applications are compiled, they hard code the jump table pointer offset
> > into the global_data structure.  When new versions of U-Boot come out
> > which add/remove a field from the global_data structure, old standalone
> > applications will no longer work as the location of the jt pointer has
> > changed.  I've noticed this issue when updating U-Boot from 1.3.0 to
> > 1.3.4.
> 
> It seems to me to be very broken that the contents an interface 
> definition would shift from version to version.  IMHO, unless there are 
> unassailable reasons, new values should *always* be appended to the 
> struct so that the struct is backwards compatible to previous versions.
> 
> Maybe we need to upgrade our interface to a flattened device tree to 
> avoid the horrible interface-as-a-struct layout problem.
>    <http://www.brainyquote.com/quotes/quotes/b/bernardbar181387.html> ;-)

Great quote, very fitting:)

> [snip]
> 
> > ?FROM FUTURE VERSION 1.3.5:
> > ?typedef struct  global_data {
> >         bd_t            *bd;
> >         unsigned long   flags;
> >         unsigned long   baudrate;
> >         unsigned long   stack_end;      /* highest stack address */
> >         unsigned long   have_console;   /* serial_init() was called */
> >         unsigned long   reloc_off;      /* Relocation Offset */
> >         unsigned long   env_addr;       /* Address of env struct */
> >         unsigned long   env_valid;      /* Checksum of env valid? */
> >         unsigned long   cpu_hz;         /* cpu core clock frequency */
> > ====>   unsigned long	fancy_value;	/* FANCY NEW VALUE ADDED!! */
> >         void            **jt;           /* jump table */
> > } gd_t;
> 
> This addition is broken IMHO.
> > One possible fix would be to move **jt to the 2nd item in global_data to
> > prevent it moving in the future.  This would break everyone's current
> > standalone apps however:) eg:
> > ?typedef struct  global_data {
> >         bd_t            *bd;
> > ====>   void            **jt;           /* jump table */
> >         unsigned long   flags;
> >         unsigned long   baudrate;
> >         unsigned long   stack_end;      /* highest stack address */
> >         unsigned long   have_console;   /* serial_init() was called */
> >         unsigned long   reloc_off;      /* Relocation Offset */
> >         unsigned long   env_addr;       /* Address of env struct */
> >         unsigned long   env_valid;      /* Checksum of env valid? */
> >         unsigned long   cpu_hz;         /* cpu core clock frequency */
> > } gd_t;
> 
> That only "fixes" the jump table reference.  If someone adds fancy_value 
> after baudrate, it still will break backwards compatibility (maybe not 
> visibly, maybe not immediately, maybe not for a given application, but 
> it still is broken).
> > Another option would be to mandate that new fields only be added after
> > the **jt field to prevent it from moving, although this would be hard to
> > enforce and seems a bit hokey.
> 
> No, only append new fields to the end of the struct (adding fields after 
> **jt only fixes the problem for the first new field ;-).  The correct 
> rule is to never add fields in the middle of the struct.
> 
> An instructive comment should go a long way and we have some pretty 
> eagle-eyed code reviewers on the mailing list that should go the rest of 
> the way.

The one large downside of mandating that fields only be added to the end
of the struct is that a field can never be removed from the global_data
struct.  I have to imagine fields will be removed at some point...

> > Do others view this issue as a problem that should be fixed?
> 
> Yes.
> 
> > If others feel that the jt pointer should be moved to the 2nd item in
> > global_data structure let me know and I can generate a patch.
> 
> Add a comment and police it is my vote.

That's definitely an improvement, but doesn't handle both
adding/removing fields from the global_data structure in a clean manner.
I'd still lean towards moving the jt pointer to one of the earlier
fields of the structs as well as adding a comment.  Then at least the
jump table portion of the API would be stable, even if accessing the
"global_data fields" API wouldn't.  Right now, neither API is stable:)

If anyone has any other clever ideas on improving the API, I'd be happy
to investigate/implement.

Best,
Peter

[U-Boot] Location of jump table in global_data structure

[Rafal Jaworowski]

Peter Tyser wrote:

*snip*

>>> Do others view this issue as a problem that should be fixed?
>> Yes.
>>
>>> If others feel that the jt pointer should be moved to the 2nd item in
>>> global_data structure let me know and I can generate a patch.
>> Add a comment and police it is my vote.
> 
> That's definitely an improvement, but doesn't handle both
> adding/removing fields from the global_data structure in a clean manner.
> I'd still lean towards moving the jt pointer to one of the earlier
> fields of the structs as well as adding a comment.  Then at least the
> jump table portion of the API would be stable, even if accessing the
> "global_data fields" API wouldn't.  Right now, neither API is stable:)
> 
> If anyone has any other clever ideas on improving the API, I'd be happy
> to investigate/implement.

There already is a modern API for external applications in U-Boot since 1.3.2
(but not enabled by default). Please see 'api/README' and 'api_examples' in
the U-Boot tree.

So far it has been used successfully on PowerPC and ARM; there's no support
for AVR arch yet, but providing the syscall entry point for it shouldn't be a
big deal for someone with AVR knowledge. Let me know if you'd be willing to
chew on it (I don't have AVR h/w but can help/support regarding the API itself).

kind regards,
Rafal