[PATCH 0/2] commit-slab cleanups

[PATCH 0/2] commit-slab cleanups

[Thomas Rast]

I gathered these while writing an "all merge bases simultaneously"
algorithm.  Turns out this fancy algorithm loses vs. simply calling
get_merge_bases() a lot, so I dropped that.  But the cleanups seem
valid anyway.


Thomas Rast (2):
  commit-slab: document clear_$slabname()
  commit-slab: declare functions "static inline"

 commit-slab.h | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

-- 
1.8.5.rc3.397.g2a3acd5

[PATCH 1/2] commit-slab: document clear_$slabname()

[Thomas Rast]

The clear_$slabname() function was only documented by source code so
far.  Write something about it.

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
 commit-slab.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/commit-slab.h b/commit-slab.h
index d4c8286..d77aaea 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -24,6 +24,10 @@
  *   to each commit. 'stride' specifies how big each array is.  The slab
  *   that id initialied by the variant without "_with_stride" associates
  *   each commit with an array of one integer.
+ *
+ * - void clear_indegree(struct indegree *);
+ *
+ *   Free the slab's data structures.
  */
 
 /* allocate ~512kB at once, allowing for malloc overhead */
-- 
1.8.5.rc3.397.g2a3acd5

[PATCH 2/2] commit-slab: declare functions "static inline"

[Thomas Rast]

This shuts up compiler warnings about unused functions.

While there, also remove the redundant second declaration of
stat_##slabname##realloc.

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
 commit-slab.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/commit-slab.h b/commit-slab.h
index d77aaea..d5c353e 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -45,8 +45,8 @@ struct slabname {							\
 };									\
 static int stat_ ##slabname## realloc;					\
 									\
-static void init_ ##slabname## _with_stride(struct slabname *s,		\
-					    unsigned stride)		\
+static inline void init_ ##slabname## _with_stride(struct slabname *s,	\
+						   unsigned stride)	\
 {									\
 	unsigned int elem_size;						\
 	if (!stride)							\
@@ -58,12 +58,12 @@ struct slabname {							\
 	s->slab = NULL;							\
 }									\
 									\
-static void init_ ##slabname(struct slabname *s)			\
+static inline void init_ ##slabname(struct slabname *s)			\
 {									\
 	init_ ##slabname## _with_stride(s, 1);				\
 }									\
 									\
-static void clear_ ##slabname(struct slabname *s)			\
+static inline void clear_ ##slabname(struct slabname *s)		\
 {									\
 	int i;								\
 	for (i = 0; i < s->slab_count; i++)				\
@@ -73,8 +73,8 @@ struct slabname {							\
 	s->slab = NULL;							\
 }									\
 									\
-static elemtype *slabname## _at(struct slabname *s,			\
-				const struct commit *c)			\
+static inline elemtype *slabname## _at(struct slabname *s,		\
+				       const struct commit *c)		\
 {									\
 	int nth_slab, nth_slot;						\
 									\
@@ -94,8 +94,7 @@ struct slabname {							\
 		s->slab[nth_slab] = xcalloc(s->slab_size,		\
 					    sizeof(**s->slab) * s->stride);		\
 	return &s->slab[nth_slab][nth_slot * s->stride];				\
-}									\
-									\
-static int stat_ ##slabname## realloc
+}
+
 
 #endif /* COMMIT_SLAB_H */
-- 
1.8.5.rc3.397.g2a3acd5

Re: [PATCH 2/2] commit-slab: declare functions "static inline"

[Junio C Hamano]

Thomas Rast <tr@thomasrast.ch> writes:

> This shuts up compiler warnings about unused functions.

Thanks.

> While there, also remove the redundant second declaration of
> stat_##slabname##realloc.

I think the latter was done very much deliberately to allow the
using code to say:

	define_commit_slab(name, type);

by ending the macro with something that requires a terminating
semicolon.  If you just remove it, doesn't it break the compilation
by forcing the expanded source to define a function

	slabname ## _at(...)
        {
        	...
	};

