[PATCH 0/4] Fix LLP64 `(size_t)1` compatibility VS C4334 warnings

[PATCH 0/4] Fix LLP64 `(size_t)1` compatibility VS C4334 warnings

[Philip Oakley]

The Visual Studio MSVC compilation reports a number of C4334 "was 64-bit
shift intended" size mismatch warnings. In most of these cases a size_t
is ANDed (masked) with a bit shift of 1, or 1U. On LLP64 systems the unity
value is 32 bits, while size_t is 64 bits. 

The fix is to upcast the unity value to size_t.   

The first patch has also been reported [1] by René Scharfe as an extra patch
to the rs/mergesort series. That patch had been on maint.

The middle two patches are similar changes, though [2/4] is a uintptr_t.

The final patch is applied to object-file.c, which has recently been
renamed from sha1-file.c, so couldn't be applied to the earlier maint
branch.[2]

These fixes clear all the current C4334 warnings.

The patches can be squashed together if required.

[1] https://lore.kernel.org/git/7fbd4cf4-5f66-a4cd-0c41-e5b12d14d761@iee.email/
[2] https://lore.kernel.org/git/3e7af5d3-58fd-3a92-371f-3fa26cfe05a0@iee.email/

Philip Oakley (4):
  mergesort.c: LLP64 compatibility, upcast unity for left shift
  repack.c: LLP64 compatibility, upcast unity for left shift
  diffcore-delta.c: LLP64 compatibility, upcast unity for left shift
  object-file.c: LLP64 compatibility, upcast unity for left shift

 builtin/repack.c | 2 +-
 diffcore-delta.c | 6 +++---
 mergesort.c      | 2 +-
 object-file.c    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.34.0.rc1.windows.1.4.ga126985b17

[PATCH 1/4] mergesort.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

Visual Studio reports C4334 "was 64-bit shift intended" size mismatch
warning because of size miss-match.

Promote unity to the matching type to fit with the `&` operator.

Signed-off-by: Philip Oakley <philipoakley@iee.email>

---
This is the same fix that René Scharfe provided in 42c456ff81
(mergesort: avoid left shift overflow, 2021-11-16)

Use size_t to match n when building the bitmask for checking whether a
rank is occupied, instead of the default signed int.
---
 mergesort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergesort.c b/mergesort.c
