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