Jeff King <peff@xxxxxxxx> wrote:
> The union thing should be portable, I'd think, but unfortunately has a
> lot of fallout through the code because the name of "buffer" changes (we
> could also declare the storage separately and make "buffer" a pointer to
> it, but we'd have to be careful about calls to sizeof()).
>
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 2197d6d933..65db435b46 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -23,7 +23,10 @@ static int dry_run, quiet, recover, has_errors, strict;
> static const char unpack_usage[] = "git unpack-objects [-n] [-q] [-r] [--strict]";
>
> /* We always read in 4kB chunks. */
> -static unsigned char buffer[4096];
> +static union {
> + struct pack_header hdr;
> + unsigned char bytes[4096];
> +} buffer;
> static unsigned int offset, len;
> static off_t consumed_bytes;
> static off_t max_input_size;
> @@ -65,24 +68,24 @@ static void add_object_buffer(struct object *object, char *buffer, unsigned long
> static void *fill(int min)
> {
> if (min <= len)
> - return buffer + offset;
> - if (min > sizeof(buffer))
>
> + return buffer.bytes + offset;
> + if (min > sizeof(buffer.bytes))
>
> die("cannot fill %d bytes", min);
> if (offset) {
> - the_hash_algo->update_fn(&ctx, buffer, offset);
>
> - memmove(buffer, buffer + offset, len);
> + the_hash_algo->update_fn(&ctx, buffer.bytes, offset);
>
> + memmove(buffer.bytes, buffer.bytes + offset, len);
> offset = 0;
> }
> do {
> - ssize_t ret = xread(0, buffer + len, sizeof(buffer) - len);
> + ssize_t ret = xread(0, buffer.bytes + len, sizeof(buffer.bytes) - len);
> if (ret <= 0) {
> if (!ret)
> die("early EOF");
> die_errno("read error on input");
> }
> len += ret;
> } while (len < min);
> - return buffer;
> + return buffer.bytes;
> }
>
> static void use(int bytes)
> @@ -645,18 +648,16 @@ int cmd_unpack_objects(int argc,
> continue;
> }
> if (starts_with(arg, "--pack_header=")) {
> - struct pack_header *hdr;
> char *c;
>
> - hdr = (struct pack_header *)buffer;
> - hdr->hdr_signature = htonl(PACK_SIGNATURE);
>
> - hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10));
>
> + buffer.hdr.hdr_signature = htonl(PACK_SIGNATURE);
> + buffer.hdr.hdr_version = htonl(strtoul(arg + 14, &c, 10));
> if (*c != ',')
> die("bad %s", arg);
> - hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10));
>
> + buffer.hdr.hdr_entries = htonl(strtoul(c + 1, &c, 10));
> if (*c)
> die("bad %s", arg);
> - len = sizeof(*hdr);
> + len = sizeof(buffer.hdr);
> continue;
> }
> if (skip_prefix(arg, "--max-input-size=", &arg)) {
> @@ -671,7 +672,7 @@ int cmd_unpack_objects(int argc,
> }
> the_hash_algo->init_fn(&ctx);
>
> unpack_all();
> - the_hash_algo->update_fn(&ctx, buffer, offset);
>
> + the_hash_algo->update_fn(&ctx, buffer.bytes, offset);
>
> the_hash_algo->init_fn(&tmp_ctx);
>
> the_hash_algo->clone_fn(&tmp_ctx, &ctx);
>
> the_hash_algo->final_oid_fn(&oid, &tmp_ctx);
>
> @@ -686,7 +687,7 @@ int cmd_unpack_objects(int argc,
> use(the_hash_algo->rawsz);
>
>
> /* Write the last part of the buffer to stdout /
> - write_in_full(1, buffer + offset, len);
> + write_in_full(1, buffer.bytes + offset, len);
>
> / All done */
> return has_errors;
This patch works well here, yes.
> I guess that's not too surprising. Probably an application of get_be32()
> would solve it. But I do wonder if it would be simpler just to make sure
> the buffer is aligned. You mentioned that you tried that before and it
> worked. How did you do it? With a pragma/attribute, or with a union (as
> below)?
I did it by adding an attribute, though probably a better implementation would
query for the alignment of `structack_hdr` instead of hardcoding it.
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 842a90353a..98f270ec0c 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -23,7 +23,7 @@ static int dry_run, quiet, recover, has_errors, strict;
static const char unpack_usage[] = "git unpack-objects [-n] [-q] [-r] [--strict]";
/* We always read in 4kB chunks. */
-static unsigned char buffer[4096];
+static unsigned char buffer[4096] __attribute__((aligned(16)));
static unsigned int offset, len;
static off_t consumed_bytes;
static off_t max_input_size;
