[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