[PATCH] builtin/gc: improve total_ram calculation for HAVE_BSD_SYSCTL

[PATCH] builtin/gc: improve total_ram calculation for HAVE_BSD_SYSCTL

[Carlo Marcelo Arenas Belón]

In BSD systems other than macOS, since 9806f5a7bf (gc --auto:
exclude base pack if not enough mem to "repack -ad", 2018-04-15),
sysctl() use HW_PHYSMEM with the wrong size for the target.

Use the correct type for physical_memory on each option and make
sure it is initialized, so it is safe to use even if sysctl() fails.

While at it, add a cast to the returned value for consistency.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/gc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 845876ff02..3b2ca99af9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -539,21 +539,25 @@ static uint64_t total_ram(void)
 		return total;
 	}
 #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
-	int64_t physical_memory;
 	int mib[2];
 	size_t length;
-
-	mib[0] = CTL_HW;
 # if defined(HW_MEMSIZE)
+	int64_t physical_memory = 0;
+
 	mib[1] = HW_MEMSIZE;
 # elif defined(HW_PHYSMEM64)
+	int64_t physical_memory = 0;
+
 	mib[1] = HW_PHYSMEM64;
 # else
+	int physical_memory = 0;
+
 	mib[1] = HW_PHYSMEM;
 # endif
-	length = sizeof(int64_t);
+	mib[0] = CTL_HW;
+	length = sizeof(physical_memory);
 	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
-		return physical_memory;
+		return (uint64_t)physical_memory;
 #elif defined(GIT_WINDOWS_NATIVE)
 	MEMORYSTATUSEX memInfo;
 
-- 
2.50.0.147.gafe0d4ec5b

Re: [PATCH] builtin/gc: improve total_ram calculation for HAVE_BSD_SYSCTL

[Patrick Steinhardt]

On Wed, Jul 02, 2025 at 07:42:44AM -0700, Carlo Marcelo Arenas Belón wrote:
> In BSD systems other than macOS, since 9806f5a7bf (gc --auto:
> exclude base pack if not enough mem to "repack -ad", 2018-04-15),
> sysctl() use HW_PHYSMEM with the wrong size for the target.
> 
> Use the correct type for physical_memory on each option and make
> sure it is initialized, so it is safe to use even if sysctl() fails.

We don't use it though when sysctl(3) fails, do we? We only return
`physical_memory` in case sysctl(3) returned zero, which indicates
success. Which raises the question whether that function ever returns a
zero value without writing the value to the pointer.

Not that it would really hurt to initialize the value, but I found this
explanation to be puzzling.

> While at it, add a cast to the returned value for consistency.

Okay. It's not really needed, but other branches do it, as well.

Patrick

Re: [PATCH] builtin/gc: improve total_ram calculation for HAVE_BSD_SYSCTL

[Carlo Marcelo Arenas Belón]

On Wed, Jul 02, 2025 at 05:23:09PM -0800, Patrick Steinhardt wrote:
> On Wed, Jul 02, 2025 at 07:42:44AM -0700, Carlo Marcelo Arenas Belón wrote:
> > In BSD systems other than macOS, since 9806f5a7bf (gc --auto:
> > exclude base pack if not enough mem to "repack -ad", 2018-04-15),
> > sysctl() use HW_PHYSMEM with the wrong size for the target.
> > 
> > Use the correct type for physical_memory on each option and make
> > sure it is initialized, so it is safe to use even if sysctl() fails.
> 
> We don't use it though when sysctl(3) fails, do we? We only return
> `physical_memory` in case sysctl(3) returned zero, which indicates
> success. Which raises the question whether that function ever returns a
> zero value without writing the value to the pointer.
> 
> Not that it would really hurt to initialize the value, but I found this
> explanation to be puzzling.

Correct; the issue I was trying to address with the initialization was that
the function would return 0, but if there is a size mismatch between the
variable used and the size of HW_PHYSMEM then the other half of the variable
was used uninitialized.

Initializing it, "solves" the problem by making sure that (at least in little
endian) boxes the result was still valid.

Carlo

[PATCH v2] builtin/gc: protect against sysctl() failure in total_ram

[Carlo Marcelo Arenas Belón]

In the unlikely scenario that sysctl() fails, and uninitialized
value could be returned.

Initialize the variable used and make sure its expected size
was correct before using it.

While at it, add a cast for consistency.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
V1 would regress FreeBSD, so instead make sure that the obsoleted name isn't
used in OpenBSD/NetBSD instead

 builtin/gc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 845876ff02..3958707feb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -539,7 +539,7 @@ static uint64_t total_ram(void)
 		return total;
 	}
 #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
