On December 7, 2021 5:22 PM, Neeraj Singh wrote: > 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_foc > > us=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. I don't know which compiler versions support this check, but it is customary to have a union with the largest allocation first, then smaller allocations following in separate sub-union fields. This may not match your intent. -Randall
