git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning
@ 2018-03-19 17:56 Ramsay Jones
  2018-03-20  4:36 ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Ramsay Jones @ 2018-03-19 17:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, GIT Mailing-list


The function ce_write_entry() uses a 'self-initialised' variable
construct, for the symbol 'saved_namelen', to suppress a gcc
'-Wmaybe-uninitialized' warning, given that the warning is a false
positive.

For the purposes of this discussion, the ce_write_entry() function has
three code blocks of interest, that look like so:

        /* block #1 */
        if (ce->ce_flags & CE_STRIP_NAME) {
                saved_namelen = ce_namelen(ce);
                ce->ce_namelen = 0;
        }

        /* block #2 */
        /*
	 * several code blocks that contain, among others, calls
         * to copy_cache_entry_to_ondisk(ondisk, ce);
         */

        /* block #3 */
        if (ce->ce_flags & CE_STRIP_NAME) {
                ce->ce_namelen = saved_namelen;
                ce->ce_flags &= ~CE_STRIP_NAME;
        }

The warning implies that gcc thinks it is possible that the first
block is not entered, the calls to copy_cache_entry_to_ondisk()
could toggle the CE_STRIP_NAME flag on, thereby entering block #3
with saved_namelen unset. However, the copy_cache_entry_to_ondisk()
function does not write to ce->ce_flags (it only reads). gcc could
easily determine this, since that function is local to this file,
but it obviously doesn't.

In order to suppress this warning, we make it clear to the reader
(human and compiler), that block #3 will only be entered when the
first block has been entered, by introducing a new 'stripped_name'
boolean variable. We also take the opportunity to change the type
of 'saved_namelen' to 'unsigned int' to match ce->ce_namelen.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 read-cache.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b..49607ddcd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
 			  struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)
 {
 	int size;
-	int saved_namelen = saved_namelen; /* compiler workaround */
 	int result;
+	unsigned int saved_namelen;
+	int stripped_name = 0;
 	static unsigned char padding[8] = { 0x00 };
 
 	if (ce->ce_flags & CE_STRIP_NAME) {
 		saved_namelen = ce_namelen(ce);
 		ce->ce_namelen = 0;
+		stripped_name = 1;
 	}
 
 	if (ce->ce_flags & CE_EXTENDED)
@@ -2150,7 +2152,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
 		strbuf_splice(previous_name, common, to_remove,
 			      ce->name + common, ce_namelen(ce) - common);
 	}
