Hi Dscho
On 07/12/2021 21:33, Johannes Schindelin wrote:
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.
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
(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
