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

I'm a little confused by this issue. Looks like an empty flex array is supported here: https://godbolt.org/z/j3ndYTEfx.

(Note that I'm passing /TC to force C compilation).

Also, heap_fsentry doesn't appear to use a flex array in current sources.

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.

Hi Neeraj,

Quoted 2 lines · Neeraj Singh

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

Quoted 1 line

(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 1 line

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

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?

Quoted 12 lines · Johannes Schindelin

Hi Neeraj,

Quoted 2 lines · Neeraj Singh

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:

Quoted 8 lines

struct MyStruct {
    int x;
    int flexA[];
    char string[256];
};

Note the added string field.

Please see https://godbolt.org/z/4Tb9PobYM. GCC throws a morally equivalent error. I don't think that's a valid usage of flex-array.

Quoted 3 lines

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

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

array

Quoted 8 lines

Compiler returned: 2

Quoted 1 line

(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_foc
us=true#step:9:125

Quoted 5 lines

Quoted 1 line

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

Quoted 9 lines

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

Quoted 17 lines

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

Quoted 3 lines

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

Quoted 22 lines

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.

Quoted 8 lines

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

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

Quoted 7 lines

Quoted 2 lines

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?

Thanks for explaining all of this. It was hard for me to see what was going on before.

So when trying the same thing with Clang, this construct is claimed to be a GNU extension: https://godbolt.org/z/q3ndr57Pf

The fix I'd propose (uncomment the line defining FIXED_DEF in godbolt) is to use a union where the char buf has the size of the fsentry _plus_ the desired buffer.

From: Johannes Schindelin <[email protected]>

This seems to be required to define FLEX_ARRAY in a manner that MSVC in C11 mode accepts. Without this fix, building Git for Windows' experimental FSCache code would fail thusly:

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>

This seems to be required to define FLEX_ARRAY in a manner that MSVC in C11 mode accepts. Without this fix, building Git for Windows' experimental FSCache code would fail thusly:

So, what's the final verdict on this one, after a few back and forth to clarify the "seems to be required" above?

In the meantime, I am tempted to queue the following as a pure clean-up.

Thoughts?

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] flex-array: simplify compiler-specific workaround

We use "type array[];" syntax for the flex-array member at the end of a struct under C99 or later, except when we are building with older SUNPRO_C compilers. As we find more vendor compilers that claim to grok C99 but not understand ts flex-array syntax, the existing "If we are using C99, but not with these compilers..." conditional will keep growing.

Make it more manageable by listing vendor-specific exceptions earlier, with the expectation that new exceptions will not be combined into existing ones to make the condition longer, and instead will be implemented as a new "#elif" in the cascade of #if/#elif/#else/#endif. E.g. if MSC is found to have a quirk similar to old SUNPRO_C, we can just add a single line

#elif defined(_MSC_VER)

immediately before "#elif defined(__GNUC__)" to cause us to fallback to the safer but a bit wasteful version.

Signed-off-by Junio C Hamano <[email protected]>

---
 git-compat-util.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

git-compat-util.h +9 -2

index c6bd2a84e5..9ba047a58e 100644
@@ -33,14 +33,23 @@
 /*
  * See if our compiler is known to support flexible array members.
  */
-#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))
-# define FLEX_ARRAY /* empty */
+
+/*
+ * Check vendor specific quirks first, before checking the
+ * __STDC_VERSION__, as vendor compilers can lie and we need to be
+ * able to work them around.  Note that by not defining FLEX_ARRAY
+ * here, we can fall back to use the "safer but a bit wasteful" one
+ * later.
+ */
+#if defined(__SUNPRO_C) && (__SUNPRO_C <= 0x580)
 #elif defined(__GNUC__)

Hi Neeraj,

Quoted 2 lines · Neeraj Singh

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

Quoted 1 line

(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 1 line

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

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

Quoted 8 lines

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.