index 6216835566..bd9c6ef8ee 100644
--- a/mergesort.c
+++ b/mergesort.c
@@ -63,7 +63,7 @@ void *llist_mergesort(void *list,
 		void *next = get_next_fn(list);
 		if (next)
 			set_next_fn(list, NULL);
-		for (i = 0; n & (1 << i); i++)
+		for (i = 0; n & ((size_t)1 << i); i++)
 			list = llist_merge(ranks[i], list, get_next_fn,
 					   set_next_fn, compare_fn);
 		n++;
-- 
2.34.0.rc1.windows.1.4.ga126985b17

Re: [PATCH 1/4] mergesort.c: LLP64 compatibility, upcast unity for left shift

[René Scharfe]

Am 26.11.21 um 12:36 schrieb Philip Oakley:
> Visual Studio reports C4334 "was 64-bit shift intended" size mismatch
> warning because of size miss-match.
>
> Promote unity to the matching type to fit with the `&` operator.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>
> ---
> This is the same fix that René Scharfe provided in 42c456ff81
> (mergesort: avoid left shift overflow, 2021-11-16)
>
> Use size_t to match n when building the bitmask for checking whether a
> rank is occupied, instead of the default signed int.

Fine with me -- it's just nicer to take the whole set.

René

> ---
>  mergesort.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mergesort.c b/mergesort.c
> index 6216835566..bd9c6ef8ee 100644
> --- a/mergesort.c
> +++ b/mergesort.c
> @@ -63,7 +63,7 @@ void *llist_mergesort(void *list,
>  		void *next = get_next_fn(list);
>  		if (next)
>  			set_next_fn(list, NULL);
> -		for (i = 0; n & (1 << i); i++)
> +		for (i = 0; n & ((size_t)1 << i); i++)
>  			list = llist_merge(ranks[i], list, get_next_fn,
>  					   set_next_fn, compare_fn);
>  		n++;
>

Re: [PATCH 1/4] mergesort.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

On 27/11/2021 07:32, René Scharfe wrote:
> Am 26.11.21 um 12:36 schrieb Philip Oakley:
>> Visual Studio reports C4334 "was 64-bit shift intended" size mismatch
>> warning because of size miss-match.
>>
>> Promote unity to the matching type to fit with the `&` operator.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>>
>> ---
>> This is the same fix that René Scharfe provided in 42c456ff81
>> (mergesort: avoid left shift overflow, 2021-11-16)
>>
>> Use size_t to match n when building the bitmask for checking whether a
>> rank is occupied, instead of the default signed int.
> Fine with me -- it's just nicer to take the whole set.
>
> René

Thanks, I'm happy either way if others feels it belongs better with your
mergesort series.
>
>> ---
>>  mergesort.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mergesort.c b/mergesort.c
>> index 6216835566..bd9c6ef8ee 100644
>> --- a/mergesort.c
>> +++ b/mergesort.c
>> @@ -63,7 +63,7 @@ void *llist_mergesort(void *list,
>>  		void *next = get_next_fn(list);
>>  		if (next)
>>  			set_next_fn(list, NULL);
>> -		for (i = 0; n & (1 << i); i++)
>> +		for (i = 0; n & ((size_t)1 << i); i++)
>>  			list = llist_merge(ranks[i], list, get_next_fn,
>>  					   set_next_fn, compare_fn);
>>  		n++;
>>
Philip

[PATCH 2/4] repack.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

Visual Studio reports C4334 "was 64-bit shift intended" warning
because of size miss-match.

Promote unity to the matching type to fit with the `&` operator.

Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
 builtin/repack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 0b2d1e5d82..6da66474fd 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -842,7 +842,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);
 
-			if (((uintptr_t)item->util) & (1 << ext)) {
+			if (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {
 				struct stat statbuffer;
 				if (!stat(fname_old, &statbuffer)) {
 					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
-- 
2.34.0.rc1.windows.1.4.ga126985b17

Re: [PATCH 2/4] repack.c: LLP64 compatibility, upcast unity for left shift

[Taylor Blau]

On Fri, Nov 26, 2021 at 11:36:12AM +0000, Philip Oakley wrote:
> Visual Studio reports C4334 "was 64-bit shift intended" warning
> because of size miss-match.

s/miss-/mis

> diff --git a/builtin/repack.c b/builtin/repack.c
> index 0b2d1e5d82..6da66474fd 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -842,7 +842,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  			fname_old = mkpathdup("%s-%s%s",
>  					packtmp, item->string, exts[ext].name);
>
> -			if (((uintptr_t)item->util) & (1 << ext)) {
> +			if (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {

This line blames to me from back in 2fcb03b52d (builtin/repack.c: don't
move existing packs out of the way, 2020-11-17).

The proposed fix here looks good to me (though we were never at any
practical risk of getting bitten by a down-cast here since the maximum
value for `ext` is 5).

Thanks,
Taylor

Re: [PATCH 2/4] repack.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

On 30/11/2021 00:35, Taylor Blau wrote:
> On Fri, Nov 26, 2021 at 11:36:12AM +0000, Philip Oakley wrote:
>> Visual Studio reports C4334 "was 64-bit shift intended" warning
>> because of size miss-match.
> s/miss-/mis

Thanks. Looks like "miss-pelling" is my blind spot. Will fix V2 soon.
>
>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index 0b2d1e5d82..6da66474fd 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -842,7 +842,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>>  			fname_old = mkpathdup("%s-%s%s",
>>  					packtmp, item->string, exts[ext].name);
>>
>> -			if (((uintptr_t)item->util) & (1 << ext)) {
>> +			if (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {
> This line blames to me from back in 2fcb03b52d (builtin/repack.c: don't
> move existing packs out of the way, 2020-11-17).
>
> The proposed fix here looks good to me (though we were never at any
> practical risk of getting bitten by a down-cast here since the maximum
> value for `ext` is 5).
Agreed. It's nice to get a head start on fixing a group of warnings.

> Thanks,
> Taylor
Philip

[PATCH 3/4] diffcore-delta.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

Visual Studio reports C4334 "was 64-bit shift intended" warning
because of size miss-match.

Promote unity to the matching type to fit with its subsequent operation.

Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
 diffcore-delta.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diffcore-delta.c b/diffcore-delta.c
index 5668ace60d..a4e86dfa38 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -133,10 +133,10 @@ static struct spanhash_top *hash_chars(struct repository *r,
 
 	i = INITIAL_HASH_SIZE;
 	hash = xmalloc(st_add(sizeof(*hash),
-			      st_mult(sizeof(struct spanhash), 1<<i)));
+			      st_mult(sizeof(struct spanhash), (size_t)1<<i)));
 	hash->alloc_log2 = i;
 	hash->free = INITIAL_FREE(i);
-	memset(hash->data, 0, sizeof(struct spanhash) * (1<<i));
+	memset(hash->data, 0, sizeof(struct spanhash) * ((size_t)1 << i));
 
 	n = 0;
 	accum1 = accum2 = 0;
@@ -159,7 +159,7 @@ static struct spanhash_top *hash_chars(struct repository *r,
 		n = 0;
 		accum1 = accum2 = 0;
 	}
-	QSORT(hash->data, 1ul << hash->alloc_log2, spanhash_cmp);
+	QSORT(hash->data, (size_t)1ul << hash->alloc_log2, spanhash_cmp);
 	return hash;
 }
 
-- 
2.34.0.rc1.windows.1.4.ga126985b17

Re: [PATCH 3/4] diffcore-delta.c: LLP64 compatibility, upcast unity for left shift

[Derrick Stolee]

On 11/26/2021 6:36 AM, Philip Oakley wrote:
> Visual Studio reports C4334 "was 64-bit shift intended" warning
> because of size miss-match.
> 
> Promote unity to the matching type to fit with its subsequent operation.
> 
> Signed-off-by: Philip Oakley <philipoakley@iee.email>
> ---
>  diffcore-delta.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/diffcore-delta.c b/diffcore-delta.c
> index 5668ace60d..a4e86dfa38 100644
> --- a/diffcore-delta.c
> +++ b/diffcore-delta.c
> @@ -133,10 +133,10 @@ static struct spanhash_top *hash_chars(struct repository *r,
>  
>  	i = INITIAL_HASH_SIZE;
>  	hash = xmalloc(st_add(sizeof(*hash),
> -			      st_mult(sizeof(struct spanhash), 1<<i)));
> +			      st_mult(sizeof(struct spanhash), (size_t)1<<i)));

This could use spaces around "<<"

>  	hash->alloc_log2 = i;
>  	hash->free = INITIAL_FREE(i);
> -	memset(hash->data, 0, sizeof(struct spanhash) * (1<<i));
> +	memset(hash->data, 0, sizeof(struct spanhash) * ((size_t)1 << i));

Especially because you correctly add them here.

Thanks,
-Stolee

Re: [PATCH 3/4] diffcore-delta.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

On 29/11/2021 14:44, Derrick Stolee wrote:
> On 11/26/2021 6:36 AM, Philip Oakley wrote:
>> Visual Studio reports C4334 "was 64-bit shift intended" warning
>> because of size miss-match.
>>
>> Promote unity to the matching type to fit with its subsequent operation.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>> ---
>>  diffcore-delta.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/diffcore-delta.c b/diffcore-delta.c
>> index 5668ace60d..a4e86dfa38 100644
>> --- a/diffcore-delta.c
>> +++ b/diffcore-delta.c
>> @@ -133,10 +133,10 @@ static struct spanhash_top *hash_chars(struct repository *r,
>>  
>>  	i = INITIAL_HASH_SIZE;
>>  	hash = xmalloc(st_add(sizeof(*hash),
>> -			      st_mult(sizeof(struct spanhash), 1<<i)));
>> +			      st_mult(sizeof(struct spanhash), (size_t)1<<i)));
> This could use spaces around "<<"
OK.
>
>>  	hash->alloc_log2 = i;
>>  	hash->free = INITIAL_FREE(i);
>> -	memset(hash->data, 0, sizeof(struct spanhash) * (1<<i));
>> +	memset(hash->data, 0, sizeof(struct spanhash) * ((size_t)1 << i));
> Especially because you correctly add them here.
True. The spacing isn't that consistent in the code base, but adding the
spaces here does look better.
>
> Thanks,
> -Stolee

Philip

[PATCH 4/4] object-file.c: LLP64 compatibility, upcast unity for left shift

[Philip Oakley]

Visual Studio reports C4334 "was 64-bit shift intended" warning because
of size miss-match.

Promote unity to the matching type to fit with the assignment.

Signed-off-by: Philip Oakley <philipoakley@iee.email>

---

This cannot be applied to the maint-2.32 branch as the earlier René Scharfe
patch had been, because the original sha1-file.c, to which the backport
would apply, has been renamed in e5afd4449d (object-file.c: rename
from sha1-file.c, 2020-12-31) which was merged in 8b327f1784
(Merge branch 'ma/sha1-is-a-hash', 2021-01-15)
---
 object-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/object-file.c b/object-file.c
index c3d866a287..da8821cb91 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2425,7 +2425,7 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 	struct strbuf buf = STRBUF_INIT;
 	size_t word_bits = bitsizeof(odb->loose_objects_subdir_seen[0]);
 	size_t word_index = subdir_nr / word_bits;
-	size_t mask = 1u << (subdir_nr % word_bits);
+	size_t mask = (size_t)1u << (subdir_nr % word_bits);
 	uint32_t *bitmap;
 
 	if (subdir_nr < 0 ||
-- 
2.34.0.rc1.windows.1.4.ga126985b17

Re: [PATCH 0/4] Fix LLP64 `(size_t)1` compatibility VS C4334 warnings

[Derrick Stolee]

On 11/26/2021 6:36 AM, Philip Oakley wrote:
> The Visual Studio MSVC compilation reports a number of C4334 "was 64-bit
> shift intended" size mismatch warnings. In most of these cases a size_t
> is ANDed (masked) with a bit shift of 1, or 1U. On LLP64 systems the unity
> value is 32 bits, while size_t is 64 bits. 
> 
> The fix is to upcast the unity value to size_t.   
> 
> The first patch has also been reported [1] by René Scharfe as an extra patch
> to the rs/mergesort series. That patch had been on maint.
> 
> The middle two patches are similar changes, though [2/4] is a uintptr_t.
> 
> The final patch is applied to object-file.c, which has recently been
> renamed from sha1-file.c, so couldn't be applied to the earlier maint
> branch.[2]
> 
> These fixes clear all the current C4334 warnings.

Thank you for these changes. They all are obviously correct.

I had one style nitpick that you could take or leave.

> The patches can be squashed together if required.

I'm fine either way.

Thanks,
-Stolee