> > 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