-	int64_t physical_memory;
+	int64_t physical_memory = 0;
 	int mib[2];
 	size_t length;
 
@@ -552,8 +552,9 @@ static uint64_t total_ram(void)
 	mib[1] = HW_PHYSMEM;
 # endif
 	length = sizeof(int64_t);
-	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
-		return physical_memory;
+	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0) &&
+	    length == sizeof(int64_t))
+		return (uint64_t)physical_memory;
 #elif defined(GIT_WINDOWS_NATIVE)
 	MEMORYSTATUSEX memInfo;
 
-- 
2.50.0.147.gafe0d4ec5b

Re: [PATCH v2] builtin/gc: protect against sysctl() failure in total_ram

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> In the unlikely scenario that sysctl() fails, and uninitialized
> value could be returned.
>
> Initialize the variable used and make sure its expected size
> was correct before using it.

Oh, that's interesting.  I wonder if the system returns physical
memory in 32 bits, we would want fall back doing something silly
like

	if (!sysctl(mib, 2, &i64_tmp, &length, NULL, 0)) {
		if (length == sizeof(i64_tmp))
			return i64_tmp;
		else if (length == 4 &&
                	 !sysctl(mib, 2, &i32_tmp, &length, NULL, 0) &&
			length == 4)
			return i32_tmp;
	}

> While at it, add a cast for consistency.

OK, I do not mind being more explicit than necessary, but wouldn't
"return X" take care of casting X to the expected return type of
that function?

Anyway, thanks for improvements.  Will queue.


> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> V1 would regress FreeBSD, so instead make sure that the obsoleted name isn't
> used in OpenBSD/NetBSD instead
>
>  builtin/gc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 845876ff02..3958707feb 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -539,7 +539,7 @@ static uint64_t total_ram(void)
>  		return total;
>  	}
>  #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
> -	int64_t physical_memory;
> +	int64_t physical_memory = 0;
>  	int mib[2];
>  	size_t length;
>  
> @@ -552,8 +552,9 @@ static uint64_t total_ram(void)
>  	mib[1] = HW_PHYSMEM;
>  # endif
>  	length = sizeof(int64_t);
> -	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
> -		return physical_memory;
> +	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0) &&
> +	    length == sizeof(int64_t))
> +		return (uint64_t)physical_memory;
>  #elif defined(GIT_WINDOWS_NATIVE)
>  	MEMORYSTATUSEX memInfo;

Re: [PATCH v2] builtin/gc: protect against sysctl() failure in total_ram

[Carlo Marcelo Arenas Belón]

On Wed, Jul 02, 2025 at 11:42:59AM -0800, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> 
> > In the unlikely scenario that sysctl() fails, and uninitialized
> > value could be returned.
> >
> > Initialize the variable used and make sure its expected size
> > was correct before using it.
> 
> Oh, that's interesting.  I wonder if the system returns physical
> memory in 32 bits, we would want fall back doing something silly
> like

Yes, a FreeBSD 32bit x86 system will report at most 4294963200 from
this call, even if using PAE and more memory is available. 

Eitherway, since the caller for this is ok with a smaller size than
real, shouldn't be that big of an issue IMHO.

> 	if (!sysctl(mib, 2, &i64_tmp, &length, NULL, 0)) {
> 		if (length == sizeof(i64_tmp))
> 			return i64_tmp;
> 		else if (length == 4 &&
>                 	 !sysctl(mib, 2, &i32_tmp, &length, NULL, 0) &&
> 			length == 4)
> 			return i32_tmp;
> 	}

probably a little bit less silly in 20250702202118.48742-1-carenas@gmail.com

> > While at it, add a cast for consistency.
> 
> OK, I do not mind being more explicit than necessary, but wouldn't
> "return X" take care of casting X to the expected return type of
> that function?

yes it does, but the "implicit" sign conversion will trigger a warning
eitherway, I had removed the cast in v3.

Carlo

Re: [PATCH v2] builtin/gc: protect against sysctl() failure in total_ram

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> yes it does, but the "implicit" sign conversion will trigger a warning

Ahh, OK.  That makes sense.  Compilers tend to go quiet when we tell
them we do mean what we are doing.  Thanks.

[PATCH v3] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Carlo Marcelo Arenas Belón]

The calls to sysctl() assume an 64-bit memory size, but the actual
size depends on the key name and platform, at least for HW_PHYSMEM.

Detect any mismatched reads, and make sure that any non used bytes
are correctly discarded before returning.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/gc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 845876ff02..c2f248052c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -539,7 +539,7 @@ static uint64_t total_ram(void)
 		return total;
 	}
 #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
