From: Phillip Wood Date: Thu, 09 Dec 2021 11:00:55 GMT Subject: Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays Message-ID: In-Reply-To: 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: > > (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 >