threads / patch / 6420

patch, 2 partsUse fixed-size integers for .idx file I/O

Subject: [PATCH 2/2] Use fixed-size integers for .idx file I/O

## tl;dr

6 messages between Jan 18, 2007 and Jan 18, 2007. Diffs are folded; open one to read it.

replies: 5people: 5as markdown or json

Junio C Hamano· Jan 18, 2007, 07:24 UTC · lore

This attempts to finish what Simon started in the previous commit.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * I do not think this should have any platform dependent impact
   as long as 1/2 is Ok, but for completeness's sake here it is.

 builtin-pack-objects.c |    6 +++---
 cache.h                |    2 +-
 sha1_file.c            |    6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

Show changes to 3 files +7 −7

builtin-pack-objects.c, cache.h, sha1_file.c

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 42dd8c8..3824ee3 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -569,7 +569,7 @@ static void write_index_file(void)
 					sha1_to_hex(object_list_sha1), "idx");
 	struct object_entry **list = sorted_by_sha;
 	struct object_entry **last = list + nr_result;
-	unsigned int array[256];
+	uint32_t array[256];
 
 	/*
 	 * Write the first-level table (the list is sorted,
@@ -587,7 +587,7 @@ static void write_index_file(void)
 		array[i] = htonl(next - sorted_by_sha);
 		list = next;
 	}
-	sha1write(f, array, 256 * sizeof(int));
+	sha1write(f, array, 256 * 4);
 
 	/*
 	 * Write the actual SHA1 entries..
@@ -595,7 +595,7 @@ static void write_index_file(void)
 	list = sorted_by_sha;
 	for (i = 0; i < nr_result; i++) {
 		struct object_entry *entry = *list++;
-		unsigned int offset = htonl(entry->offset);
+		uint32_t offset = htonl(entry->offset);
 		sha1write(f, &offset, 4);
 		sha1write(f, entry->sha1, 20);
 	}
diff --git a/cache.h b/cache.h
index fda3f8e..e6e19bd 100644
--- a/cache.h
+++ b/cache.h
@@ -351,7 +351,7 @@ struct pack_window {
 extern struct packed_git {
 	struct packed_git *next;
 	struct pack_window *windows;
-	unsigned int *index_base;
+	uint32_t *index_base;
 	off_t index_size;
 	off_t pack_size;
 	int pack_fd;
diff --git a/sha1_file.c b/sha1_file.c
index 0b70545..3025440 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -435,7 +435,7 @@ static int check_packed_git_idx(const char *path, unsigned long *idx_size_,
 				void **idx_map_)
 {
 	void *idx_map;
-	unsigned int *index;
+	uint32_t *index;
 	unsigned long idx_size;
 	int nr, i;
 	int fd = open(path, O_RDONLY);
@@ -1351,7 +1351,7 @@ int nth_packed_object_sha1(const struct packed_git *p, int n,
 unsigned long find_pack_entry_one(const unsigned char *sha1,
 				  struct packed_git *p)
 {
-	unsigned int *level1_ofs = p->index_base;
+	uint32_t *level1_ofs = p->index_base;
 	int hi = ntohl(level1_ofs[*sha1]);
 	int lo = ((*sha1 == 0x0) ? 0 : ntohl(level1_ofs[*sha1 - 1]));
 	void *index = p->index_base + 256;
@@ -1360,7 +1360,7 @@ unsigned long find_pack_entry_one(const unsigned char *sha1,
 		int mi = (lo + hi) / 2;
 		int cmp = hashcmp((unsigned char *)index + (24 * mi) + 4, sha1);
 		if (!cmp)
-			return ntohl(*((unsigned int *) ((char *) index + (24 * mi))));
+			return ntohl(*((uint32_t *)((char *)index + (24 * mi))));
 		if (cmp > 0)
 			hi = mi;
 		else

-- 
1.5.0.rc1.g05cf8

Morten Welinder· Jan 18, 2007, 14:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Use fixed-size integers for .idx file I/O

> -                       return ntohl(*((unsigned int *) ((char *) index + (24 * mi))));
> +                       return ntohl(*((uint32_t *)((char *)index + (24 * mi))));

Is that pointer gymnastics guaranteed to work? I.e., how do we know that we can access an uint32_t (or unsigned) at such an address?

M.

Simon 'corecode' Schubert· Jan 18, 2007, 14:56 UTC · re: Morten Welinder · lore

Re: [PATCH 2/2] Use fixed-size integers for .idx file I/O

Morten Welinder wrote:

Show 7 quoted lines

>> -                       return ntohl(*((unsigned int *) ((char *) 
>> index + (24 * mi))));
>> +                       return ntohl(*((uint32_t *)((char *)index + 
>> (24 * mi))));
> 
> Is that pointer gymnastics guaranteed to work?  I.e., how do we know
> that we can access an uint32_t (or unsigned) at such an address?

if index is always aligned to a 4-byte boundary, this is safe.  apart from that, the problem already existed.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \

Johannes Schindelin· Jan 18, 2007, 15:15 UTC · re: Simon 'corecode' Schubert · lore

Re: [PATCH 2/2] Use fixed-size integers for .idx file I/O

Hi,

On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:

Show 11 quoted lines

> Morten Welinder wrote:
> > > -                       return ntohl(*((unsigned int *) ((char *) index +
> > > (24 * mi))));
> > > +                       return ntohl(*((uint32_t *)((char *)index + (24 *
> > > mi))));
> > 
> > Is that pointer gymnastics guaranteed to work?  I.e., how do we know
> > that we can access an uint32_t (or unsigned) at such an address?
> 
> if index is always aligned to a 4-byte boundary, this is safe.  apart from
> that, the problem already existed.

index is assigned from p->index_base, which comes from check_packed_git_idx(), and there it comes from an xmmap(). AFAICT mmap() (and for NO_MMAP, malloc()) _always_ return aligned pointers, so there is no problem, as long as the alignment step divides 24.

Ciao, Dscho

Shawn O. Pearce· Jan 18, 2007, 15:21 UTC · re: Simon 'corecode' Schubert · lore

Re: [PATCH 2/2] Use fixed-size integers for .idx file I/O

Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:

Show 11 quoted lines

> Morten Welinder wrote:
> >>-                       return ntohl(*((unsigned int *) ((char *) 
> >>index + (24 * mi))));
> >>+                       return ntohl(*((uint32_t *)((char *)index + 
> >>(24 * mi))));
> >
> >Is that pointer gymnastics guaranteed to work?  I.e., how do we know
> >that we can access an uint32_t (or unsigned) at such an address?
> 
> if index is always aligned to a 4-byte boundary, this is safe.  apart from 
> that, the problem already existed.

Its always 4-byte aligned here. The index is mmap()'d as one huge chunk so the first byte of the index is page aligned. The index starts out with 256 4-byte words, then is composed of 24 byte units (20 byte SHA1, 4 byte offset). So no worries with the current file format, or code.

Yes, we're taking the leap of faith that any currently-used processor will work on 32 bit unsigned integers if they are 4 byte aligned in memory. Maybe someone has a processor that this isn't true for, but a lot of other software would probably break on that same system...

-- 
Shawn.

Johannes Schindelin· Jan 18, 2007, 15:30 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 2/2] Use fixed-size integers for .idx file I/O

Hi,

On Thu, 18 Jan 2007, Shawn O. Pearce wrote:

Show 22 quoted lines

> Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> > Morten Welinder wrote:
> > >>-                       return ntohl(*((unsigned int *) ((char *) 
> > >>index + (24 * mi))));
> > >>+                       return ntohl(*((uint32_t *)((char *)index + 
> > >>(24 * mi))));
> > >
> > >Is that pointer gymnastics guaranteed to work?  I.e., how do we know
> > >that we can access an uint32_t (or unsigned) at such an address?
> > 
> > if index is always aligned to a 4-byte boundary, this is safe.  apart from 
> > that, the problem already existed.
> 
> Its always 4-byte aligned here.  The index is mmap()'d as one huge
> chunk so the first byte of the index is page aligned.  The index
> starts out with 256 4-byte words, then is composed of 24 byte units
> (20 byte SHA1, 4 byte offset).  So no worries with the current file
> format, or code.
> 
> Yes, we're taking the leap of faith that any currently-used processor
> will work on 32 bit unsigned integers if they are 4 byte aligned in
> memory.

Actually, it's even valid in 8 byte aligned mode. (24 is divisible by 8.)

Ciao, Dscho