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

[<rsbecker@nexbridge.com>]

On December 7, 2021 5:22 PM, Neeraj Singh wrote:
> On Tue, Dec 7, 2021 at 1:33 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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