with a trailing and undesired semicolon?

>
> Signed-off-by: Thomas Rast <tr@thomasrast.ch>
> ---
>  commit-slab.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index d77aaea..d5c353e 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -45,8 +45,8 @@ struct slabname {							\
>  };									\
>  static int stat_ ##slabname## realloc;					\
>  									\
> -static void init_ ##slabname## _with_stride(struct slabname *s,		\
> -					    unsigned stride)		\
> +static inline void init_ ##slabname## _with_stride(struct slabname *s,	\
> +						   unsigned stride)	\
>  {									\
>  	unsigned int elem_size;						\
>  	if (!stride)							\
> @@ -58,12 +58,12 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static void init_ ##slabname(struct slabname *s)			\
> +static inline void init_ ##slabname(struct slabname *s)			\
>  {									\
>  	init_ ##slabname## _with_stride(s, 1);				\
>  }									\
>  									\
> -static void clear_ ##slabname(struct slabname *s)			\
> +static inline void clear_ ##slabname(struct slabname *s)		\
>  {									\
>  	int i;								\
>  	for (i = 0; i < s->slab_count; i++)				\
> @@ -73,8 +73,8 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static elemtype *slabname## _at(struct slabname *s,			\
> -				const struct commit *c)			\
> +static inline elemtype *slabname## _at(struct slabname *s,		\
> +				       const struct commit *c)		\
>  {									\
>  	int nth_slab, nth_slot;						\
>  									\
> @@ -94,8 +94,7 @@ struct slabname {							\
>  		s->slab[nth_slab] = xcalloc(s->slab_size,		\
>  					    sizeof(**s->slab) * s->stride);		\
>  	return &s->slab[nth_slab][nth_slot * s->stride];				\
> -}									\
> -									\
> -static int stat_ ##slabname## realloc
> +}
> +
>  
>  #endif /* COMMIT_SLAB_H */

Re: [PATCH 2/2] commit-slab: declare functions "static inline"

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <tr@thomasrast.ch> writes:
>
>> This shuts up compiler warnings about unused functions.
>
> Thanks.
>
>> While there, also remove the redundant second declaration of
>> stat_##slabname##realloc.
>
> I think the latter was done very much deliberately to allow the
> using code to say:
>
> 	define_commit_slab(name, type);
>
> by ending the macro with something that requires a terminating
> semicolon.  If you just remove it, doesn't it break the compilation
> by forcing the expanded source to define a function
>
> 	slabname ## _at(...)
>         {
>         	...
> 	};
>
> with a trailing and undesired semicolon?

Oooh.  The sudden enlightenment.

-- 
Thomas Rast
tr@thomasrast.ch

[PATCH v2] commit-slab: declare functions "static inline"

[Thomas Rast]

This shuts up compiler warnings about unused functions.  No such
warnings are currently triggered, but if someone were to actually use
init_NAME_with_stride() as documented, they would get a warning about
init_NAME() being unused.

While there, write a comment about why we need two declarations of the
same variable.

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---

Here's a version that has a fat comment instead of the removal.

Also, since I was rerolling anyway I put a reason why we need this.
In the original motivation I actually created more functions
afterwards, which made it more convincing, but the problem already
exists.


 commit-slab.h | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/commit-slab.h b/commit-slab.h
index d77aaea..21d54f1 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -45,8 +45,8 @@ struct slabname {							\
 };									\
 static int stat_ ##slabname## realloc;					\
 									\
