From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Van Baren Date: Tue, 19 Aug 2008 21:34:43 -0400 Subject: [U-Boot] Location of jump table in global_data structure In-Reply-To: <1219156535.1273.269.camel@localhost.localdomain> References: <1219156535.1273.269.camel@localhost.localdomain> Message-ID: <48AB74B3.5090505@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. ;-) [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