{"thread":{"id":"48088","subject":"[PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning","startedAt":"2018-03-19T17:56:22Z","lastAt":"2018-03-21T05:59:50Z","messageCount":4,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"342219","messageId":"6d434e76-cfa0-2a6e-f163-b54316a24cee@ramsayjones.plus.com","threadId":"48088","inReplyTo":null,"subject":"[PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-19T17:56:11Z","receivedAt":"2018-03-19T17:56:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe function ce_write_entry() uses a 'self-initialised' variable\nconstruct, for the symbol 'saved_namelen', to suppress a gcc\n'-Wmaybe-uninitialized' warning, given that the warning is a false\npositive.\n\nFor the purposes of this discussion, the ce_write_entry() function has\nthree code blocks of interest, that look like so:\n\n        /* block #1 */\n        if (ce->ce_flags & CE_STRIP_NAME) {\n                saved_namelen = ce_namelen(ce);\n                ce->ce_namelen = 0;\n        }\n\n        /* block #2 */\n        /*\n\t * several code blocks that contain, among others, calls\n         * to copy_cache_entry_to_ondisk(ondisk, ce);\n         */\n\n        /* block #3 */\n        if (ce->ce_flags & CE_STRIP_NAME) {\n                ce->ce_namelen = saved_namelen;\n                ce->ce_flags &= ~CE_STRIP_NAME;\n        }\n\nThe warning implies that gcc thinks it is possible that the first\nblock is not entered, the calls to copy_cache_entry_to_ondisk()\ncould toggle the CE_STRIP_NAME flag on, thereby entering block #3\nwith saved_namelen unset. However, the copy_cache_entry_to_ondisk()\nfunction does not write to ce->ce_flags (it only reads). gcc could\neasily determine this, since that function is local to this file,\nbut it obviously doesn't.\n\nIn order to suppress this warning, we make it clear to the reader\n(human and compiler), that block #3 will only be entered when the\nfirst block has been entered, by introducing a new 'stripped_name'\nboolean variable. We also take the opportunity to change the type\nof 'saved_namelen' to 'unsigned int' to match ce->ce_namelen.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n read-cache.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b..49607ddcd 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \t\t\t  struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n {\n \tint size;\n-\tint saved_namelen = saved_namelen; /* compiler workaround */\n \tint result;\n+\tunsigned int saved_namelen;\n+\tint stripped_name = 0;\n \tstatic unsigned char padding[8] = { 0x00 };\n \n \tif (ce->ce_flags & CE_STRIP_NAME) {\n \t\tsaved_namelen = ce_namelen(ce);\n \t\tce->ce_namelen = 0;\n+\t\tstripped_name = 1;\n \t}\n \n \tif (ce->ce_flags & CE_EXTENDED)\n@@ -2150,7 +2152,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \t\tstrbuf_splice(previous_name, common, to_remove,\n \t\t\t      ce->name + common, ce_namelen(ce) - common);\n \t}\n-\tif (ce->ce_flags & CE_STRIP_NAME) {\n+\tif (stripped_name) {\n \t\tce->ce_namelen = saved_namelen;\n \t\tce->ce_flags &= ~CE_STRIP_NAME;\n \t}\n-- \n2.16.0\n"},{"id":"342286","messageId":"20180320043652.GC13302@sigill.intra.peff.net","threadId":"48088","inReplyTo":"6d434e76-cfa0-2a6e-f163-b54316a24cee@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-20T04:36:52Z","receivedAt":"2018-03-20T04:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 19, 2018 at 05:56:11PM +0000, Ramsay Jones wrote:\n\n> For the purposes of this discussion, the ce_write_entry() function has\n> three code blocks of interest, that look like so:\n> \n>         /* block #1 */\n>         if (ce->ce_flags & CE_STRIP_NAME) {\n>                 saved_namelen = ce_namelen(ce);\n>                 ce->ce_namelen = 0;\n>         }\n> \n>         /* block #2 */\n>         /*\n> \t * several code blocks that contain, among others, calls\n>          * to copy_cache_entry_to_ondisk(ondisk, ce);\n>          */\n> \n>         /* block #3 */\n>         if (ce->ce_flags & CE_STRIP_NAME) {\n>                 ce->ce_namelen = saved_namelen;\n>                 ce->ce_flags &= ~CE_STRIP_NAME;\n>         }\n> \n> The warning implies that gcc thinks it is possible that the first\n> block is not entered, the calls to copy_cache_entry_to_ondisk()\n> could toggle the CE_STRIP_NAME flag on, thereby entering block #3\n> with saved_namelen unset. However, the copy_cache_entry_to_ondisk()\n> function does not write to ce->ce_flags (it only reads). gcc could\n> easily determine this, since that function is local to this file,\n> but it obviously doesn't.\n\nWeird. It seems like it would be pretty easy for it to know that we\ndon't write the flags field at all. But I also don't see any other thing\nthat would fool the compiler.\n\n> In order to suppress this warning, we make it clear to the reader\n> (human and compiler), that block #3 will only be entered when the\n> first block has been entered, by introducing a new 'stripped_name'\n> boolean variable. We also take the opportunity to change the type\n> of 'saved_namelen' to 'unsigned int' to match ce->ce_namelen.\n\nThese probably both ought to be size_t, but it makes sense to match\nce_namelen for now.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 2eb81a66b..49607ddcd 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n>  \t\t\t  struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n>  {\n>  \tint size;\n> -\tint saved_namelen = saved_namelen; /* compiler workaround */\n>  \tint result;\n> +\tunsigned int saved_namelen;\n> +\tint stripped_name = 0;\n\nMaybe too clever, but I think you could just do:\n\n  unsigned int saved_namelen = 0;\n  ...\n\tsaved_namelen = ce_namelen(ce);\n  ...\n  if (saved_namelen)\n\tce->ce_namelen = saved_namelen;\n  ce->ce_flags &= ~CE_STRIP_NAME;\n\nthe zero-length name case (if that's even legal) would work out the\nsame.\n\nThat probably falls under the category of bikeshedding, though.\n\n-Peff\n"},{"id":"342373","messageId":"e3894650-3639-c871-1291-f3efa7fbea0a@ramsayjones.plus.com","threadId":"48088","inReplyTo":"20180320043652.GC13302@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-20T22:52:16Z","receivedAt":"2018-03-20T22:52:23Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/03/18 04:36, Jeff King wrote:\n> On Mon, Mar 19, 2018 at 05:56:11PM +0000, Ramsay Jones wrote:\n> \n[snip]\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 2eb81a66b..49607ddcd 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n>>  \t\t\t  struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n>>  {\n>>  \tint size;\n>> -\tint saved_namelen = saved_namelen; /* compiler workaround */\n>>  \tint result;\n>> +\tunsigned int saved_namelen;\n>> +\tint stripped_name = 0;\n> \n> Maybe too clever, but I think you could just do:\n> \n>   unsigned int saved_namelen = 0;\n>   ...\n> \tsaved_namelen = ce_namelen(ce);\n>   ...\n>   if (saved_namelen)\n> \tce->ce_namelen = saved_namelen;\n>   ce->ce_flags &= ~CE_STRIP_NAME;\n> \n> the zero-length name case (if that's even legal) would work out the\n> same.\n\nYeah, that was one option that I looked at. The first option\nwas to initialise saved_namelen to -1 (it was still an int) then\nthe test became if (saved_namelen >= 0). However, that started\nme thinking about the zero-length case - should I assert if\n((ce->ce_flags & CE_STRIP_NAME) && (ce_namelen(ce) == 0))? etc.\n\nIn the end, I decided that I wanted it to be 'drop dead' obvious\nwhat was going on! Hopefully, the result was just that. :-D\n\nATB,\nRamsay Jones\n\n\n"},{"id":"342385","messageId":"20180321055944.GA15719@sigill.intra.peff.net","threadId":"48088","inReplyTo":"e3894650-3639-c871-1291-f3efa7fbea0a@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:59:44Z","receivedAt":"2018-03-21T05:59:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 20, 2018 at 10:52:16PM +0000, Ramsay Jones wrote:\n\n> > Maybe too clever, but I think you could just do:\n> > \n> >   unsigned int saved_namelen = 0;\n> >   ...\n> > \tsaved_namelen = ce_namelen(ce);\n> >   ...\n> >   if (saved_namelen)\n> > \tce->ce_namelen = saved_namelen;\n> >   ce->ce_flags &= ~CE_STRIP_NAME;\n> > \n> > the zero-length name case (if that's even legal) would work out the\n> > same.\n> \n> Yeah, that was one option that I looked at. The first option\n> was to initialise saved_namelen to -1 (it was still an int) then\n> the test became if (saved_namelen >= 0). However, that started\n> me thinking about the zero-length case - should I assert if\n> ((ce->ce_flags & CE_STRIP_NAME) && (ce_namelen(ce) == 0))? etc.\n> \n> In the end, I decided that I wanted it to be 'drop dead' obvious\n> what was going on! Hopefully, the result was just that. :-D\n\nYeah, thinking on it more, simple and stupid is the right thing to do\nhere. Thanks for a dose of sanity. :)\n\n-Peff\n"}]}