-	int64_t physical_memory;
+	uint64_t physical_memory;
 	int mib[2];
 	size_t length;
 
@@ -551,9 +551,16 @@ static uint64_t total_ram(void)
 # else
 	mib[1] = HW_PHYSMEM;
 # endif
-	length = sizeof(int64_t);
-	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
+	length = sizeof(physical_memory);
+	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
+		if (length < sizeof(physical_memory)) {
+			unsigned bits = (sizeof(physical_memory) - length) * 8;
+
+			physical_memory <<= bits;
+			physical_memory >>= bits;
+		}
 		return physical_memory;
+	}
 #elif defined(GIT_WINDOWS_NATIVE)
 	MEMORYSTATUSEX memInfo;
 
-- 
2.50.0.145.g83014dc05f.dirty

Re: [PATCH v3] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> -	length = sizeof(int64_t);
> -	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
> +	length = sizeof(physical_memory);
> +	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
> +		if (length < sizeof(physical_memory)) {
> +			unsigned bits = (sizeof(physical_memory) - length) * 8;
> +
> +			physical_memory <<= bits;
> +			physical_memory >>= bits;

I do not quite understand this version.  Does the correctness of
this depend on the machine having a certain byte-order?  

The system call treats &physical_memory as a mere blob of bytes, and
may tell us that it filled only 4 bytes out of 8, but depending on
the endianness, left shifting 4*8 bits first may discard the real
information (i.e., big endian).

On a little endian 32-bit box, it might give us length == 4, filling
the lower half of the i64, and shifting by 32-bits to the left and
then shifting it down by 32-bits to the right may fill the upper half
with 1 if the result in the 4-byte long is more than 2GB because
the type of physical_memory is signed, and then we cast that value
to u64.  Which does not sound correct, either.

Would it make more sense to pass &u64 and return it only when
length==8 as you did in v2 while removing the need to cast?




> +		}
>  		return physical_memory;
> +	}
>  #elif defined(GIT_WINDOWS_NATIVE)
>  	MEMORYSTATUSEX memInfo;

Re: [PATCH v3] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Carlo Marcelo Arenas Belón]

On Wed, Jul 02, 2025 at 02:14:15PM -0800, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> 
> > -	length = sizeof(int64_t);
> > -	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
> > +	length = sizeof(physical_memory);
> > +	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
> > +		if (length < sizeof(physical_memory)) {
> > +			unsigned bits = (sizeof(physical_memory) - length) * 8;
> > +
> > +			physical_memory <<= bits;
> > +			physical_memory >>= bits;
> 
> I do not quite understand this version.  Does the correctness of
> this depend on the machine having a certain byte-order?  

Yes, sorry and as you pointed out it is obviously incorrect and should had
been instead something like (barelly tested, though so please let me make
sure and not waste more of your time)

  uint64_t physical_memory = 0;
  ...
  if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
  # if GIT_BYTE_ORDER == GIT_BIG_ENDIAN
  	if (length < sizeof(physical_memory)) {
  		unsigned bits = (sizeof(physical_memory) - length) * 8;

  		physical_memory >>= bits;
	}
  # endif
  	return physycal_nenory
  }
		
> then shifting it down by 32-bits to the right may fill the upper half
> with 1 if the result in the 4-byte long is more than 2GB because
> the type of physical_memory is signed, and then we cast that value
> to u64.  Which does not sound correct, either.

note that I changed the type to unsigned previously, but the rest was
obviously wrong.

the shifting was meant to be a cooler way to get those bits cleared,
because I thought that relying in the initialization wasn't as cool
from the previous comments.

> Would it make more sense to pass &u64 and return it only when
> length==8 as you did in v2 while removing the need to cast?

v2 (without the cast) is indeed enough and better, but my concern was
that this affects 32-bit FreeBSD which will be returning always 0.

a fixed version of this, would allow at least a better return, and
because most of the extra work is only needed in Big Endian (which
could only affect Power) then it is almost a free upgrade.

Carlo

Re: [PATCH v3] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

>> > +			physical_memory <<= bits;
>> > +			physical_memory >>= bits;
>> 
>> I do not quite understand this version.  Does the correctness of
>> this depend on the machine having a certain byte-order?  
> ...
> the shifting was meant to be a cooler way to get those bits cleared,
> because I thought that relying in the initialization wasn't as cool
> from the previous comments.

I more often have seen a pattern like

	physical_memory &= ((1U << bits) - 1);

for clearing the upper bits, but that's fine.

