{"thread":{"id":"57041","subject":"[PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","startedAt":"2021-12-06T20:48:48Z","lastAt":"2021-12-09T21:18:36Z","messageCount":10,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Neeraj Singh","Johannes Schindelin","rsbecker@nexbridge.com","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"443196","messageId":"pull.1094.git.1638823724410.gitgitgadget@gmail.com","threadId":"57041","inReplyTo":null,"subject":"[PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-06T20:48:44Z","receivedAt":"2021-12-06T20:48:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThis seems to be required to define `FLEX_ARRAY` in a manner that MSVC\nin C11 mode accepts. Without this fix, building Git for Windows'\nexperimental FSCache code would fail thusly:\n\n\terror C2229: struct 'heap_fsentry' has an illegal zero-sized array\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    git-compat-util(msvc): C11 does not imply support for zero-sized arrays\n    \n    In Git for Windows' continuously-rebased branches, I found this problem\n    [https://github.com/git-for-windows/git/runs/4431149285?check_suite_focus=true#step:9:14507]\n    (which uses FLEX_ARRAY correctly, but fails because FLEX_ARRAY is no\n    longer defined as required by MS Visual C).\n    \n    This patch is based on bc/require-c99.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1094%2Fdscho%2Fflex-array-and-msvc-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1094/dscho/flex-array-and-msvc-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1094\n\n git-compat-util.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 19943e214ba..c9f508b3a83 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -46,7 +46,7 @@\n /*\n  * See if our compiler is known to support flexible array members.\n  */\n-#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))\n+#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && !defined(_MSC_VER) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))\n # define FLEX_ARRAY /* empty */\n #elif defined(__GNUC__)\n # if (__GNUC__ >= 3)\n\nbase-commit: 7bc341e21b566c6685b7d993ca80459f9994be38\n-- \ngitgitgadget\n"},{"id":"443201","messageId":"xmqqtuflcsl6.fsf@gitster.g","threadId":"57041","inReplyTo":"pull.1094.git.1638823724410.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T21:48:53Z","receivedAt":"2021-12-06T21:49:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 19943e214ba..c9f508b3a83 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -46,7 +46,7 @@\n>  /*\n>   * See if our compiler is known to support flexible array members.\n>   */\n> -#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))\n> +#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && !defined(_MSC_VER) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))\n>  # define FLEX_ARRAY /* empty */\n>  #elif defined(__GNUC__)\n>  # if (__GNUC__ >= 3)\n\nIs flex array a requirement for STDC 199901L or newer?\n\nI am assuming it is (e.g. https://en.wikipedia.org/wiki/C99), and I\ncan see that it is an unreliable source of information, given that\nwe see the second vendor whose conformance statement __STDC_VERSION__\nmakes contradicts with what the compiler does.\n\nIt might be that future MSC starts supporting flex array and this\nline will become even harder to grok when it happens.  I'd rather\nsee us cleaning up the mess first.\n\nSpecifically, I am wondering if we should use the test based on\n__STDC_VERSION__ as the last resort, giving the precedence to more\nvendor specific tests, to avoid future problems.  That way, we can\ncater to both camps of compiler vendors, the ones where their\nSTDC_VERSION may not say they have C99 but they do support flex\narray the modern way, and the others where their STDC_VERSION say\nthey have C99 but the don't do flex array.\n\nHow about a preliminary clean-up patch that brings us to the\npreimage of the following patchlet in step [1/2]?  Then we can do\nthe single-liner addition of _MSC_VER to support you, and the end\nresult would be a lot easier to read and maintain?\n\n    /* Vendor specific exceptions first */\n    #if   defined(__SUNPRO_C) && (__SUNPRO_C <= 0x580)\n   +#elif defined(_MSC_VER)\n    #elif defined(__GNUC__)\n    # if (__GNUC__ >= 3)\n    #  define FLEX_ARRAY /* empty */\n    # else\n    #  define FLEX_ARRAY 0 /* older GNU extension */\n    # endif\n    #elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L)\n    # define FLEX_ARRAY /* empty */\n    #endif\n\n    /* Otherwise default to safer but wasteful */\n    #ifndef FLEX_ARRAY\n    # define FLEX_ARRAY 1\n    #endif\n\nThanks.\n"},{"id":"443209","messageId":"20211206222539.GA27821@neerajsi-x1.localdomain","threadId":"57041","inReplyTo":"pull.1094.git.1638823724410.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-12-06T22:25:39Z","receivedAt":"2021-12-06T22:25:43Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"I'm a little confused by this issue. Looks like an empty flex array\nis supported here: https://godbolt.org/z/j3ndYTEfx. \n(Note that I'm passing /TC to force C compilation).\n\nAlso, heap_fsentry doesn't appear to use a flex array in current sources.\n\nIf it does start using it, there issue may actually be elsewhere besides\nthe struct definition (it could be just a badly targeted compiler error).\nWe have code like `struct heap_fsentry key[2];`.  That declaration can't\nwork with a flex array.\n\nThanks,\nNeeraj\n(wearing the ex-MSVC dev hat)\n"},{"id":"443373","messageId":"nycvar.QRO.7.76.6.2112072217280.90@tvgsbejvaqbjf.bet","threadId":"57041","inReplyTo":"20211206222539.GA27821@neerajsi-x1.localdomain","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-12-07T21:33:48Z","receivedAt":"2021-12-07T21:33:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Neeraj,\n\nOn Mon, 6 Dec 2021, Neeraj Singh wrote:\n\n> I'm a little confused by this issue. Looks like an empty flex array\n> is supported here: https://godbolt.org/z/j3ndYTEfx.\n\nIt is somewhat strange, but understandable, that the empty flex array at\nthe end of a struct isn't triggering a compile error. But having a field\n_after_ that empty flex array _does_ trigger a compile error:\n\nstruct MyStruct {\n    int x;\n    int flexA[];\n    char string[256];\n};\n\nNote the added `string` field.\n\nSee https://godbolt.org/z/TbcYhEW5d, it says:\n\n\t<source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized array\n\tCompiler returned: 2\n\n> (Note that I'm passing /TC to force C compilation).\n\nYes, `/TC` is one of the flags we pass to MSVC. For more details, see\nhttps://github.com/git-for-windows/git/runs/4389081542?check_suite_focus=true#step:9:125\n\n> Also, heap_fsentry doesn't appear to use a flex array in current sources.\n\nIt does, but it is admittedly a bit convoluted and not very easy to see.\nThe definition of `heap_fsentry` is\n[this](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L77-L80):\n\n\tstruct heap_fsentry {\n\t\tstruct fsentry ent;\n\t\tchar dummy[MAX_LONG_PATH];\n\t};\n\nNo flex array here, right? But wait, there is a `struct fsentry`. Let's\nlook at\n[that](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L43-L74):\n\n\tstruct fsentry {\n\t\tstruct hashmap_entry ent;\n\t\t[...]\n\t\t/*\n\t\t * Name of the entry. For directory listings: relative path of the\n\t\t * directory, without trailing '/' (empty for cwd()). For file\n\t\t * entries:\n\t\t * name of the file. Typically points to the end of the structure\n\t\t * if\n\t\t * the fsentry is allocated on the heap (see fsentry_alloc), or to\n\t\t * a\n\t\t * local variable if on the stack (see fsentry_init).\n\t\t */\n\t\tstruct dirent dirent;\n\t};\n\nStill no flex array, right? But wait, there is a `struct dirent`. Let's\n[see](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/dirent.h#L9-L12):\n\n\tstruct dirent {\n\t\tunsigned char d_type; /* file type to prevent lstat after readdir */\n\t\tchar d_name[FLEX_ARRAY]; /* file name */\n\t};\n\nFinally! We see the flex array.\n\nNow, you may ask why is this even correct? How can you have an empty-sized\nfield in a struct that is inside another struct that is inside yet another\nstruct _and then followed by another field_?\n\nThe reason why this is correct and intended is that `struct dirent`\nintentionally leaves the length of the `d_name` undefined, to leave it to\nthe implementation whether a fixed-size buffer is used or a\nspecifically-allocated one of the exact correct size for a _specific_\ndirectory entry.\n\nIn FSCache, we want to allocate a large-enough buffer to fit _any_ file\nname, and it should not only contain the metadata in `struct dirent`, but\nadditionally some FSCache-specific metadata.\n\nTherefore, `struct fsentry` is kind of a subclass of `struct dirent`, and\n`struct heap_fsentry` is kind of a subclass of something that does not\nexist, a `struct dirent` that offers enough space to fit _any_ legal\n`d_name` (that is what that `dummy` field is for, it is not actually\nintended to be accessed except via `d_name`).\n\n> If it does start using it, there issue may actually be elsewhere besides\n> the struct definition (it could be just a badly targeted compiler error).\n> We have code like `struct heap_fsentry key[2];`.  That declaration can't\n> work with a flex array.\n\nI hope my explanation above made sense to you.\n\nAdmittedly, it is slightly icky code, but honestly, I do not have any\nsplendid idea how to make it less complicated to understand. Do you?\n\nCiao,\nDscho\n"},{"id":"443381","messageId":"CANQDOdfkRMMWpF2gaRpZW0NRzMuN-ADO++D1J4rS8WLxOPudRw@mail.gmail.com","threadId":"57041","inReplyTo":"nycvar.QRO.7.76.6.2112072217280.90@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-12-07T22:22:23Z","receivedAt":"2021-12-07T22:22:37Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Tue, Dec 7, 2021 at 1:33 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Neeraj,\n>\n> On Mon, 6 Dec 2021, Neeraj Singh wrote:\n>\n> > I'm a little confused by this issue. Looks like an empty flex array\n> > is supported here: https://godbolt.org/z/j3ndYTEfx.\n>\n> It is somewhat strange, but understandable, that the empty flex array at\n> the end of a struct isn't triggering a compile error. But having a field\n> _after_ that empty flex array _does_ trigger a compile error:\n>\n> struct MyStruct {\n>     int x;\n>     int flexA[];\n>     char string[256];\n> };\n>\n> Note the added `string` field.\n>\n\nPlease see https://godbolt.org/z/4Tb9PobYM.  GCC throws a morally\nequivalent error.  I don't think that's a valid usage of flex-array.\n\n> See https://godbolt.org/z/TbcYhEW5d, it says:\n>\n>         <source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized array\n>         Compiler returned: 2\n>\n> > (Note that I'm passing /TC to force C compilation).\n>\n> Yes, `/TC` is one of the flags we pass to MSVC. For more details, see\n> https://github.com/git-for-windows/git/runs/4389081542?check_suite_focus=true#step:9:125\n>\n> > Also, heap_fsentry doesn't appear to use a flex array in current sources.\n>\n> It does, but it is admittedly a bit convoluted and not very easy to see.\n> The definition of `heap_fsentry` is\n> [this](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L77-L80):\n>\n>         struct heap_fsentry {\n>                 struct fsentry ent;\n>                 char dummy[MAX_LONG_PATH];\n>         };\n>\n> No flex array here, right? But wait, there is a `struct fsentry`. Let's\n> look at\n> [that](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L43-L74):\n>\n>         struct fsentry {\n>                 struct hashmap_entry ent;\n>                 [...]\n>                 /*\n>                  * Name of the entry. For directory listings: relative path of the\n>                  * directory, without trailing '/' (empty for cwd()). For file\n>                  * entries:\n>                  * name of the file. Typically points to the end of the structure\n>                  * if\n>                  * the fsentry is allocated on the heap (see fsentry_alloc), or to\n>                  * a\n>                  * local variable if on the stack (see fsentry_init).\n>                  */\n>                 struct dirent dirent;\n>         };\n>\n> Still no flex array, right? But wait, there is a `struct dirent`. Let's\n> [see](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/dirent.h#L9-L12):\n>\n>         struct dirent {\n>                 unsigned char d_type; /* file type to prevent lstat after readdir */\n>                 char d_name[FLEX_ARRAY]; /* file name */\n>         };\n>\n> Finally! We see the flex array.\n>\n> Now, you may ask why is this even correct? How can you have an empty-sized\n> field in a struct that is inside another struct that is inside yet another\n> struct _and then followed by another field_?\n>\n> The reason why this is correct and intended is that `struct dirent`\n> intentionally leaves the length of the `d_name` undefined, to leave it to\n> the implementation whether a fixed-size buffer is used or a\n> specifically-allocated one of the exact correct size for a _specific_\n> directory entry.\n>\n> In FSCache, we want to allocate a large-enough buffer to fit _any_ file\n> name, and it should not only contain the metadata in `struct dirent`, but\n> additionally some FSCache-specific metadata.\n>\n> Therefore, `struct fsentry` is kind of a subclass of `struct dirent`, and\n> `struct heap_fsentry` is kind of a subclass of something that does not\n> exist, a `struct dirent` that offers enough space to fit _any_ legal\n> `d_name` (that is what that `dummy` field is for, it is not actually\n> intended to be accessed except via `d_name`).\n>\n> > If it does start using it, there issue may actually be elsewhere besides\n> > the struct definition (it could be just a badly targeted compiler error).\n> > We have code like `struct heap_fsentry key[2];`.  That declaration can't\n> > work with a flex array.\n>\n> I hope my explanation above made sense to you.\n>\n> Admittedly, it is slightly icky code, but honestly, I do not have any\n> splendid idea how to make it less complicated to understand. Do you?\n\nThanks for explaining all of this.  It was hard for me to see what was\ngoing on before.\n\nSo when trying the same thing with Clang, this construct is claimed to\nbe a GNU extension: https://godbolt.org/z/q3ndr57Pf\n\nThe fix I'd propose (uncomment the line defining FIXED_DEF in godbolt)\nis to use a union where the char buf has the size of the fsentry\n_plus_ the desired buffer.\n\nThanks,\nNeeraj\n"},{"id":"443383","messageId":"010c01d7ebbe$255a0890$700e19b0$@nexbridge.com","threadId":"57041","inReplyTo":"CANQDOdfkRMMWpF2gaRpZW0NRzMuN-ADO++D1J4rS8WLxOPudRw@mail.gmail.com","subject":"RE: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-12-07T22:59:47Z","receivedAt":"2021-12-07T22:59:59Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 7, 2021 5:22 PM, Neeraj Singh wrote:\n> On Tue, Dec 7, 2021 at 1:33 PM Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >\n> > Hi Neeraj,\n> >\n> > On Mon, 6 Dec 2021, Neeraj Singh wrote:\n> >\n> > > I'm a little confused by this issue. Looks like an empty flex array\n> > > is supported here: https://godbolt.org/z/j3ndYTEfx.\n> >\n> > It is somewhat strange, but understandable, that the empty flex array\n> > at the end of a struct isn't triggering a compile error. But having a\n> > field _after_ that empty flex array _does_ trigger a compile error:\n> >\n> > struct MyStruct {\n> >     int x;\n> >     int flexA[];\n> >     char string[256];\n> > };\n> >\n> > Note the added `string` field.\n> >\n> \n> Please see https://godbolt.org/z/4Tb9PobYM.  GCC throws a morally\n> equivalent error.  I don't think that's a valid usage of flex-array.\n> \n> > See https://godbolt.org/z/TbcYhEW5d, it says:\n> >\n> >         <source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized\n> array\n> >         Compiler returned: 2\n> >\n> > > (Note that I'm passing /TC to force C compilation).\n> >\n> > Yes, `/TC` is one of the flags we pass to MSVC. For more details, see\n> > https://github.com/git-for-windows/git/runs/4389081542?check_suite_foc\n> > us=true#step:9:125\n> >\n> > > Also, heap_fsentry doesn't appear to use a flex array in current sources.\n> >\n> > It does, but it is admittedly a bit convoluted and not very easy to see.\n> > The definition of `heap_fsentry` is\n> > [this](https://github.com/git-for-\n> windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L77-L80):\n> >\n> >         struct heap_fsentry {\n> >                 struct fsentry ent;\n> >                 char dummy[MAX_LONG_PATH];\n> >         };\n> >\n> > No flex array here, right? But wait, there is a `struct fsentry`.\n> > Let's look at\n> > [that](https://github.com/git-for-\n> windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L43-L74):\n> >\n> >         struct fsentry {\n> >                 struct hashmap_entry ent;\n> >                 [...]\n> >                 /*\n> >                  * Name of the entry. For directory listings: relative path of the\n> >                  * directory, without trailing '/' (empty for cwd()). For file\n> >                  * entries:\n> >                  * name of the file. Typically points to the end of the structure\n> >                  * if\n> >                  * the fsentry is allocated on the heap (see fsentry_alloc), or to\n> >                  * a\n> >                  * local variable if on the stack (see fsentry_init).\n> >                  */\n> >                 struct dirent dirent;\n> >         };\n> >\n> > Still no flex array, right? But wait, there is a `struct dirent`.\n> > Let's\n> > [see](https://github.com/git-for-\n> windows/git/blob/v2.34.1.windows.1/compat/win32/dirent.h#L9-L12):\n> >\n> >         struct dirent {\n> >                 unsigned char d_type; /* file type to prevent lstat after readdir */\n> >                 char d_name[FLEX_ARRAY]; /* file name */\n> >         };\n> >\n> > Finally! We see the flex array.\n> >\n> > Now, you may ask why is this even correct? How can you have an\n> > empty-sized field in a struct that is inside another struct that is\n> > inside yet another struct _and then followed by another field_?\n> >\n> > The reason why this is correct and intended is that `struct dirent`\n> > intentionally leaves the length of the `d_name` undefined, to leave it\n> > to the implementation whether a fixed-size buffer is used or a\n> > specifically-allocated one of the exact correct size for a _specific_\n> > directory entry.\n> >\n> > In FSCache, we want to allocate a large-enough buffer to fit _any_\n> > file name, and it should not only contain the metadata in `struct\n> > dirent`, but additionally some FSCache-specific metadata.\n> >\n> > Therefore, `struct fsentry` is kind of a subclass of `struct dirent`,\n> > and `struct heap_fsentry` is kind of a subclass of something that does\n> > not exist, a `struct dirent` that offers enough space to fit _any_\n> > legal `d_name` (that is what that `dummy` field is for, it is not\n> > actually intended to be accessed except via `d_name`).\n> >\n> > > If it does start using it, there issue may actually be elsewhere\n> > > besides the struct definition (it could be just a badly targeted compiler\n> error).\n> > > We have code like `struct heap_fsentry key[2];`.  That declaration\n> > > can't work with a flex array.\n> >\n> > I hope my explanation above made sense to you.\n> >\n> > Admittedly, it is slightly icky code, but honestly, I do not have any\n> > splendid idea how to make it less complicated to understand. Do you?\n> \n> Thanks for explaining all of this.  It was hard for me to see what was going on\n> before.\n> \n> So when trying the same thing with Clang, this construct is claimed to be a\n> GNU extension: https://godbolt.org/z/q3ndr57Pf\n> \n> The fix I'd propose (uncomment the line defining FIXED_DEF in godbolt) is to\n> use a union where the char buf has the size of the fsentry _plus_ the desired\n> buffer.\n\nI 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.\n\n-Randall\n\n"},{"id":"443558","messageId":"xmqqee6mv9no.fsf@gitster.g","threadId":"57041","inReplyTo":"pull.1094.git.1638823724410.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T01:39:39Z","receivedAt":"2021-12-09T01:39:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> This seems to be required to define `FLEX_ARRAY` in a manner that MSVC\n> in C11 mode accepts. Without this fix, building Git for Windows'\n> experimental FSCache code would fail thusly:\n\nSo, what's the final verdict on this one, after a few back and forth\nto clarify the \"seems to be required\" above?\n\nIn the meantime, I am tempted to queue the following as a pure\nclean-up.\n\nThoughts?\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] flex-array: simplify compiler-specific workaround\n\nWe use \"type array[];\" syntax for the flex-array member at the end\nof a struct under C99 or later, except when we are building with\nolder SUNPRO_C compilers.  As we find more vendor compilers that\nclaim to grok C99 but not understand  ts flex-array syntax, the\nexisting \"If we are using C99, but not with these compilers...\"\nconditional will keep growing.\n\nMake it more manageable by listing vendor-specific exceptions\nearlier, with the expectation that new exceptions will not be\ncombined into existing ones to make the condition longer, and\ninstead will be implemented as a new \"#elif\" in the cascade of\n#if/#elif/#else/#endif.  E.g. if MSC is found to have a quirk\nsimilar to old SUNPRO_C, we can just add a single line \n\n    #elif defined(_MSC_VER)\n\nimmediately before \"#elif defined(__GNUC__)\" to cause us to fallback\nto the safer but a bit wasteful version.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-compat-util.h | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git c/git-compat-util.h w/git-compat-util.h\nindex c6bd2a84e5..9ba047a58e 100644\n--- c/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -33,14 +33,23 @@\n /*\n  * See if our compiler is known to support flexible array members.\n  */\n-#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))\n-# define FLEX_ARRAY /* empty */\n+\n+/*\n+ * Check vendor specific quirks first, before checking the\n+ * __STDC_VERSION__, as vendor compilers can lie and we need to be\n+ * able to work them around.  Note that by not defining FLEX_ARRAY\n+ * here, we can fall back to use the \"safer but a bit wasteful\" one\n+ * later.\n+ */\n+#if defined(__SUNPRO_C) && (__SUNPRO_C <= 0x580)\n #elif defined(__GNUC__)\n # if (__GNUC__ >= 3)\n #  define FLEX_ARRAY /* empty */\n # else\n #  define FLEX_ARRAY 0 /* older GNU extension */\n # endif\n+#elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L)\n+# define FLEX_ARRAY /* empty */\n #endif\n \n /*\n\n\n"},{"id":"443559","messageId":"20211209014913.GA15497@neerajsi-x1.localdomain","threadId":"57041","inReplyTo":"xmqqee6mv9no.fsf@gitster.g","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-12-09T01:49:13Z","receivedAt":"2021-12-09T01:49:19Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Wed, Dec 08, 2021 at 05:39:39PM -0800, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n> \n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > This seems to be required to define `FLEX_ARRAY` in a manner that MSVC\n> > in C11 mode accepts. Without this fix, building Git for Windows'\n> > experimental FSCache code would fail thusly:\n> \n> So, what's the final verdict on this one, after a few back and forth\n> to clarify the \"seems to be required\" above?\n> \n> In the meantime, I am tempted to queue the following as a pure\n> clean-up.\n> \n> Thoughts?\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] flex-array: simplify compiler-specific workaround\n> \n> We use \"type array[];\" syntax for the flex-array member at the end\n> of a struct under C99 or later, except when we are building with\n> older SUNPRO_C compilers.  As we find more vendor compilers that\n> claim to grok C99 but not understand  ts flex-array syntax, the\n> existing \"If we are using C99, but not with these compilers...\"\n> conditional will keep growing.\n> \n> Make it more manageable by listing vendor-specific exceptions\n> earlier, with the expectation that new exceptions will not be\n> combined into existing ones to make the condition longer, and\n> instead will be implemented as a new \"#elif\" in the cascade of\n> #if/#elif/#else/#endif.  E.g. if MSC is found to have a quirk\n> similar to old SUNPRO_C, we can just add a single line \n> \n>     #elif defined(_MSC_VER)\n> \n> immediately before \"#elif defined(__GNUC__)\" to cause us to fallback\n> to the safer but a bit wasteful version.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  git-compat-util.h | 13 +++++++++++--\n>  1 file changed, 11 insertions(+), 2 deletions(-)\n> \n> diff --git c/git-compat-util.h w/git-compat-util.h\n> index c6bd2a84e5..9ba047a58e 100644\n> --- c/git-compat-util.h\n> +++ w/git-compat-util.h\n> @@ -33,14 +33,23 @@\n>  /*\n>   * See if our compiler is known to support flexible array members.\n>   */\n> -#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && (!defined(__SUNPRO_C) || (__SUNPRO_C > 0x580))\n> -# define FLEX_ARRAY /* empty */\n> +\n> +/*\n> + * Check vendor specific quirks first, before checking the\n> + * __STDC_VERSION__, as vendor compilers can lie and we need to be\n> + * able to work them around.  Note that by not defining FLEX_ARRAY\n> + * here, we can fall back to use the \"safer but a bit wasteful\" one\n> + * later.\n> + */\n> +#if defined(__SUNPRO_C) && (__SUNPRO_C <= 0x580)\n>  #elif defined(__GNUC__)\n>  # if (__GNUC__ >= 3)\n>  #  define FLEX_ARRAY /* empty */\n>  # else\n>  #  define FLEX_ARRAY 0 /* older GNU extension */\n>  # endif\n> +#elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L)\n> +# define FLEX_ARRAY /* empty */\n>  #endif\n>  \n>  /*\n> \n> \n\nYour change is an improvement that should be easier to read and avoid conflicts when adding quirks for compilers.\n\nLGTM.\n\nI hope on Dscho's side that they change the fscache code to use the union, since I don't believe the current code\nis C-standards compliant and at least in this case MSVC is not actually broken.\n\n-Neeraj\n"},{"id":"443644","messageId":"f245f2b7-ca00-5497-3d1d-658c4c0fbcb6@gmail.com","threadId":"57041","inReplyTo":"nycvar.QRO.7.76.6.2112072217280.90@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-12-09T11:00:55Z","receivedAt":"2021-12-09T11:00:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 07/12/2021 21:33, Johannes Schindelin wrote:\n> Hi Neeraj,\n> \n> On Mon, 6 Dec 2021, Neeraj Singh wrote:\n> \n>> I'm a little confused by this issue. Looks like an empty flex array\n>> is supported here: https://godbolt.org/z/j3ndYTEfx.\n> \n> It is somewhat strange, but understandable, that the empty flex array at\n> the end of a struct isn't triggering a compile error. But having a field\n> _after_ that empty flex array _does_ trigger a compile error:\n> \n> struct MyStruct {\n>      int x;\n>      int flexA[];\n>      char string[256];\n> };\n> \n> Note the added `string` field.\n\nHaving a field after the flex array is a violation of the C standard. \nSection 6.7.2.1:\n    ... the last member of a structure with more than one named member\n    may have incomplete array type, such a structure (and any union\n    containing, possibly recursively, a member that is such a structure)\n    shall not be a member of a structure or an element of an array.\n\nNote that the wording also forbids\n\nstruct A {\n\tint x;\n\tchar flex[];\n};\n\nstruct B {\n\tstruct A a; /* This is forbidden */\n};\n\nThere was a proposal a few years ago to relax that restriction [1] but \nit does not seem to be in the latest draft standard.\n\nNone of this helps fix the problem, but it does explain why MSVC complains.\n\nBest Wishes\n\nPhillip\n\n[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2083.htm\n\n> See https://godbolt.org/z/TbcYhEW5d, it says:\n> \n> \t<source>(5): error C2229: struct 'MyStruct' has an illegal zero-sized array\n> \tCompiler returned: 2\n> \n>> (Note that I'm passing /TC to force C compilation).\n> \n> Yes, `/TC` is one of the flags we pass to MSVC. For more details, see\n> https://github.com/git-for-windows/git/runs/4389081542?check_suite_focus=true#step:9:125\n> \n>> Also, heap_fsentry doesn't appear to use a flex array in current sources.\n> \n> It does, but it is admittedly a bit convoluted and not very easy to see.\n> The definition of `heap_fsentry` is\n> [this](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L77-L80):\n> \n> \tstruct heap_fsentry {\n> \t\tstruct fsentry ent;\n> \t\tchar dummy[MAX_LONG_PATH];\n> \t};\n> \n> No flex array here, right? But wait, there is a `struct fsentry`. Let's\n> look at\n> [that](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/fscache.c#L43-L74):\n> \n> \tstruct fsentry {\n> \t\tstruct hashmap_entry ent;\n> \t\t[...]\n> \t\t/*\n> \t\t * Name of the entry. For directory listings: relative path of the\n> \t\t * directory, without trailing '/' (empty for cwd()). For file\n> \t\t * entries:\n> \t\t * name of the file. Typically points to the end of the structure\n> \t\t * if\n> \t\t * the fsentry is allocated on the heap (see fsentry_alloc), or to\n> \t\t * a\n> \t\t * local variable if on the stack (see fsentry_init).\n> \t\t */\n> \t\tstruct dirent dirent;\n> \t};\n> \n> Still no flex array, right? But wait, there is a `struct dirent`. Let's\n> [see](https://github.com/git-for-windows/git/blob/v2.34.1.windows.1/compat/win32/dirent.h#L9-L12):\n> \n> \tstruct dirent {\n> \t\tunsigned char d_type; /* file type to prevent lstat after readdir */\n> \t\tchar d_name[FLEX_ARRAY]; /* file name */\n> \t};\n> \n> Finally! We see the flex array.\n> \n> Now, you may ask why is this even correct? How can you have an empty-sized\n> field in a struct that is inside another struct that is inside yet another\n> struct _and then followed by another field_?\n> \n> The reason why this is correct and intended is that `struct dirent`\n> intentionally leaves the length of the `d_name` undefined, to leave it to\n> the implementation whether a fixed-size buffer is used or a\n> specifically-allocated one of the exact correct size for a _specific_\n> directory entry.\n> \n> In FSCache, we want to allocate a large-enough buffer to fit _any_ file\n> name, and it should not only contain the metadata in `struct dirent`, but\n> additionally some FSCache-specific metadata.\n> \n> Therefore, `struct fsentry` is kind of a subclass of `struct dirent`, and\n> `struct heap_fsentry` is kind of a subclass of something that does not\n> exist, a `struct dirent` that offers enough space to fit _any_ legal\n> `d_name` (that is what that `dummy` field is for, it is not actually\n> intended to be accessed except via `d_name`).\n> \n>> If it does start using it, there issue may actually be elsewhere besides\n>> the struct definition (it could be just a badly targeted compiler error).\n>> We have code like `struct heap_fsentry key[2];`.  That declaration can't\n>> work with a flex array.\n> \n> I hope my explanation above made sense to you.\n> \n> Admittedly, it is slightly icky code, but honestly, I do not have any\n> splendid idea how to make it less complicated to understand. Do you?\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"443697","messageId":"xmqqfsr1pjdo.fsf@gitster.g","threadId":"57041","inReplyTo":"f245f2b7-ca00-5497-3d1d-658c4c0fbcb6@gmail.com","subject":"Re: [PATCH] git-compat-util(msvc): C11 does not imply support for zero-sized arrays","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T21:18:27Z","receivedAt":"2021-12-09T21:18:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> the end of a struct isn't triggering a compile error. But having a field\n>> _after_ that empty flex array _does_ trigger a compile error:\n>> struct MyStruct {\n>>      int x;\n>>      int flexA[];\n>>      char string[256];\n>> };\n>> Note the added `string` field.\n\nIck.\n\nIf we use the \"safer but a bit wasteful\" alternative in such a case,\nit will become just wrong.  We'll have a single-member array there,\nand extra allocation for an instance of MyStruct will only lengthen\nthe .string member, without allowing the .flexA member to have more\nelements.\n\n> Having a field after the flex array is a violation of the C\n> standard. Section 6.7.2.1:\n>    ... the last member of a structure with more than one named member\n>    may have incomplete array type, such a structure (and any union\n>    containing, possibly recursively, a member that is such a structure)\n>    shall not be a member of a structure or an element of an array.\n>\n> Note that the wording also forbids\n>\n> struct A {\n> \tint x;\n> \tchar flex[];\n> };\n>\n> struct B {\n> \tstruct A a; /* This is forbidden */\n> };\n>\n> There was a proposal a few years ago to relax that restriction [1] but\n> it does not seem to be in the latest draft standard.\n>\n> None of this helps fix the problem, but it does explain why MSVC complains.\n\nThanks.\n\nIn short, the code is wrong, and the compiler is complaining to\navoid us using a wrong code.  If the same code is given to other\ncompilers that support ANCI C style flexible array member, we are\nlikely to see the same error.\n\nWhich makes the whole thing (including my \"let's make the\nconditionals to set up FLEX_ARRAY for various compilers easier to\nread\") much less urgent.\n\nThanks for a well-written summary.\n\n"}]}