GooString has 3 fields: char sStatic[STR_STATIC_SIZE]; int length; char *s; 32-bit system without the patches: STR_STATIC_SIZE: 24 sizeof(MemoryLayout): 12 sizeof(char*): 4 sizeof(int): 4 sizeof(GooString): 32 ------------------------ Used = STR_STATIC_SIZE + sizeof(int) + sizeof(char*) = 24 + 4 + 4 = 32 out of 32 taken (100% efficiency) 64-bit system without the patches: STR_STATIC_SIZE: 16 sizeof(MemoryLayout): 24 sizeof(char*): 8 sizeof(int): 4 sizeof(GooString): 32 ------------------------ Used = STR_STATIC_SIZE + sizeof(int) + sizeof(char*) = 16 + 4 + 8 = 28 out of 32 taken (87.5% efficiency) Right now STR_STATIC_SIZE is calculated using class MemoryLayout which doesn't help, but only makes it harder due to memory alignment performed by the compiler (sizeof(MemoryLayout) on a 64-bit system >= the sum of MemoryLayout's fields sizes). With my patches sizeof(GooString) is still == 32, and there are no wasted bytes.
Created attachment 126067 [details] [review] [PATCH 1/2] goo: fix GooString::STR_STATIC_SIZE calculation
Created attachment 126068 [details] [review] [PATCH 2/2] goo: check at compile time if GooString has the right size This patch makes it harder to introduce bugs related to GooString size. (Wouldn't really prevent this one, because I think this one couldn't be automatically detected without a special static code analyzer or something like that.)
If the current behavior is intended to keep length at aligned offset, then maybe GooString could be made bigger, because 16 bytes for sStatic on a 64-bit system (they usually have a bit more RAM than 32-bit systems, so it could be made use of) seems a bit small to me. I've tried that by defining STR_FINAL_SIZE as: static const int STR_FINAL_SIZE = 8 * sizeof(char*); // 32 on a 32-bit system, 64 on a 64-bit one , but for some reason it breaks PDFDoc construction in PDFDocFactory (a problem in a "new" operator function - not sure which though). If you say the above line is desirable, then I could take some time and debug the issue.
elf-dissector disagrees with your patch class GooString // location: /home/tsdgeos/devel/poppler/goo/GooString.h:49 { char[36] sStatic; // member offset: 0, size: 36, alignment: 1 int length; // member offset: 36, size: 4, alignment: 4 char* s; // member offset: 40, size: 8, alignment: 8 }; // size: 48, alignment: 8 Used bytes: 48/48 (100%) Used bits: 384/384 (100%) This is with your patch, GooString is now 48 bytes instead of 32. but agrees with your bug (without the patch it says) class GooString // location: /home/tsdgeos/devel/poppler/goo/GooString.h:49 { char[16] sStatic; // member offset: 0, size: 16, alignment: 1 int length; // member offset: 16, size: 4, alignment: 4 // 4 byte(s) padding char* s; // member offset: 24, size: 8, alignment: 8 }; // size: 32, alignment: 8 Used bytes: 28/32 (87.5%) Used bits: 224/256 (87.5%)
Created attachment 126076 [details] [review] [PATCH] check GooString size (only for 64-bit platforms)
(In reply to Albert Astals Cid from comment #4) > elf-dissector disagrees with your patch > > class GooString // location: /home/tsdgeos/devel/poppler/goo/GooString.h:49 > { > char[36] sStatic; // member offset: 0, size: 36, alignment: 1 > int length; // member offset: 36, size: 4, alignment: 4 > char* s; // member offset: 40, size: 8, alignment: 8 > }; // size: 48, alignment: 8 > Used bytes: 48/48 (100%) > Used bits: 384/384 (100%) > > This is with your patch, GooString is now 48 bytes instead of 32. > > but agrees with your bug (without the patch it says) > > class GooString // location: /home/tsdgeos/devel/poppler/goo/GooString.h:49 > { > char[16] sStatic; // member offset: 0, size: 16, alignment: 1 > int length; // member offset: 16, size: 4, alignment: 4 > // 4 byte(s) padding > char* s; // member offset: 24, size: 8, alignment: 8 > }; // size: 32, alignment: 8 > > Used bytes: 28/32 (87.5%) > Used bits: 224/256 (87.5%) Are you sure elf-dissector is right? I don't know this tool, but when I run something with poppler with the above patch, it doesn't complain.
Well, it's following your math, no? static const int STR_STATIC_SIZE = STR_FINAL_SIZE - sizeof(int) - sizeof(char*); so STR_STATIC_SIZE = 32 - 4 + 8 = 36 so if STR_STATIC_SIZE is already 36 the whole thing will be bigger than 32, no?
(In reply to Albert Astals Cid from comment #7) > Well, it's following your math, no? > > static const int STR_STATIC_SIZE = STR_FINAL_SIZE - sizeof(int) - > sizeof(char*); > so > STR_STATIC_SIZE = 32 - 4 + 8 = 36 > so if STR_STATIC_SIZE is already 36 the whole thing will be bigger than 32, > no? No. There is no addition. STR_STATIC_SIZE = 32 - 4 - 8 = 20. My only worry here is performance (due to memory alignment). Benchmarks could help, but recently I don't have time for figuring out how to properly benchmark it (or for anything else for that matter). So I suggest letting it hang here until someone, who has time for that, benchmarks it. Cheers, Jakub
Sorry that's my fault for being silly and trying to apply the patch manually. I'll try to go over this soon but given i'm taking a 3 week holiday away from a computer starting on friday i'm not sure i'll manage.
pushed
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.