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

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

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 <redacted>

---

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

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 <redacted>

---
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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>

---
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é

On 11/26/2021 6:36 AM, Philip Oakley wrote:

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

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

On 27/11/2021 07:32, René Scharfe wrote:

Am 26.11.21 um 12:36 schrieb Philip Oakley:

quoted

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 <redacted>

---
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.

quoted

---
 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

On 29/11/2021 14:44, Derrick Stolee wrote:

On 11/26/2021 6:36 AM, Philip Oakley wrote:

quoted

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 <redacted>
---
 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.

quoted

 	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

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

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

On 30/11/2021 00:35, Taylor Blau wrote:

On Fri, Nov 26, 2021 at 11:36:12AM +0000, Philip Oakley wrote:

quoted

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.

quoted

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