Re: [PATCH 10/10] hashmap_for_each_entry(): work around MSVC's run-time check failure #3

[Junio C Hamano <gitster@pobox.com>]

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

> Whoa, wait.  If it is just that macro, can we perhaps do something
> like the attached patch?

I looked at all the uses of OFFSETOF_VAR() and I think the one used
for hashmap_for_each_entry() is the only instance that 'var' given
to it can legitimately be uninitialized, if typeof() were available.

Here are the findings.

#define hashmap_put_entry(map, keyvar, member) \
	container_of_or_null_offset(hashmap_put(map, &(keyvar)->member), \
				OFFSETOF_VAR(keyvar, member))

The keyvar is a pointer to the entry being placed in the map; it
must hold a valid one so the pointer-diff implementation of
OFFSETOF_VAR() should work fine, or we are putting garbage in to the
map.

#define hashmap_remove_entry(map, keyvar, member, keydata) \
	container_of_or_null_offset( \
			hashmap_remove(map, &(keyvar)->member, keydata), \
			OFFSETOF_VAR(keyvar, member))

The keyvar is used to match against an existing entry in the map to
be removed---it must have a valid value.

#define hashmap_for_each_entry(map, iter, var, member) \
	for (var = hashmap_iter_first_entry_offset(map, iter, \
						OFFSETOF_VAR(var, member)); \
		var; \
		var = hashmap_iter_next_entry_offset(iter, \
						OFFSETOF_VAR(var, member)))

This, as you discovered, can be fed an uninitialized var and the
first thing it does is to use OFFSETOF_VAR() on it in order to call
hashmap_iter_first_entry_offset().  After that, i.e. when we called
that function to start the loop, var is defined and we would be OK.

The trick I suggested is to initialize var to NULL before making the
call to hashmap_iter_first_entry_offset(), i.e.

	for (var = NULL, \
	     var = hashmap_iter_first_entry_offset(map, iter, \
						OFFSETOF_VAR(var, member)); \

#define hashmap_get_entry(map, keyvar, member, keydata) \
	container_of_or_null_offset( \
				hashmap_get(map, &(keyvar)->member, keydata), \
				OFFSETOF_VAR(keyvar, member))

Must be OK for the same reason _put_entry() is OK.

#define hashmap_get_next_entry(map, var, member) \
	container_of_or_null_offset(hashmap_get_next(map, &(var)->member), \
				OFFSETOF_VAR(var, member))

This tries to go to the next-equal-pointer starting from var, so var
must be valid already.

So, perhaps the attached may be a viable replacement that would be
more futureproof with less maintenance cost, I suspect.

Thanks.

--- >8 ----- cut here ----- >8 ---
Subject: hashmap_for_each_entry(): workaround MSVC's runtime check failure #3

The OFFSETOF_VAR(var, member) macro is implemented in terms of
offsetof(typeof(*var), member) with compilers that know typeof(),
but its fallback implemenation compares &(var->member) and (var) and
count the distance in bytes, i.e.

    ((uintptr_t)&(var)->member - (uintptr_t)(var))

MSVC's runtime check, when fed an uninitialized 'var', flags this as
a use of an uninitialized variable (and that is legit---uninitialized
contents of 'var' is subtracted) in a debug build.

After auditing all 6 uses of OFFSETOF_VAR(), 1 of them does feed a
potentially uninitialized 'var' to the macro in the beginning of the
for() loop:

    #define hashmap_for_each_entry(map, iter, var, member) \
            for (var = hashmap_iter_first_entry_offset(map, iter, \
                                                    OFFSETOF_VAR(var, member)); \
                    var; \
                    var = hashmap_iter_next_entry_offset(iter, \
                                                    OFFSETOF_VAR(var, member)))

We can work around this by making sure that var has _some_ value
when OFFSETOF_VAR() is called.  Strictly speaking, it invites
undefined behaviour to use NULL here if we end up with pointer
comparison, but MSVC runtime seems to be happy with it, and most
other systems have typeof() and don't even need pointer comparison
fallback code.

---
 hashmap.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git c/hashmap.h w/hashmap.h
index ef220de4c6..b011b394fe 100644
--- c/hashmap.h
+++ w/hashmap.h
@@ -449,7 +449,8 @@ static inline struct hashmap_entry *hashmap_iter_first(struct hashmap *map,
  * containing a @member which is a "struct hashmap_entry"
  */
 #define hashmap_for_each_entry(map, iter, var, member) \
-	for (var = hashmap_iter_first_entry_offset(map, iter, \
+	for (var = NULL, /* for systems without typeof */ \
+	     var = hashmap_iter_first_entry_offset(map, iter, \
 						OFFSETOF_VAR(var, member)); \
 		var; \
 		var = hashmap_iter_next_entry_offset(iter, \