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

[Jerry Van Baren <gvb.uboot@gmail.com>]

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