-static void init_ ##slabname## _with_stride(struct slabname *s,		\
-					    unsigned stride)		\
+static inline void init_ ##slabname## _with_stride(struct slabname *s,	\
+						   unsigned stride)	\
 {									\
 	unsigned int elem_size;						\
 	if (!stride)							\
@@ -58,12 +58,12 @@ struct slabname {							\
 	s->slab = NULL;							\
 }									\
 									\
-static void init_ ##slabname(struct slabname *s)			\
+static inline void init_ ##slabname(struct slabname *s)			\
 {									\
 	init_ ##slabname## _with_stride(s, 1);				\
 }									\
 									\
-static void clear_ ##slabname(struct slabname *s)			\
+static inline void clear_ ##slabname(struct slabname *s)		\
 {									\
 	int i;								\
 	for (i = 0; i < s->slab_count; i++)				\
@@ -73,8 +73,8 @@ struct slabname {							\
 	s->slab = NULL;							\
 }									\
 									\
-static elemtype *slabname## _at(struct slabname *s,			\
-				const struct commit *c)			\
+static inline elemtype *slabname## _at(struct slabname *s,		\
+				       const struct commit *c)		\
 {									\
 	int nth_slab, nth_slot;						\
 									\
@@ -98,4 +98,16 @@ struct slabname {							\
 									\
 static int stat_ ##slabname## realloc
 
+/*
+ * Note that this seemingly redundant second declaration is required
+ * to allow a terminating semicolon, which makes instantiations look
+ * like function declarations.  I.e., the expansion of
+ *
+ *    define_commit_slab(indegree, int);
+ *
+ * ends in 'static int stat_indegreerealloc;'.  This would otherwise
+ * be a syntax error according (at least) to ISO C.  It's hard to
+ * catch because GCC silently parses it by default.
+ */
+
 #endif /* COMMIT_SLAB_H */
-- 
1.8.5.rc2.355.g6969a19

Re: [PATCH v2] commit-slab: declare functions "static inline"

[Jonathan Nieder]

Thomas Rast wrote:

> This shuts up compiler warnings about unused functions.

If that is the only goal, I think it would be cleaner to use

	#define MAYBE_UNUSED __attribute__((__unused__))

	static MAYBE_UNUSED void init_ ...

like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
2010-12-13) et al.

I haven't thought carefully about whether encouraging inlining here
(or encouraging the reader to think of these functions as inline) is a
good or bad change.

[...]
> @@ -98,4 +98,16 @@ struct slabname {							\
>  									\
>  static int stat_ ##slabname## realloc
>  
> +/*
> + * Note that this seemingly redundant second declaration is required
> + * to allow a terminating semicolon, which makes instantiations look
> + * like function declarations.  I.e., the expansion of

Micronit: this reads more clearly without the "Note that".  That is,
the comment can get the reader's attention more easily by going right
into what it is about to say without asking for the reader's
attention:

	/*
	 * This seemingly redundant second declaration is required to ...

Thanks,
Jonathan

Re: [PATCH v2] commit-slab: declare functions "static inline"

[Thomas Rast]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thomas Rast wrote:
>
>> This shuts up compiler warnings about unused functions.
>
> If that is the only goal, I think it would be cleaner to use
>
> 	#define MAYBE_UNUSED __attribute__((__unused__))
>
> 	static MAYBE_UNUSED void init_ ...
>
> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
> 2010-12-13) et al.
>
> I haven't thought carefully about whether encouraging inlining here
> (or encouraging the reader to think of these functions as inline) is a
> good or bad change.

Hmm.

I actually had this idea after seeing the same trick in khash.h.  Is
__atribute__((__unused__)) universal?  If so, maybe we could apply the
same also to khash?  If not, I'd rather go with the inline.

-- 
Thomas Rast
tr@thomasrast.ch

Re: [PATCH v2] commit-slab: declare functions "static inline"

[Junio C Hamano]

Thomas Rast <tr@thomasrast.ch> writes:

> Here's a version that has a fat comment instead of the removal.
>
> Also, since I was rerolling anyway I put a reason why we need this.
> In the original motivation I actually created more functions
> afterwards, which made it more convincing, but the problem already
> exists.

Thanks.

I considered the bottom one the real declaration (with the top one a
forward declaration we need to make the result compile), by the way,
so there may be no redundancy anywhere ;-)



>  commit-slab.h | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index d77aaea..21d54f1 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -45,8 +45,8 @@ struct slabname {							\
>  };									\
>  static int stat_ ##slabname## realloc;					\
>  									\
> -static void init_ ##slabname## _with_stride(struct slabname *s,		\
> -					    unsigned stride)		\
> +static inline void init_ ##slabname## _with_stride(struct slabname *s,	\
> +						   unsigned stride)	\
>  {									\
>  	unsigned int elem_size;						\
>  	if (!stride)							\
> @@ -58,12 +58,12 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static void init_ ##slabname(struct slabname *s)			\
> +static inline void init_ ##slabname(struct slabname *s)			\
>  {									\
>  	init_ ##slabname## _with_stride(s, 1);				\
>  }									\
>  									\
> -static void clear_ ##slabname(struct slabname *s)			\
> +static inline void clear_ ##slabname(struct slabname *s)		\
>  {									\
>  	int i;								\
>  	for (i = 0; i < s->slab_count; i++)				\
> @@ -73,8 +73,8 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static elemtype *slabname## _at(struct slabname *s,			\
> -				const struct commit *c)			\
> +static inline elemtype *slabname## _at(struct slabname *s,		\
> +				       const struct commit *c)		\
>  {									\
>  	int nth_slab, nth_slot;						\
>  									\
> @@ -98,4 +98,16 @@ struct slabname {							\
>  									\
>  static int stat_ ##slabname## realloc
>  
> +/*
> + * Note that this seemingly redundant second declaration is required
> + * to allow a terminating semicolon, which makes instantiations look
> + * like function declarations.  I.e., the expansion of
> + *
> + *    define_commit_slab(indegree, int);
> + *
> + * ends in 'static int stat_indegreerealloc;'.  This would otherwise
> + * be a syntax error according (at least) to ISO C.  It's hard to
> + * catch because GCC silently parses it by default.
> + */
> +
>  #endif /* COMMIT_SLAB_H */

Re: [PATCH 1/2] commit-slab: document clear_$slabname()

[Jonathan Nieder]

Thomas Rast wrote:

> The clear_$slabname() function was only documented by source code so
> far.  Write something about it.

Good idea.

[...]
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -24,6 +24,10 @@
>   *   to each commit. 'stride' specifies how big each array is.  The slab
>   *   that id initialied by the variant without "_with_stride" associates
>   *   each commit with an array of one integer.
> + *
> + * - void clear_indegree(struct indegree *);
> + *
> + *   Free the slab's data structures.

Tense shift (previous descriptions were in the present tense, while
this one is in the imperative).

More importantly, this doesn't answer the questions I'd have if I were
in a hurry, which are what exactly is being freed (has the slab taken
ownership of any memory from the user, e.g. when elemtype is a
pointer?) and whether the slab needs to be init_ ed again.

Maybe something like the following would work?

	- void clear_indegree(struct indegree *);

	  Empties the slab.  The slab can be reused with the same
	  stride without calling init_indegree again or can be
	  reconfigured to a different stride by calling
	  init_indegree_with_stride.

	  Call this function before the slab falls out of scope to
	  avoid leaking memory.

Thanks,
Jonathan

Re: [PATCH v2] commit-slab: declare functions "static inline"

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thomas Rast wrote:
>
>> This shuts up compiler warnings about unused functions.
>
> If that is the only goal, I think it would be cleaner to use
>
> 	#define MAYBE_UNUSED __attribute__((__unused__))
>
> 	static MAYBE_UNUSED void init_ ...
>
> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
> 2010-12-13) et al.
>
> I haven't thought carefully about whether encouraging inlining here
> (or encouraging the reader to think of these functions as inline) is a
> good or bad change.
>
> [...]
>> @@ -98,4 +98,16 @@ struct slabname {							\
>>  									\
>>  static int stat_ ##slabname## realloc
>>  
>> +/*
>> + * Note that this seemingly redundant second declaration is required
>> + * to allow a terminating semicolon, which makes instantiations look
>> + * like function declarations.  I.e., the expansion of
>
> Micronit: this reads more clearly without the "Note that".  That is,
> the comment can get the reader's attention more easily by going right
> into what it is about to say without asking for the reader's
> attention:
>
> 	/*
> 	 * This seemingly redundant second declaration is required to ...

Hmm, both of these are good points.

Thanks.

Re: [PATCH v2] commit-slab: declare functions "static inline"

[Jonathan Nieder]

Thomas Rast wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Thomas Rast wrote:

>>> This shuts up compiler warnings about unused functions.
>>
>> If that is the only goal, I think it would be cleaner to use
>>
>> 	#define MAYBE_UNUSED __attribute__((__unused__))
>>
>> 	static MAYBE_UNUSED void init_ ...
>>
>> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
>> 2010-12-13) et al.
>>
>> I haven't thought carefully about whether encouraging inlining here
>> (or encouraging the reader to think of these functions as inline) is a
>> good or bad change.
>
> Hmm.
>
> I actually had this idea after seeing the same trick in khash.h.  Is
> __atribute__((__unused__)) universal?  If so, maybe we could apply the
> same also to khash?  If not, I'd rather go with the inline.

The khash functions are very small, so it very well may make sense for
them to be inline.

git-compat-util.h (or compat/msvc.h) defines __attribute__(x) to an
empty sequence of tokens except on HP C and gcc.  Attribute unused has
existed at least since GCC 2.95.

Unfortunately HP C doesn't support attribute __unused__. :(
http://h21007.www2.hp.com/portal/download/files/unprot/aCxx/Online_Help/pragmas.htm#Attributes

On the bright side, it would be easy to work around using a
conditional definition of MAYBE_UNUSED for the sake of HP C.  From
August 2010 until March 2011 nobody noticed.

Jonathan

Re: [PATCH 1/2] commit-slab: document clear_$slabname()

[Junio C Hamano]

Thomas Rast <tr@thomasrast.ch> writes:

> The clear_$slabname() function was only documented by source code so
> far.  Write something about it.
>
> Signed-off-by: Thomas Rast <tr@thomasrast.ch>
> ---
>  commit-slab.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index d4c8286..d77aaea 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -24,6 +24,10 @@
>   *   to each commit. 'stride' specifies how big each array is.  The slab
>   *   that id initialied by the variant without "_with_stride" associates

Is that "id" a typo for "is"?

>   *   each commit with an array of one integer.
> + *
> + * - void clear_indegree(struct indegree *);
> + *
> + *   Free the slab's data structures.
>   */
>  
>  /* allocate ~512kB at once, allowing for malloc overhead */

Re: [PATCH 1/2] commit-slab: document clear_$slabname()

[Eric Sunshine]

On Mon, Nov 25, 2013 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thomas Rast <tr@thomasrast.ch> writes:
>
>> The clear_$slabname() function was only documented by source code so
>> far.  Write something about it.
>>
>> Signed-off-by: Thomas Rast <tr@thomasrast.ch>
>> ---
>>  commit-slab.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/commit-slab.h b/commit-slab.h
>> index d4c8286..d77aaea 100644
>> --- a/commit-slab.h
>> +++ b/commit-slab.h
>> @@ -24,6 +24,10 @@
>>   *   to each commit. 'stride' specifies how big each array is.  The slab
>>   *   that id initialied by the variant without "_with_stride" associates
>
> Is that "id" a typo for "is"?

And, s/initialied/initialized/

>>   *   each commit with an array of one integer.
>> + *
>> + * - void clear_indegree(struct indegree *);
>> + *
>> + *   Free the slab's data structures.
>>   */
>>
>>  /* allocate ~512kB at once, allowing for malloc overhead */
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] commit-slab: document clear_$slabname()

[Thomas Rast]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thomas Rast wrote:
>
>> + *
>> + * - void clear_indegree(struct indegree *);
>> + *
>> + *   Free the slab's data structures.
>
> Tense shift (previous descriptions were in the present tense, while
> this one is in the imperative).
>
> More importantly, this doesn't answer the questions I'd have if I were
> in a hurry, which are what exactly is being freed (has the slab taken
> ownership of any memory from the user, e.g. when elemtype is a
> pointer?) and whether the slab needs to be init_ ed again.
>
> Maybe something like the following would work?
[...]

Ok, I see that while I was procrastinating, you sorted this out and
Junio merged it to next.

Thanks, both.

-- 
Thomas Rast
tr@thomasrast.ch

Re: [PATCH 2/2] commit-slab: declare functions "static inline"

[Duy Nguyen]

Thomas,

As you're touching this, perhaps you coud fix this line in slabname_at() too?

s->slab = xrealloc(s->slab, (nth_slab + 1) * sizeof(s->slab));

I think it should be sizeof(*s->slab), not sizeof(s->slab), even
though the end result is the same.
-- 
Duy

[PATCH] commit-slab: sizeof() the right type in xrealloc

[Thomas Rast]

When allocating the slab, the code accidentally computed the array
size from s->slab (an elemtype**).  The slab is an array of elemtype*,
however, so we should take the size of *s->slab.

Noticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
[I hope this comes through clean.  git-send-email is currently broken
for me, and I'm still investigating, so I have to kludge around it.]

I browsed around for a while, and couldn't find out whether any
architecture actually has any hope of running git (i.e. is at least
mostly POSIX conformant) but still violates the assumption that all
pointers[*] are the same size.

The comp.lang.c FAQ has some interesting examples of wildly different
pointer representations at:

  http://c-faq.com/null/machexamp.html

Consider the cases mentioned there where void* and char* have
different representations from, e.g., int*.  Then if elemtype is char,
the slab will be too small before this patch.

But I have no idea if any of those are POSIXish.

One interesting, though orthogonal, tidbit is that POSIX actually
requires _function_ pointers to have the same representation as void*.
From the specification of dlsym(), which depends on this to be able to
return function pointers:

RATIONALE
  [...] Indeed, the ISO C standard does not require that an object of
  type void * can hold a pointer to a function. Implementations
  supporting the XSI extension, however, do require that an object of
  type void * can hold a pointer to a function.

Thank god for POSIX.  So much craziness averted.


[*] Note that C++ method pointers are yet another story.  This only
applies to the kinds of pointers that C supports.


 commit-slab.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit-slab.h b/commit-slab.h
index d068e2d..cc114b5 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -91,7 +91,7 @@ struct slabname {							\
 	if (s->slab_count <= nth_slab) {				\
 		int i;							\
 		s->slab = xrealloc(s->slab,				\
-				   (nth_slab + 1) * sizeof(s->slab));	\
+				   (nth_slab + 1) * sizeof(*s->slab));	\
 		stat_ ##slabname## realloc++;				\
 		for (i = s->slab_count; i <= nth_slab; i++)		\
 			s->slab[i] = NULL;				\
-- 
1.8.5.427.g6d3141d

Re: [PATCH] commit-slab: sizeof() the right type in xrealloc

[Jeff King]

On Sun, Dec 01, 2013 at 09:41:55PM +0100, Thomas Rast wrote:

> When allocating the slab, the code accidentally computed the array
> size from s->slab (an elemtype**).  The slab is an array of elemtype*,
> however, so we should take the size of *s->slab.

Looks obviously correct.

> I browsed around for a while, and couldn't find out whether any
> architecture actually has any hope of running git (i.e. is at least
> mostly POSIX conformant) but still violates the assumption that all
> pointers[*] are the same size.
> 
> The comp.lang.c FAQ has some interesting examples of wildly different
> pointer representations at:
> 
>   http://c-faq.com/null/machexamp.html

Note that most of those examples are not different sizes, but rather
different null-pointer representations. We are already grossly out of
compliance with the standard there, as we typically use memset() to
zero-out structs with pointers.

-Peff