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

Hi Dscho

On 07/12/2021 21:33, Johannes Schindelin wrote:

Hi Neeraj,

On Mon, 6 Dec 2021, Neeraj Singh wrote:

quoted

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.

Having a field after the flex array is a violation of the C standard. Section 6.7.2.1:
   ... the last member of a structure with more than one named member
   may have incomplete array type, such a structure (and any union
   containing, possibly recursively, a member that is such a structure)
   shall not be a member of a structure or an element of an array.

Note that the wording also forbids

struct A {
	int x;
	char flex[];
};

struct B {
	struct A a; /* This is forbidden */
};

There was a proposal a few years ago to relax that restriction [1] but it does not seem to be in the latest draft standard.

None of this helps fix the problem, but it does explain why MSVC complains.

Best Wishes

Phillip

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2083.htm

See https://godbolt.org/z/TbcYhEW5d, it says:

	<source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized array
	Compiler returned: 2

quoted

(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

quoted

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

quoted

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