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

On December 7, 2021 5:22 PM, Neeraj Singh wrote:

On Tue, Dec 7, 2021 at 1:33 PM Johannes Schindelin
[off-list ref] wrote:

quoted

Hi Neeraj,

On Mon, 6 Dec 2021, Neeraj Singh wrote:

quoted

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.

quoted

See https://godbolt.org/z/TbcYhEW5d, it says:

        <source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized

array

quoted

        Compiler returned: 2

quoted

(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

quoted

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):

quoted

        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):

quoted

        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):

quoted

        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`).

quoted

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).

quoted

quoted

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