> a fixed version of this, would allow at least a better return, and
> because most of the extra work is only needed in Big Endian (which
> could only affect Power) then it is almost a free upgrade.

OK.  As this is not a performance-critical operation anyway, I am
perfectly OK with the dumb "we ask for 8 and be happy if the answer
is 8 bytes long.  Otherwise if the answer is 4, we prepare a u32 and
ask again with 4; other "possible" answer width like 2 or 6 are
probably not worth worrying about" ;-).

Thanks.

[PATCH v4] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Carlo Marcelo Arenas Belón]

The calls to sysctl() assume a 64-bit memory size for the variable
holding the value, but the actual size depends on the key name and
platform, at least for HW_PHYSMEM.

Detect any mismatched reads, and retry with a shorter variable as
needed.  Make the second tentatively optional in little endian
systems, as the variable was preinitialized and would work AS-IS.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/gc.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7dc94f243d..b680a1f739 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -517,7 +517,7 @@ static uint64_t total_ram(void)
 		return total;
 	}
 #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
-	int64_t physical_memory;
+	uint64_t physical_memory = 0;
 	int mib[2];
 	size_t length;
 
@@ -529,9 +529,18 @@ static uint64_t total_ram(void)
 # else
 	mib[1] = HW_PHYSMEM;
 # endif
-	length = sizeof(int64_t);
-	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
+	length = sizeof(physical_memory);
+	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
+# ifndef __LITTLE_ENDIAN__
+		if (length == 4) {
+			unsigned mem;
+
+			if (!sysctl(mib, 2, &mem, &length, NULL, 0))
+				physical_memory = mem;
+		}
+# endif
 		return physical_memory;
+	}
 #elif defined(GIT_WINDOWS_NATIVE)
 	MEMORYSTATUSEX memInfo;
 
-- 
2.50.0.rc0.48.gd8154328d9

Re: [PATCH v4] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

>  #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
> -	int64_t physical_memory;
> +	uint64_t physical_memory = 0;
>  	int mib[2];
>  	size_t length;
>  
> @@ -529,9 +529,18 @@ static uint64_t total_ram(void)
>  # else
>  	mib[1] = HW_PHYSMEM;
>  # endif
> -	length = sizeof(int64_t);
> -	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
> +	length = sizeof(physical_memory);
> +	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
> +# ifndef __LITTLE_ENDIAN__
> +		if (length == 4) {
> +			unsigned mem;

Don't we guarantee that uint32_t is always available?  If not, we
should and use that type instead of "unsigned", I think.

Also I do not quite see the point of limiting this fallback to
little-endian architectures.  It wouldn't be called from any
performance critical codepaths, would it?

Thanks.

> +			if (!sysctl(mib, 2, &mem, &length, NULL, 0))
> +				physical_memory = mem;
> +		}
> +# endif
>  		return physical_memory;
> +	}
>  #elif defined(GIT_WINDOWS_NATIVE)
>  	MEMORYSTATUSEX memInfo;

[PATCH v5] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Carlo Marcelo Arenas Belón]

The calls to sysctl() assume a 64-bit memory size for the variable
holding the value, but the actual size depends on the key name and
platform, at least for HW_PHYSMEM.

Detect any mismatched reads, and retry with a shorter variable
when needed.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/gc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7dc94f243d..6880f5b13d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -517,7 +517,7 @@ static uint64_t total_ram(void)
 		return total;
 	}
 #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
-	int64_t physical_memory;
+	uint64_t physical_memory;
 	int mib[2];
 	size_t length;
 
@@ -529,9 +529,16 @@ static uint64_t total_ram(void)
 # else
 	mib[1] = HW_PHYSMEM;
 # endif
-	length = sizeof(int64_t);
-	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
+	length = sizeof(physical_memory);
+	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0)) {
+		if (length == 4) {
+			uint32_t mem;
+
+			if (!sysctl(mib, 2, &mem, &length, NULL, 0))
+				physical_memory = mem;
+		}
 		return physical_memory;
+	}
 #elif defined(GIT_WINDOWS_NATIVE)
 	MEMORYSTATUSEX memInfo;
 
-- 
2.50.0.90591.ge5deae765f.dirty

Re: [PATCH v5] builtin/gc: correct total_ram calculation with HAVE_BSD_SYSCTL

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> The calls to sysctl() assume a 64-bit memory size for the variable
> holding the value, but the actual size depends on the key name and
> platform, at least for HW_PHYSMEM.
>
> Detect any mismatched reads, and retry with a shorter variable
> when needed.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/gc.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Thanks.  Let's declare victory and merge this to 'next'.