On Tue, Dec 7, 2021 at 1:33 PM Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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. > 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. > 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? 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. Thanks, Neeraj
