Use specially-aligned types instead of packed structs #1
Conversation
|
Well this is quite a huge change, and it already breaks OSX version, that does not compile anymore after the first patch: Actually on OSX we have: #if LP64 and so your change does not work.... More generally, my feeling is that the whole idea is a bit invasive I also need to be sure it won't break on Windows 64 and 32 bit), so I think we should try it more slowly. Do you think you could restructure the patches a bit? Stéphane Le 22 juil. 2012 à 03:32, basilnut a écrit :
|
|
On Wed, Jul 25, 2012 at 07:17:29AM +0200, Stéphane Letz wrote:
Oh wild. Well, that patch was just there because I didn't want to create
Ok. I've pulled that UInt32 patch out of My tree. Are there any other Thanks, -- Basil |
|
Le 26 juil. 2012 à 00:31, Basil Nutmeg a écrit :
OK, OSX compilation now works again. And mixing 64 server and 32/64 bits clients seems to works correctly on OSX. I'll need to do some more tests. Compilation and tests on Windows have to be done again (probably not before a few days/week...) and ABI change issues have to be tested carefully (especially with the Harisson/Mixbus guys on Windows....) Stéphane |
|
On Fri, Jul 27, 2012 at 12:18:05PM +0200, Stéphane Letz wrote:
Ok, great. Thanks for your help. Let me know anything comes up that I can -- Basil |
The types int64_t, uint64_t, and double have different alignments between common 32-bit and 64-bit platforms, and this can cause trouble when they are used in structs which are shared between address spaces. Introduce new typedefs for these types with special alignment attributes to request consistent alignment. ABI implications: In the current tree, with packed attributes on the relevant structs, the only impact of this change is that the alignments of jack_unique_t and jack_time_t are increased from 4 to 8 on 32-bit platforms, so the layouts of structs which include them as members may be affected. The ABI impact relative to the tree before the packed attributes were introduced is that the alignment of jack_position_t is increased from 4 to 8 on 32-bit platforms. The layout of the struct itself is unmodified, though the layouts of structs which include it as a member may be affected.
Update internal types to have the same layout between 32-bit and 64-bit, using the new alignment macros and types.
Remove use of packed structs, now that the infrastructure for consistently aligning types between 32-bit and 64-bit platforms is in place. This fixes crashes on platforms which enforce alignment rules (because there are places in the code where pointers to packed fields are used). Also, it theoretically improves performance, though I haven't actually performed any measurements. ABI implications: jack_latency_range_t and jack_position_t are no longer packed. The layouts of these structs themselves are unmodified, though the layouts of any structs which include these structs as members may be affected.
Here's my proposed patchset for replacing packed structs with specially aligned types.
I've tested on x86 Linux and confirmed that the structs are compatible between 64-bit and 32-bit. The patch includes changes for MSVC, which I believe should work, but I have not tested it.
The patch includes some minor ABI breakage in jack's public headers; I've described the changes in detail in the commits. This breakage could be avoided, however it would require more complex changes.
I decided to introduce new typedefs only for types which actually differ between 32-bit and 64-bit, rather than all types. This reduced overall churn, though I recognize that it makes it somewhat more likely that someone in the future might add a field to a shared struct and forget to ensure proper alignment.
Let me know if you would like to see anything done differently. Or if you'd like to just forget it altogether I'll understand :-).