{"thread":{"id":"43132","subject":"Re: sizeof(struct ...)","startedAt":"2006-11-23T10:16:09Z","lastAt":"2006-11-24T08:53:57Z","messageCount":13,"participants":["Andy Whitcroft","Junio C Hamano","René Scharfe","Gerrit Pape","Erik Mouw"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"296923","messageId":"20061123101609.1711.qmail@8b73034525b1a6.315fe32.mid.smarden.org","threadId":"43132","inReplyTo":null,"subject":"sizeof(struct ...)","fromName":"Gerrit Pape","fromEmail":"pape@smarden.org","sentAt":"2006-11-23T10:16:09Z","receivedAt":"2006-11-23T10:16:09Z","isPatch":false,"sender":{"key":"pape@smarden.org","avatar":"https://avatars.githubusercontent.com/u/143170252?v=4"},"body":"Hi, I don't think we can rely on sizeof(struct ...) to be the exact size\nof the struct as defined.  As the selftests show, archive-zip doesn't\nwork correctly on Debian/arm\n\n http://buildd.debian.org/fetch.cgi?&pkg=git-core&ver=1%3A1.4.4-1&arch=arm&stamp=1164122355&file=log\n\nIt's because sizeof(struct zip_local_header) is 32, zip_dir_header 48,\nand zip_dir_trailer 24, breaking the zip files.  Compiling with\n-fpack-struct seemed to break other things, so I for now I ended up with\nthis (not so nice) workaround.\n\nRegards, Gerrit.\n\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex ae5572a..4fcda44 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -211,7 +211,7 @@ static int write_zip_entry(const unsigne\n \t}\n \n \t/* make sure we have enough free space in the dictionary */\n-\tdirentsize = sizeof(struct zip_dir_header) + pathlen;\n+\tdirentsize = 46 + pathlen;\n \twhile (zip_dir_size < zip_dir_offset + direntsize) {\n \t\tzip_dir_size += ZIP_DIRECTORY_MIN_SIZE;\n \t\tzip_dir = xrealloc(zip_dir, zip_dir_size);\n@@ -234,8 +234,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le16(dirent.attr1, 0);\n \tcopy_le32(dirent.attr2, attr2);\n \tcopy_le32(dirent.offset, zip_offset);\n-\tmemcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));\n-\tzip_dir_offset += sizeof(struct zip_dir_header);\n+\tmemcpy(zip_dir + zip_dir_offset, &dirent, 46);\n+\tzip_dir_offset += 46;\n \tmemcpy(zip_dir + zip_dir_offset, path, pathlen);\n \tzip_dir_offset += pathlen;\n \tzip_dir_entries++;\n@@ -251,8 +251,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le32(header.size, uncompressed_size);\n \tcopy_le16(header.filename_length, pathlen);\n \tcopy_le16(header.extra_length, 0);\n-\twrite_or_die(1, &header, sizeof(struct zip_local_header));\n-\tzip_offset += sizeof(struct zip_local_header);\n+\twrite_or_die(1, &header, 30);\n+\tzip_offset += 30;\n \twrite_or_die(1, path, pathlen);\n \tzip_offset += pathlen;\n \tif (compressed_size > 0) {\n@@ -282,7 +282,7 @@ static void write_zip_trailer(const unsi\n \tcopy_le16(trailer.comment_length, sha1 ? 40 : 0);\n \n \twrite_or_die(1, zip_dir, zip_dir_offset);\n-\twrite_or_die(1, &trailer, sizeof(struct zip_dir_trailer));\n+\twrite_or_die(1, &trailer, 22);\n \tif (sha1)\n \t\twrite_or_die(1, sha1_to_hex(sha1), 40);\n"},{"id":"296671","messageId":"45659781.5050005@lsrfire.ath.cx","threadId":"43132","inReplyTo":"20061123101609.1711.qmail@8b73034525b1a6.315fe32.mid.smarden.org","subject":"Re: sizeof(struct ...)","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2006-11-23T12:43:45Z","receivedAt":"2006-11-23T12:43:45Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Gerrit Pape schrieb:\n> Hi, I don't think we can rely on sizeof(struct ...) to be the exact size\n> of the struct as defined.  As the selftests show, archive-zip doesn't\n> work correctly on Debian/arm\n> \n>  http://buildd.debian.org/fetch.cgi?&pkg=git-core&ver=1%3A1.4.4-1&arch=arm&stamp=1164122355&file=log\n> \n> It's because sizeof(struct zip_local_header) is 32, zip_dir_header 48,\n> and zip_dir_trailer 24, breaking the zip files.  Compiling with\n> -fpack-struct seemed to break other things, so I for now I ended up with\n> this (not so nice) workaround.\n\nHm, yes, this use sizeof() is not strictly correct.  But I'd very much\nlike to keep being lazy and let the compiler to do the summing.  How\nabout this patch instead?  Does it work for you, Gerrit?\n\nThanks,\nRené\n\n\n archive-zip.c |   24 ++++++++++++++++++------\n 1 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex ae5572a..4caaec4 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -35,6 +35,7 @@ struct zip_local_header {\n \tunsigned char size[4];\n \tunsigned char filename_length[2];\n \tunsigned char extra_length[2];\n+\tunsigned char _end[0];\n };\n \n struct zip_dir_header {\n@@ -55,6 +56,7 @@ struct zip_dir_header {\n \tunsigned char attr1[2];\n \tunsigned char attr2[4];\n \tunsigned char offset[4];\n+\tunsigned char _end[0];\n };\n \n struct zip_dir_trailer {\n@@ -66,8 +68,18 @@ struct zip_dir_trailer {\n \tunsigned char size[4];\n \tunsigned char offset[4];\n \tunsigned char comment_length[2];\n+\tunsigned char _end[0];\n };\n \n+/*\n+ * On ARM, padding is added at the end of the struct, so a simple\n+ * sizeof(struct ...) reports two bytes more than the payload size\n+ * we're interested in.\n+ */\n+#define ZIP_LOCAL_HEADER_SIZE\toffsetof(struct zip_local_header, _end)\n+#define ZIP_DIR_HEADER_SIZE\toffsetof(struct zip_dir_header, _end)\n+#define ZIP_DIR_TRAILER_SIZE\toffsetof(struct zip_dir_trailer, _end)\n+\n static void copy_le16(unsigned char *dest, unsigned int n)\n {\n \tdest[0] = 0xff & n;\n@@ -211,7 +223,7 @@ static int write_zip_entry(const unsigne\n \t}\n \n \t/* make sure we have enough free space in the dictionary */\n-\tdirentsize = sizeof(struct zip_dir_header) + pathlen;\n+\tdirentsize = ZIP_DIR_HEADER_SIZE + pathlen;\n \twhile (zip_dir_size < zip_dir_offset + direntsize) {\n \t\tzip_dir_size += ZIP_DIRECTORY_MIN_SIZE;\n \t\tzip_dir = xrealloc(zip_dir, zip_dir_size);\n@@ -234,8 +246,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le16(dirent.attr1, 0);\n \tcopy_le32(dirent.attr2, attr2);\n \tcopy_le32(dirent.offset, zip_offset);\n-\tmemcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));\n-\tzip_dir_offset += sizeof(struct zip_dir_header);\n+\tmemcpy(zip_dir + zip_dir_offset, &dirent, ZIP_DIR_HEADER_SIZE);\n+\tzip_dir_offset += ZIP_DIR_HEADER_SIZE;\n \tmemcpy(zip_dir + zip_dir_offset, path, pathlen);\n \tzip_dir_offset += pathlen;\n \tzip_dir_entries++;\n@@ -251,8 +263,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le32(header.size, uncompressed_size);\n \tcopy_le16(header.filename_length, pathlen);\n \tcopy_le16(header.extra_length, 0);\n-\twrite_or_die(1, &header, sizeof(struct zip_local_header));\n-\tzip_offset += sizeof(struct zip_local_header);\n+\twrite_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);\n+\tzip_offset += ZIP_LOCAL_HEADER_SIZE;\n \twrite_or_die(1, path, pathlen);\n \tzip_offset += pathlen;\n \tif (compressed_size > 0) {\n@@ -282,7 +294,7 @@ static void write_zip_trailer(const unsi\n \tcopy_le16(trailer.comment_length, sha1 ? 40 : 0);\n \n \twrite_or_die(1, zip_dir, zip_dir_offset);\n-\twrite_or_die(1, &trailer, sizeof(struct zip_dir_trailer));\n+\twrite_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);\n \tif (sha1)\n \t\twrite_or_die(1, sha1_to_hex(sha1), 40);\n"},{"id":"294572","messageId":"4565A46C.6090805@lsrfire.ath.cx","threadId":"43132","inReplyTo":"45659781.5050005@lsrfire.ath.cx","subject":"Re: sizeof(struct ...)","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2006-11-23T13:38:52Z","receivedAt":"2006-11-23T13:38:52Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Oh no, whitespace damage!  The major downside of getting a new\nmachine is that Thunderbird needs to be beaten into submission\nagain. :-(  Here's the patch a second time, hopefully intact.\n\nRené\n\n\n archive-zip.c |   24 ++++++++++++++++++------\n 1 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex ae5572a..4caaec4 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -35,6 +35,7 @@ struct zip_local_header {\n \tunsigned char size[4];\n \tunsigned char filename_length[2];\n \tunsigned char extra_length[2];\n+\tunsigned char _end[0];\n };\n \n struct zip_dir_header {\n@@ -55,6 +56,7 @@ struct zip_dir_header {\n \tunsigned char attr1[2];\n \tunsigned char attr2[4];\n \tunsigned char offset[4];\n+\tunsigned char _end[0];\n };\n \n struct zip_dir_trailer {\n@@ -66,8 +68,18 @@ struct zip_dir_trailer {\n \tunsigned char size[4];\n \tunsigned char offset[4];\n \tunsigned char comment_length[2];\n+\tunsigned char _end[0];\n };\n \n+/*\n+ * On ARM, padding is added at the end of the struct, so a simple\n+ * sizeof(struct ...) reports two bytes more than the payload size\n+ * we're interested in.\n+ */\n+#define ZIP_LOCAL_HEADER_SIZE\toffsetof(struct zip_local_header, _end)\n+#define ZIP_DIR_HEADER_SIZE\toffsetof(struct zip_dir_header, _end)\n+#define ZIP_DIR_TRAILER_SIZE\toffsetof(struct zip_dir_trailer, _end)\n+\n static void copy_le16(unsigned char *dest, unsigned int n)\n {\n \tdest[0] = 0xff & n;\n@@ -211,7 +223,7 @@ static int write_zip_entry(const unsigne\n \t}\n \n \t/* make sure we have enough free space in the dictionary */\n-\tdirentsize = sizeof(struct zip_dir_header) + pathlen;\n+\tdirentsize = ZIP_DIR_HEADER_SIZE + pathlen;\n \twhile (zip_dir_size < zip_dir_offset + direntsize) {\n \t\tzip_dir_size += ZIP_DIRECTORY_MIN_SIZE;\n \t\tzip_dir = xrealloc(zip_dir, zip_dir_size);\n@@ -234,8 +246,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le16(dirent.attr1, 0);\n \tcopy_le32(dirent.attr2, attr2);\n \tcopy_le32(dirent.offset, zip_offset);\n-\tmemcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));\n-\tzip_dir_offset += sizeof(struct zip_dir_header);\n+\tmemcpy(zip_dir + zip_dir_offset, &dirent, ZIP_DIR_HEADER_SIZE);\n+\tzip_dir_offset += ZIP_DIR_HEADER_SIZE;\n \tmemcpy(zip_dir + zip_dir_offset, path, pathlen);\n \tzip_dir_offset += pathlen;\n \tzip_dir_entries++;\n@@ -251,8 +263,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le32(header.size, uncompressed_size);\n \tcopy_le16(header.filename_length, pathlen);\n \tcopy_le16(header.extra_length, 0);\n-\twrite_or_die(1, &header, sizeof(struct zip_local_header));\n-\tzip_offset += sizeof(struct zip_local_header);\n+\twrite_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);\n+\tzip_offset += ZIP_LOCAL_HEADER_SIZE;\n \twrite_or_die(1, path, pathlen);\n \tzip_offset += pathlen;\n \tif (compressed_size > 0) {\n@@ -282,7 +294,7 @@ static void write_zip_trailer(const unsi\n \tcopy_le16(trailer.comment_length, sha1 ? 40 : 0);\n \n \twrite_or_die(1, zip_dir, zip_dir_offset);\n-\twrite_or_die(1, &trailer, sizeof(struct zip_dir_trailer));\n+\twrite_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);\n \tif (sha1)\n \t\twrite_or_die(1, sha1_to_hex(sha1), 40);\n"},{"id":"294498","messageId":"4565A866.8020201@shadowen.org","threadId":"43132","inReplyTo":"4565A46C.6090805@lsrfire.ath.cx","subject":"Re: sizeof(struct ...)","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-11-23T13:55:50Z","receivedAt":"2006-11-23T13:55:50Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"René Scharfe wrote:\n> Oh no, whitespace damage!  The major downside of getting a new\n> machine is that Thunderbird needs to be beaten into submission\n> again. :-(  Here's the patch a second time, hopefully intact.\n> \n> René\n> \n\nThis structure represents an on-disk/on-the-wire thing, should we not be\nspecifying it in some architecture neutral way?  You are going to get\nthe length right in the case of tail padding but not in the face of any\nother padding internally.\n\nYou see packing attributes applied to similar things in the kernel.\nPerhaps they are relevant here?\nIs there not some kind of attribute thing we can apply to this structure\nto prevent the padding?  You see that in the kernel from time to time.\n\nstruct foo {\n} __attribute__((packed));\n\n-apw\n\n> \n>  archive-zip.c |   24 ++++++++++++++++++------\n>  1 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/archive-zip.c b/archive-zip.c\n> index ae5572a..4caaec4 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -35,6 +35,7 @@ struct zip_local_header {\n>  \tunsigned char size[4];\n>  \tunsigned char filename_length[2];\n>  \tunsigned char extra_length[2];\n> +\tunsigned char _end[0];\n>  };\n>  \n>  struct zip_dir_header {\n> @@ -55,6 +56,7 @@ struct zip_dir_header {\n>  \tunsigned char attr1[2];\n>  \tunsigned char attr2[4];\n>  \tunsigned char offset[4];\n> +\tunsigned char _end[0];\n>  };\n>  \n>  struct zip_dir_trailer {\n> @@ -66,8 +68,18 @@ struct zip_dir_trailer {\n>  \tunsigned char size[4];\n>  \tunsigned char offset[4];\n>  \tunsigned char comment_length[2];\n> +\tunsigned char _end[0];\n>  };\n>  \n> +/*\n> + * On ARM, padding is added at the end of the struct, so a simple\n> + * sizeof(struct ...) reports two bytes more than the payload size\n> + * we're interested in.\n> + */\n> +#define ZIP_LOCAL_HEADER_SIZE\toffsetof(struct zip_local_header, _end)\n> +#define ZIP_DIR_HEADER_SIZE\toffsetof(struct zip_dir_header, _end)\n> +#define ZIP_DIR_TRAILER_SIZE\toffsetof(struct zip_dir_trailer, _end)\n> +\n>  static void copy_le16(unsigned char *dest, unsigned int n)\n>  {\n>  \tdest[0] = 0xff & n;\n> @@ -211,7 +223,7 @@ static int write_zip_entry(const unsigne\n>  \t}\n>  \n>  \t/* make sure we have enough free space in the dictionary */\n> -\tdirentsize = sizeof(struct zip_dir_header) + pathlen;\n> +\tdirentsize = ZIP_DIR_HEADER_SIZE + pathlen;\n>  \twhile (zip_dir_size < zip_dir_offset + direntsize) {\n>  \t\tzip_dir_size += ZIP_DIRECTORY_MIN_SIZE;\n>  \t\tzip_dir = xrealloc(zip_dir, zip_dir_size);\n> @@ -234,8 +246,8 @@ static int write_zip_entry(const unsigne\n>  \tcopy_le16(dirent.attr1, 0);\n>  \tcopy_le32(dirent.attr2, attr2);\n>  \tcopy_le32(dirent.offset, zip_offset);\n> -\tmemcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));\n> -\tzip_dir_offset += sizeof(struct zip_dir_header);\n> +\tmemcpy(zip_dir + zip_dir_offset, &dirent, ZIP_DIR_HEADER_SIZE);\n> +\tzip_dir_offset += ZIP_DIR_HEADER_SIZE;\n>  \tmemcpy(zip_dir + zip_dir_offset, path, pathlen);\n>  \tzip_dir_offset += pathlen;\n>  \tzip_dir_entries++;\n> @@ -251,8 +263,8 @@ static int write_zip_entry(const unsigne\n>  \tcopy_le32(header.size, uncompressed_size);\n>  \tcopy_le16(header.filename_length, pathlen);\n>  \tcopy_le16(header.extra_length, 0);\n> -\twrite_or_die(1, &header, sizeof(struct zip_local_header));\n> -\tzip_offset += sizeof(struct zip_local_header);\n> +\twrite_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);\n> +\tzip_offset += ZIP_LOCAL_HEADER_SIZE;\n>  \twrite_or_die(1, path, pathlen);\n>  \tzip_offset += pathlen;\n>  \tif (compressed_size > 0) {\n> @@ -282,7 +294,7 @@ static void write_zip_trailer(const unsi\n>  \tcopy_le16(trailer.comment_length, sha1 ? 40 : 0);\n>  \n>  \twrite_or_die(1, zip_dir, zip_dir_offset);\n> -\twrite_or_die(1, &trailer, sizeof(struct zip_dir_trailer));\n> +\twrite_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);\n>  \tif (sha1)\n>  \t\twrite_or_die(1, sha1_to_hex(sha1), 40);\n>  }\n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"295916","messageId":"4565C205.8050300@lsrfire.ath.cx","threadId":"43132","inReplyTo":"4565A866.8020201@shadowen.org","subject":"Re: sizeof(struct ...)","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2006-11-23T15:45:09Z","receivedAt":"2006-11-23T15:45:09Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Andy Whitcroft schrieb:\n> This structure represents an on-disk/on-the-wire thing, should we not be\n> specifying it in some architecture neutral way?  You are going to get\n> the length right in the case of tail padding but not in the face of any\n> other padding internally.\n> \n> You see packing attributes applied to similar things in the kernel.\n> Perhaps they are relevant here?\n> Is there not some kind of attribute thing we can apply to this structure\n> to prevent the padding?  You see that in the kernel from time to time.\n> \n> struct foo {\n> } __attribute__((packed));\n\nYes, that would be nice, but unfortunately __attribute__ is no standard\nC.  Is there really a compiler that inserts padding between arrays of\nunsigned chars?\n\nThanks,\n"},{"id":"296652","messageId":"20061123155431.GD6581@harddisk-recovery.com","threadId":"43132","inReplyTo":"4565C205.8050300@lsrfire.ath.cx","subject":"Re: sizeof(struct ...)","fromName":"Erik Mouw","fromEmail":"erik@harddisk-recovery.com","sentAt":"2006-11-23T15:54:31Z","receivedAt":"2006-11-23T15:54:31Z","isPatch":false,"sender":{"key":"erik@harddisk-recovery.com","avatar":null},"body":"On Thu, Nov 23, 2006 at 04:45:09PM +0100, Ren? Scharfe wrote:\n> Andy Whitcroft schrieb:\n> > You see packing attributes applied to similar things in the kernel.\n> > Perhaps they are relevant here?\n> > Is there not some kind of attribute thing we can apply to this structure\n> > to prevent the padding?  You see that in the kernel from time to time.\n> > \n> > struct foo {\n> > } __attribute__((packed));\n> \n> Yes, that would be nice, but unfortunately __attribute__ is no standard\n> C.\n\nThere is no standard C way to pack structures. Some compilers use\n#pragma's, gcc uses __attribute__((packed)).\n\n>  Is there really a compiler that inserts padding between arrays of\n> unsigned chars?\n\nYes, that compiler is called \"gcc\".\n\n#include <stdio.h>\n\nstruct foo {\n        unsigned char a[3];\n        unsigned char b[3];\n};\n\nint main(void)\n{\n        printf(\"%d\\n\", sizeof(struct foo));\n        return 0;\n}\n\nOn i386 that prints 6, on ARM it prints 8.\n\n\nErik\n\n-- \n+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --\n"},{"id":"298615","messageId":"4565C8F4.6000606@lsrfire.ath.cx","threadId":"43132","inReplyTo":"20061123155431.GD6581@harddisk-recovery.com","subject":"Re: sizeof(struct ...)","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2006-11-23T16:14:44Z","receivedAt":"2006-11-23T16:14:44Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Erik Mouw schrieb:\n> On Thu, Nov 23, 2006 at 04:45:09PM +0100, René Scharfe wrote:\n>>  Is there really a compiler that inserts padding between arrays of\n>> unsigned chars?\n> \n> Yes, that compiler is called \"gcc\".\n> \n> #include <stdio.h>\n> \n> struct foo {\n>         unsigned char a[3];\n>         unsigned char b[3];\n> };\n> \n> int main(void)\n> {\n>         printf(\"%d\\n\", sizeof(struct foo));\n>         return 0;\n> }\n> \n> On i386 that prints 6, on ARM it prints 8.\n\nDoes it add 1 byte after a and and 1 after b or two after b?\nI suspect it's the latter case -- otherwise Gerrit's patch,\nwhich started this thread, wouldn't help solve his problem.\nOr the pad sizing follows complicated rules that I do not\nunderstand at the moment.\n\nTime to look for an ARM emulator, it seems.\n\nThanks,\n"},{"id":"293893","messageId":"4565CA02.20602@shadowen.org","threadId":"43132","inReplyTo":"4565C8F4.6000606@lsrfire.ath.cx","subject":"Re: sizeof(struct ...)","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-11-23T16:19:14Z","receivedAt":"2006-11-23T16:19:14Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"René Scharfe wrote:\n> Erik Mouw schrieb:\n>> On Thu, Nov 23, 2006 at 04:45:09PM +0100, René Scharfe wrote:\n>>>  Is there really a compiler that inserts padding between arrays of\n>>> unsigned chars?\n>> Yes, that compiler is called \"gcc\".\n>>\n>> #include <stdio.h>\n>>\n>> struct foo {\n>>         unsigned char a[3];\n>>         unsigned char b[3];\n>> };\n>>\n>> int main(void)\n>> {\n>>         printf(\"%d\\n\", sizeof(struct foo));\n>>         return 0;\n>> }\n>>\n>> On i386 that prints 6, on ARM it prints 8.\n> \n> Does it add 1 byte after a and and 1 after b or two after b?\n> I suspect it's the latter case -- otherwise Gerrit's patch,\n> which started this thread, wouldn't help solve his problem.\n> Or the pad sizing follows complicated rules that I do not\n> understand at the moment.\n> \n> Time to look for an ARM emulator, it seems.\n\nPerhaps we can look and see what a portable application like gzip or\nbzip2 do in this situation.  They must have the same problem.\n\n"},{"id":"297392","messageId":"20061123164258.GE6581@harddisk-recovery.com","threadId":"43132","inReplyTo":"4565C8F4.6000606@lsrfire.ath.cx","subject":"Re: sizeof(struct ...)","fromName":"Erik Mouw","fromEmail":"erik@harddisk-recovery.com","sentAt":"2006-11-23T16:42:58Z","receivedAt":"2006-11-23T16:42:58Z","isPatch":false,"sender":{"key":"erik@harddisk-recovery.com","avatar":null},"body":"On Thu, Nov 23, 2006 at 05:14:44PM +0100, Ren? Scharfe wrote:\n> Erik Mouw schrieb:\n> > On Thu, Nov 23, 2006 at 04:45:09PM +0100, Ren? Scharfe wrote:\n> >>  Is there really a compiler that inserts padding between arrays of\n> >> unsigned chars?\n> > \n> > Yes, that compiler is called \"gcc\".\n> > \n> > #include <stdio.h>\n> > \n> > struct foo {\n> >         unsigned char a[3];\n> >         unsigned char b[3];\n> > };\n> > \n> > int main(void)\n> > {\n> >         printf(\"%d\\n\", sizeof(struct foo));\n> >         return 0;\n> > }\n> > \n> > On i386 that prints 6, on ARM it prints 8.\n> \n> Does it add 1 byte after a and and 1 after b or two after b?\n> I suspect it's the latter case -- otherwise Gerrit's patch,\n> which started this thread, wouldn't help solve his problem.\n> Or the pad sizing follows complicated rules that I do not\n> understand at the moment.\n\nYou're right, it adds the padding after b:\n\n#define offsetof(TYPE, MEMBER)  __builtin_offsetof (TYPE, MEMBER)\nprintf(\"%d %d\\n\", offsetof(struct foo, a), offsetof(struct foo, b));\n\nprints \"0 3\" on ARM.\n\n> Time to look for an ARM emulator, it seems.\n\nobjdump -D -S is your friend. I didn't have an ARM target ready, but at\nleast I know enough ARM assembly that I can see what it will print :)\n\n\nErik\n\n-- \n+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --\n"},{"id":"295092","messageId":"4565E0EC.4030709@lsrfire.ath.cx","threadId":"43132","inReplyTo":"4565CA02.20602@shadowen.org","subject":"Re: sizeof(struct ...)","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2006-11-23T17:57:00Z","receivedAt":"2006-11-23T17:57:00Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Andy Whitcroft schrieb:\n> Perhaps we can look and see what a portable application like gzip or\n> bzip2 do in this situation.  They must have the same problem.\n\nInfo-ZIP's zip uses structs only for in-memory storage and has a write\nfunction for each of them that writes the members one by one.  I find\nthe structs in archive-zip.c easier to read, but I might be biased. ;-)\n\nAnyway, archive-zip.c assumes that there is no padding between unsigned\nchar arrays and that an unsigned char is exactly one byte wide.  The\nadditional current assumption -- that sizeof(struct ...) sums up the\nsizes of all struct members -- is wrong on ARM, and the patches in this\nthread correct this error.\n\nSo we're not as portable as Info-ZIP, but I think the assumptions above\nhold true for all interesting architectures.  And we have a readable\ndescription of the on-disk ZIP file headers.\n\n"},{"id":"294222","messageId":"7vpsbdkhzc.fsf@assigned-by-dhcp.cox.net","threadId":"43132","inReplyTo":"45659781.5050005@lsrfire.ath.cx","subject":"Re: sizeof(struct ...)","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-23T20:47:51Z","receivedAt":"2006-11-23T20:47:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Gerrit Pape schrieb:\n>...\n>> It's because sizeof(struct zip_local_header) is 32, zip_dir_header 48,\n>> and zip_dir_trailer 24, breaking the zip files.  Compiling with\n>> -fpack-struct seemed to break other things, so I for now I ended up with\n>> this (not so nice) workaround.\n>\n> Hm, yes, this use sizeof() is not strictly correct.  But I'd very much\n> like to keep being lazy and let the compiler to do the summing.  How\n> about this patch instead?  Does it work for you, Gerrit?\n>...\n> @@ -35,6 +35,7 @@ struct zip_local_header {\n> \tunsigned char size[4];\n> \tunsigned char filename_length[2];\n> \tunsigned char extra_length[2];\n> +\tunsigned char _end[0];\n> };\n\nWhile I think relying on the compiler to add no pad in the\nmiddle of the structure that has only unsigned char array\nmembers, making the compiler to sum the member length using\noffsetof() is a reasonable approach to avoid hardcoding the size\nof on-disk structure representation, zero-length member is not\nportable.\n\nWe need to deal with tail padding in any case, and this _end[N]\nis essentially a manual tail padding when N > 0, so I think that\nthe code should work even if you change these to _end[1], and\nthat would be a reasonably clean solution to this problem.\n\n\n"},{"id":"294637","messageId":"45661A7D.9070207@lsrfire.ath.cx","threadId":"43132","inReplyTo":"7vpsbdkhzc.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] archive-zip: don't use sizeof(struct ...)","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2006-11-23T22:02:37Z","receivedAt":"2006-11-23T22:02:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We can't rely on sizeof(struct zip_*) returning the sum of\nall struct members.  At least on ARM padding is added at the\nend, as Gerrit Pape reported.  This fixes the problem but\nstill lets the compiler do the summing by introducing\nexplicit padding at the end of the structs and then taking\nits offset as the combined size of the preceding members.\n\nAs Junio correctly notes, the _end[] marker array's size\nmust be greater than zero for compatibility with compilers\nother than gcc.  The space wasted by the markers can safely\nbe neglected because we only have one instance of each\nstruct, i.e. in sum 3 wasted bytes on i386, and 0 on ARM. :)\n\nWe still rely on the compiler to not add padding between the\nstruct members, but that's reasonable given that all of them\nare unsigned char arrays.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n\n\n archive-zip.c |   24 ++++++++++++++++++------\n 1 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex ae5572a..36e922a 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -35,6 +35,7 @@ struct zip_local_header {\n \tunsigned char size[4];\n \tunsigned char filename_length[2];\n \tunsigned char extra_length[2];\n+\tunsigned char _end[1];\n };\n \n struct zip_dir_header {\n@@ -55,6 +56,7 @@ struct zip_dir_header {\n \tunsigned char attr1[2];\n \tunsigned char attr2[4];\n \tunsigned char offset[4];\n+\tunsigned char _end[1];\n };\n \n struct zip_dir_trailer {\n@@ -66,8 +68,18 @@ struct zip_dir_trailer {\n \tunsigned char size[4];\n \tunsigned char offset[4];\n \tunsigned char comment_length[2];\n+\tunsigned char _end[1];\n };\n \n+/*\n+ * On ARM, padding is added at the end of the struct, so a simple\n+ * sizeof(struct ...) reports two bytes more than the payload size\n+ * we're interested in.\n+ */\n+#define ZIP_LOCAL_HEADER_SIZE\toffsetof(struct zip_local_header, _end)\n+#define ZIP_DIR_HEADER_SIZE\toffsetof(struct zip_dir_header, _end)\n+#define ZIP_DIR_TRAILER_SIZE\toffsetof(struct zip_dir_trailer, _end)\n+\n static void copy_le16(unsigned char *dest, unsigned int n)\n {\n \tdest[0] = 0xff & n;\n@@ -211,7 +223,7 @@ static int write_zip_entry(const unsigne\n \t}\n \n \t/* make sure we have enough free space in the dictionary */\n-\tdirentsize = sizeof(struct zip_dir_header) + pathlen;\n+\tdirentsize = ZIP_DIR_HEADER_SIZE + pathlen;\n \twhile (zip_dir_size < zip_dir_offset + direntsize) {\n \t\tzip_dir_size += ZIP_DIRECTORY_MIN_SIZE;\n \t\tzip_dir = xrealloc(zip_dir, zip_dir_size);\n@@ -234,8 +246,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le16(dirent.attr1, 0);\n \tcopy_le32(dirent.attr2, attr2);\n \tcopy_le32(dirent.offset, zip_offset);\n-\tmemcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));\n-\tzip_dir_offset += sizeof(struct zip_dir_header);\n+\tmemcpy(zip_dir + zip_dir_offset, &dirent, ZIP_DIR_HEADER_SIZE);\n+\tzip_dir_offset += ZIP_DIR_HEADER_SIZE;\n \tmemcpy(zip_dir + zip_dir_offset, path, pathlen);\n \tzip_dir_offset += pathlen;\n \tzip_dir_entries++;\n@@ -251,8 +263,8 @@ static int write_zip_entry(const unsigne\n \tcopy_le32(header.size, uncompressed_size);\n \tcopy_le16(header.filename_length, pathlen);\n \tcopy_le16(header.extra_length, 0);\n-\twrite_or_die(1, &header, sizeof(struct zip_local_header));\n-\tzip_offset += sizeof(struct zip_local_header);\n+\twrite_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);\n+\tzip_offset += ZIP_LOCAL_HEADER_SIZE;\n \twrite_or_die(1, path, pathlen);\n \tzip_offset += pathlen;\n \tif (compressed_size > 0) {\n@@ -282,7 +294,7 @@ static void write_zip_trailer(const unsi\n \tcopy_le16(trailer.comment_length, sha1 ? 40 : 0);\n \n \twrite_or_die(1, zip_dir, zip_dir_offset);\n-\twrite_or_die(1, &trailer, sizeof(struct zip_dir_trailer));\n+\twrite_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);\n \tif (sha1)\n \t\twrite_or_die(1, sha1_to_hex(sha1), 40);\n }\n"},{"id":"295621","messageId":"20061124085357.21541.qmail@fd76b23131eb24.315fe32.mid.smarden.org","threadId":"43132","inReplyTo":"45661A7D.9070207@lsrfire.ath.cx","subject":"Re: [PATCH] archive-zip: don't use sizeof(struct ...)","fromName":"Gerrit Pape","fromEmail":"pape@smarden.org","sentAt":"2006-11-24T08:53:57Z","receivedAt":"2006-11-24T08:53:57Z","isPatch":true,"sender":{"key":"pape@smarden.org","avatar":"https://avatars.githubusercontent.com/u/143170252?v=4"},"body":"On Thu, Nov 23, 2006 at 11:02:37PM +0100, Ren? Scharfe wrote:\n> We can't rely on sizeof(struct zip_*) returning the sum of\n> all struct members.  At least on ARM padding is added at the\n> end, as Gerrit Pape reported.  This fixes the problem but\n> still lets the compiler do the summing by introducing\n> explicit padding at the end of the structs and then taking\n> its offset as the combined size of the preceding members.\n> \n> As Junio correctly notes, the _end[] marker array's size\n> must be greater than zero for compatibility with compilers\n> other than gcc.  The space wasted by the markers can safely\n> be neglected because we only have one instance of each\n> struct, i.e. in sum 3 wasted bytes on i386, and 0 on ARM. :)\n> \n> We still rely on the compiler to not add padding between the\n> struct members, but that's reasonable given that all of them\n> are unsigned char arrays.\n> \n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n\n"}]}