-	if (ce->ce_flags & CE_STRIP_NAME) {
+	if (stripped_name) {
 		ce->ce_namelen = saved_namelen;
 		ce->ce_flags &= ~CE_STRIP_NAME;
 	}
-- 
2.16.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning
  2018-03-19 17:56 [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning Ramsay Jones
@ 2018-03-20  4:36 ` Jeff King
  2018-03-20 22:52   ` Ramsay Jones
  0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2018-03-20  4:36 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Junio C Hamano, Johannes Schindelin, GIT Mailing-list

On Mon, Mar 19, 2018 at 05:56:11PM +0000, Ramsay Jones wrote:

> For the purposes of this discussion, the ce_write_entry() function has
> three code blocks of interest, that look like so:
> 
>         /* block #1 */
>         if (ce->ce_flags & CE_STRIP_NAME) {
>                 saved_namelen = ce_namelen(ce);
>                 ce->ce_namelen = 0;
>         }
> 
>         /* block #2 */
>         /*
> 	 * several code blocks that contain, among others, calls
>          * to copy_cache_entry_to_ondisk(ondisk, ce);
>          */
> 
>         /* block #3 */
>         if (ce->ce_flags & CE_STRIP_NAME) {
>                 ce->ce_namelen = saved_namelen;
>                 ce->ce_flags &= ~CE_STRIP_NAME;
>         }
> 
> The warning implies that gcc thinks it is possible that the first
> block is not entered, the calls to copy_cache_entry_to_ondisk()
> could toggle the CE_STRIP_NAME flag on, thereby entering block #3
> with saved_namelen unset. However, the copy_cache_entry_to_ondisk()
> function does not write to ce->ce_flags (it only reads). gcc could
> easily determine this, since that function is local to this file,
> but it obviously doesn't.

Weird. It seems like it would be pretty easy for it to know that we
don't write the flags field at all. But I also don't see any other thing
that would fool the compiler.

> In order to suppress this warning, we make it clear to the reader
> (human and compiler), that block #3 will only be entered when the
> first block has been entered, by introducing a new 'stripped_name'
> boolean variable. We also take the opportunity to change the type
> of 'saved_namelen' to 'unsigned int' to match ce->ce_namelen.

These probably both ought to be size_t, but it makes sense to match
ce_namelen for now.

> diff --git a/read-cache.c b/read-cache.c
> index 2eb81a66b..49607ddcd 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
>  			  struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)
>  {
>  	int size;
> -	int saved_namelen = saved_namelen; /* compiler workaround */
>  	int result;
> +	unsigned int saved_namelen;
> +	int stripped_name = 0;

Maybe too clever, but I think you could just do:

  unsigned int saved_namelen = 0;
  ...
	saved_namelen = ce_namelen(ce);
  ...
  if (saved_namelen)
	ce->ce_namelen = saved_namelen;
  ce->ce_flags &= ~CE_STRIP_NAME;

the zero-length name case (if that's even legal) would work out the
same.

That probably falls under the category of bikeshedding, though.

-Peff

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning
  2018-03-20  4:36 ` Jeff King
@ 2018-03-20 22:52   ` Ramsay Jones
  2018-03-21  5:59     ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Ramsay Jones @ 2018-03-20 22:52 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, GIT Mailing-list



On 20/03/18 04:36, Jeff King wrote:
> On Mon, Mar 19, 2018 at 05:56:11PM +0000, Ramsay Jones wrote:
> 
[snip]
>> diff --git a/read-cache.c b/read-cache.c
>> index 2eb81a66b..49607ddcd 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
>>  			  struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)
>>  {
>>  	int size;
>> -	int saved_namelen = saved_namelen; /* compiler workaround */
>>  	int result;
>> +	unsigned int saved_namelen;
>> +	int stripped_name = 0;
> 
> Maybe too clever, but I think you could just do:
> 
>   unsigned int saved_namelen = 0;
>   ...
> 	saved_namelen = ce_namelen(ce);
>   ...
>   if (saved_namelen)
> 	ce->ce_namelen = saved_namelen;
>   ce->ce_flags &= ~CE_STRIP_NAME;
> 
> the zero-length name case (if that's even legal) would work out the
> same.

Yeah, that was one option that I looked at. The first option
was to initialise saved_namelen to -1 (it was still an int) then
the test became if (saved_namelen >= 0). However, that started
me thinking about the zero-length case - should I assert if
((ce->ce_flags & CE_STRIP_NAME) && (ce_namelen(ce) == 0))? etc.

In the end, I decided that I wanted it to be 'drop dead' obvious
what was going on! Hopefully, the result was just that. :-D

ATB,
Ramsay Jones



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning
  2018-03-20 22:52   ` Ramsay Jones
@ 2018-03-21  5:59     ` Jeff King
  0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2018-03-21  5:59 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Junio C Hamano, Johannes Schindelin, GIT Mailing-list

On Tue, Mar 20, 2018 at 10:52:16PM +0000, Ramsay Jones wrote:

> > Maybe too clever, but I think you could just do:
> > 
> >   unsigned int saved_namelen = 0;
> >   ...
> > 	saved_namelen = ce_namelen(ce);
> >   ...
> >   if (saved_namelen)
> > 	ce->ce_namelen = saved_namelen;
> >   ce->ce_flags &= ~CE_STRIP_NAME;
> > 
> > the zero-length name case (if that's even legal) would work out the
> > same.
> 
> Yeah, that was one option that I looked at. The first option
> was to initialise saved_namelen to -1 (it was still an int) then
> the test became if (saved_namelen >= 0). However, that started
> me thinking about the zero-length case - should I assert if
> ((ce->ce_flags & CE_STRIP_NAME) && (ce_namelen(ce) == 0))? etc.
> 
> In the end, I decided that I wanted it to be 'drop dead' obvious
> what was going on! Hopefully, the result was just that. :-D

Yeah, thinking on it more, simple and stupid is the right thing to do
here. Thanks for a dose of sanity. :)

-Peff

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-03-21  5:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-19 17:56 [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning Ramsay Jones
2018-03-20  4:36 ` Jeff King
2018-03-20 22:52   ` Ramsay Jones
2018-03-21  5:59     ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git