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

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: Jul 2, 2025, 22:42 UTC
Message-ID: <ep4q5xwbys4qwpkmmo5jujzjorrb24v5na4yuwpjr5owojwk2q@omb7xpp4oov5>
In-Reply-To: <xmqq5xgacn2w.fsf@gitster.g>

On Wed, Jul 02, 2025 at 02:14:15PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> 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