Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays

> Hi Neeraj,
> 
> On Mon, 6 Dec 2021, Neeraj Singh wrote:
> 
>> I'm a little confused by this issue. Looks like an empty flex array
>> is supported here: https://godbolt.org/z/j3ndYTEfx.
> 
> It is somewhat strange, but understandable, that the empty flex array at
> the end of a struct isn't triggering a compile error. But having a field
> _after_ that empty flex array _does_ trigger a compile error:
> 
> struct MyStruct {
>      int x;
>      int flexA[];
>      char string[256];
> };
> 
> Note the added `string` field.

> See https://godbolt.org/z/TbcYhEW5d, it says:
> 
> 	<source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized array
> 	Compiler returned: 2
> 
>> (Note that I'm passing /TC to force C compilation).
> 
> Yes, `/TC` is one of the flags we pass to MSVC. For more details, see
> https://github.com/git-for-windows/git/runs/4389081542?check_suite_focus=true#step:9:125
> 
>> Also, heap_fsentry doesn't appear to use a flex array in current sources.
> 
> It does, but it is admittedly a bit convoluted and not very easy to see.
> The definition of `heap_fsentry` is
> [this](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L77-L80):
> 
> 	struct heap_fsentry {
> 		struct fsentry ent;
> 		char dummy[MAX_LONG_PATH];
> 	};
> 
> No flex array here, right? But wait, there is a `struct fsentry`. Let's
> look at
> [that](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L43-L74):
> 
> 	struct fsentry {
> 		struct hashmap_entry ent;
> 		[...]
> 		/*
> 		 * Name of the entry. For directory listings: relative path of the
> 		 * directory, without trailing '/' (empty for cwd()). For file
> 		 * entries:
> 		 * name of the file. Typically points to the end of the structure
> 		 * if
> 		 * the fsentry is allocated on the heap (see fsentry_alloc), or to
> 		 * a
> 		 * local variable if on the stack (see fsentry_init).
> 		 */
> 		struct dirent dirent;
> 	};
> 
> Still no flex array, right? But wait, there is a `struct dirent`. Let's
> [see](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/dirent.h#L9-L12):
> 
> 	struct dirent {
> 		unsigned char d_type; /* file type to prevent lstat after readdir */
> 		char d_name[FLEX_ARRAY]; /* file name */
> 	};
> 
> Finally! We see the flex array.
> 
> Now, you may ask why is this even correct? How can you have an empty-sized
> field in a struct that is inside another struct that is inside yet another
> struct _and then followed by another field_?
> 
> The reason why this is correct and intended is that `struct dirent`
> intentionally leaves the length of the `d_name` undefined, to leave it to
> the implementation whether a fixed-size buffer is used or a
> specifically-allocated one of the exact correct size for a _specific_
> directory entry.
> 
> In FSCache, we want to allocate a large-enough buffer to fit _any_ file
> name, and it should not only contain the metadata in `struct dirent`, but
> additionally some FSCache-specific metadata.
> 
> Therefore, `struct fsentry` is kind of a subclass of `struct dirent`, and
> `struct heap_fsentry` is kind of a subclass of something that does not
> exist, a `struct dirent` that offers enough space to fit _any_ legal
> `d_name` (that is what that `dummy` field is for, it is not actually
> intended to be accessed except via `d_name`).
> 
>> If it does start using it, there issue may actually be elsewhere besides
>> the struct definition (it could be just a badly targeted compiler error).
>> We have code like `struct heap_fsentry key[2];`.  That declaration can't
>> work with a flex array.
> 
> I hope my explanation above made sense to you.
> 
> Admittedly, it is slightly icky code, but honestly, I do not have any
> splendid idea how to make it less complicated to understand. Do you?
> 
> Ciao,
> Dscho
>