All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix some sparse warnings
@ 2013-07-15 17:31 Ramsay Jones
  2013-07-16  5:57 ` Johannes Sixt
  0 siblings, 1 reply; 12+ messages in thread
From: Ramsay Jones @ 2013-07-15 17:31 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, GIT Mailing-list


Sparse issues three "Using plain integer as NULL pointer" warnings.
Each warning relates to the use of an '{0}' initialiser expression
in the declaration of an 'struct object_info'. The first field of
this structure has pointer type. Thus, in order to suppress these
warnings, we replace the initialiser expression with '{NULL}'.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Jeff,

If you need to re-roll the patches in your 'jk/in-pack-size-measurement'
branch, could you please squash this (or something like it) into the
patches equivalent to commit 7c07385d ("zero-initialize object_info structs",
07-07-2013) [sha1_file.c and streaming.c] and commit 778d263a ("cat-file: add
--batch-disk-sizes option", 07-07-2013) [builtin/cat-file.c].

Thanks!

ATB,
Ramsay Jones


 builtin/cat-file.c | 2 +-
 sha1_file.c        | 2 +-
 streaming.c        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bf12883..860576e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -135,7 +135,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
 	if (print_contents == BATCH)
 		contents = read_sha1_file(sha1, &type, &size);
 	else if (print_contents == BATCH_DISK_SIZES) {
-		struct object_info oi = {0};
+		struct object_info oi = {NULL};
 		oi.disk_sizep = &size;
 		type = sha1_object_info_extended(sha1, &oi);
 	}
diff --git a/sha1_file.c b/sha1_file.c
index 4c2365f..e4ab0a4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2440,7 +2440,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 
 int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 {
-	struct object_info oi = {0};
+	struct object_info oi = {NULL};
 
 	oi.sizep = sizep;
 	return sha1_object_info_extended(sha1, &oi);
diff --git a/streaming.c b/streaming.c
index cac282f..5710065 100644
--- a/streaming.c
+++ b/streaming.c
@@ -135,7 +135,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
 				 struct stream_filter *filter)
 {
 	struct git_istream *st;
-	struct object_info oi = {0};
+	struct object_info oi = {NULL};
 	const unsigned char *real = lookup_replace_object(sha1);
 	enum input_source src = istream_source(real, type, &oi);
 
-- 
1.8.3

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-15 17:31 [PATCH] Fix some sparse warnings Ramsay Jones
@ 2013-07-16  5:57 ` Johannes Sixt
  2013-07-16  6:21   ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Johannes Sixt @ 2013-07-16  5:57 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Jeff King, Junio C Hamano, GIT Mailing-list

Am 7/15/2013 19:31, schrieb Ramsay Jones:
> Sparse issues three "Using plain integer as NULL pointer" warnings.
> Each warning relates to the use of an '{0}' initialiser expression
> in the declaration of an 'struct object_info'.

I question the value of this warning. Initialization with '= {0}' is a
well-established idiom, and sparse should know about it. Also, plain 0
*is* a null pointer constant.

-- Hannes

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16  5:57 ` Johannes Sixt
@ 2013-07-16  6:21   ` Jeff King
  2013-07-16 20:53     ` Philip Oakley
  2013-07-18 17:58     ` Ramsay Jones
  0 siblings, 2 replies; 12+ messages in thread
From: Jeff King @ 2013-07-16  6:21 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Ramsay Jones, Junio C Hamano, GIT Mailing-list

On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:

> Am 7/15/2013 19:31, schrieb Ramsay Jones:
> > Sparse issues three "Using plain integer as NULL pointer" warnings.
> > Each warning relates to the use of an '{0}' initialiser expression
> > in the declaration of an 'struct object_info'.
> 
> I question the value of this warning. Initialization with '= {0}' is a
> well-established idiom, and sparse should know about it. Also, plain 0
> *is* a null pointer constant.

I agree with you. It's not a bug, and I think sparse is being overly
picky here; it is missing the forest for the trees in interpreting the
idiom.

Still, it may be worth tweaking in the name of eliminating compiler
noise, since it does not cost us very much to do so (and I believe we
have done so in the past, too).

We could also ask people with sparse to turn off the "use NULL instead
of 0" warning, but I think it _is_ a useful warning elsewhere (even
though it is never a bug, it violates our style guidelines and may be an
indication of a bug). It would be nice if sparse learned to ignore the
warning in this particular idiom, but I am not going to hold my breath
for that.

-Peff

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16  6:21   ` Jeff King
@ 2013-07-16 20:53     ` Philip Oakley
  2013-07-16 21:18       ` Stefan Beller
  2013-07-17 22:08       ` Stefan Beller
  2013-07-18 17:58     ` Ramsay Jones
  1 sibling, 2 replies; 12+ messages in thread
From: Philip Oakley @ 2013-07-16 20:53 UTC (permalink / raw)
  To: Jeff King, Johannes Sixt; +Cc: Ramsay Jones, Junio C Hamano, GIT Mailing-list

From: "Jeff King" <peff@peff.net>
Sent: Tuesday, July 16, 2013 7:21 AM
> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
>
>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>> > Sparse issues three "Using plain integer as NULL pointer" warnings.
>> > Each warning relates to the use of an '{0}' initialiser expression
>> > in the declaration of an 'struct object_info'.
>>
>> I question the value of this warning. Initialization with '= {0}' is
>> a
>> well-established idiom, and sparse should know about it. Also, plain
>> 0
>> *is* a null pointer constant.
>
> I agree with you. It's not a bug, and I think sparse is being overly
> picky here; it is missing the forest for the trees in interpreting the
> idiom.
>
> Still, it may be worth tweaking in the name of eliminating compiler
> noise, since it does not cost us very much to do so (and I believe we
> have done so in the past, too).
>
> We could also ask people with sparse to turn off the "use NULL instead
> of 0" warning, but I think it _is_ a useful warning elsewhere (even
> though it is never a bug, it violates our style guidelines and may be
> an
> indication of a bug). It would be nice if sparse learned to ignore the
> warning in this particular idiom, but I am not going to hold my breath
> for that.
>
> -Peff
> --

On the subject of warnings and null pointers, yesterday's Code Project
news linked to a blog on the problems of unexpected optimization bugs,
such as dereferencing a null pointer. "Finding Undefined Behavior Bugs
by Finding Dead Code" http://blog.regehr.org/archives/970 which links to 
the draft of an interesting paper 
[http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]

Does anyone run the "new static checker called 'Stack' that precisely
identifies unstable code"? [though the paper's conclusion says 'All
Stack source code will be publicly available.' which suggests it's not
yet available]

Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog
comments (see also index : kernel/git/torvalds/linux.git "Add
'-fno-delete-null-pointer-checks' to gcc CFLAGS"
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf

You're probably already aware of these aspects but I thought it worth
mentioning for the wider readership.

regards

Philip

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16 20:53     ` Philip Oakley
@ 2013-07-16 21:18       ` Stefan Beller
  2013-07-16 22:18         ` Philip Oakley
  2013-07-17  5:47         ` Johannes Sixt
  2013-07-17 22:08       ` Stefan Beller
  1 sibling, 2 replies; 12+ messages in thread
From: Stefan Beller @ 2013-07-16 21:18 UTC (permalink / raw)
  To: Philip Oakley
  Cc: Jeff King, Johannes Sixt, Ramsay Jones, Junio C Hamano, GIT Mailing-list

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On 07/16/2013 10:53 PM, Philip Oakley wrote:
> From: "Jeff King" <peff@peff.net>
> Sent: Tuesday, July 16, 2013 7:21 AM
>> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
>>
>>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>>> > Sparse issues three "Using plain integer as NULL pointer" warnings.
>>> > Each warning relates to the use of an '{0}' initialiser expression
>>> > in the declaration of an 'struct object_info'.
>>>
>>> I question the value of this warning. Initialization with '= {0}' is
>>> a
>>> well-established idiom, and sparse should know about it. Also, plain
>>> 0
>>> *is* a null pointer constant.
>>
>> I agree with you. It's not a bug, and I think sparse is being overly
>> picky here; it is missing the forest for the trees in interpreting the
>> idiom.
>>
>> Still, it may be worth tweaking in the name of eliminating compiler
>> noise, since it does not cost us very much to do so (and I believe we
>> have done so in the past, too).
>>
>> We could also ask people with sparse to turn off the "use NULL instead
>> of 0" warning, but I think it _is_ a useful warning elsewhere (even
>> though it is never a bug, it violates our style guidelines and may be
>> an
>> indication of a bug). It would be nice if sparse learned to ignore the
>> warning in this particular idiom, but I am not going to hold my breath
>> for that.
>>
>> -Peff
>> -- 
> 
> On the subject of warnings and null pointers, yesterday's Code Project
> news linked to a blog on the problems of unexpected optimization bugs,
> such as dereferencing a null pointer. "Finding Undefined Behavior Bugs
> by Finding Dead Code" http://blog.regehr.org/archives/970 which links to
> the draft of an interesting paper
> [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]
> 
> Does anyone run the "new static checker called 'Stack' that precisely
> identifies unstable code"? [though the paper's conclusion says 'All
> Stack source code will be publicly available.' which suggests it's not
> yet available]
> 
> Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog
> comments (see also index : kernel/git/torvalds/linux.git "Add
> '-fno-delete-null-pointer-checks' to gcc CFLAGS"
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf
> 
> 
> You're probably already aware of these aspects but I thought it worth
> mentioning for the wider readership.
> 
> regards
> 
> Philip
> 
> 

I recently started contributing and I used cppcheck found at
https://github.com/danmar/cppcheck to submit the patches
at origin/sb/misc-fixes
As it is originally for C++, that tool throws lots of
false-positives (i.e. warns about null pointer dereferencing
when it's not possible to be a null pointer) unfortunately.

Also I hear llvm/clang comes with a good static code analyzer,
which I tried today on a different project.
Though I could not figure out how to use that on a pure C project
such as git, as that tool seems to require a C++ compilation for
doing its static code analysis.

The blog post you linked to seems very interesting and promising. :)

Regards,
Stefan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16 21:18       ` Stefan Beller
@ 2013-07-16 22:18         ` Philip Oakley
  2013-07-17  5:47         ` Johannes Sixt
  1 sibling, 0 replies; 12+ messages in thread
From: Philip Oakley @ 2013-07-16 22:18 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Jeff King, Johannes Sixt, Ramsay Jones, Junio C Hamano, GIT Mailing-list

On 16/07/13 22:18, Stefan Beller wrote:
> On 07/16/2013 10:53 PM, Philip Oakley wrote:
>> From: "Jeff King" <peff@peff.net>
>> Sent: Tuesday, July 16, 2013 7:21 AM
>>> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
>>>
>>>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>>>>> Sparse issues three "Using plain integer as NULL pointer" warnings.
>>>>> Each warning relates to the use of an '{0}' initialiser expression
>>>>> in the declaration of an 'struct object_info'.
>>>>
>>>> I question the value of this warning. Initialization with '= {0}' is
>>>> a
>>>> well-established idiom, and sparse should know about it. Also, plain
>>>> 0
>>>> *is* a null pointer constant.
>>>
>>> I agree with you. It's not a bug, and I think sparse is being overly
>>> picky here; it is missing the forest for the trees in interpreting the
>>> idiom.
>>>
>>> Still, it may be worth tweaking in the name of eliminating compiler
>>> noise, since it does not cost us very much to do so (and I believe we
>>> have done so in the past, too).
>>>
>>> We could also ask people with sparse to turn off the "use NULL instead
>>> of 0" warning, but I think it _is_ a useful warning elsewhere (even
>>> though it is never a bug, it violates our style guidelines and may be
>>> an
>>> indication of a bug). It would be nice if sparse learned to ignore the
>>> warning in this particular idiom, but I am not going to hold my breath
>>> for that.
>>>
>>> -Peff
>>> --
>>
>> On the subject of warnings and null pointers, yesterday's Code Project
>> news linked to a blog on the problems of unexpected optimization bugs,
>> such as dereferencing a null pointer. "Finding Undefined Behavior Bugs
>> by Finding Dead Code" http://blog.regehr.org/archives/970 which links to
>> the draft of an interesting paper
>> [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]
>>
>> Does anyone run the "new static checker called 'Stack' that precisely
>> identifies unstable code"? [though the paper's conclusion says 'All
>> Stack source code will be publicly available.' which suggests it's not
>> yet available]
>>
>> Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog
>> comments (see also index : kernel/git/torvalds/linux.git "Add
>> '-fno-delete-null-pointer-checks' to gcc CFLAGS"
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf
>>
>>
>> You're probably already aware of these aspects but I thought it worth
>> mentioning for the wider readership.
>>
>> regards
>>
>> Philip
>>
>>
>
> I recently started contributing and I used cppcheck found at
> https://github.com/danmar/cppcheck to submit the patches
> at origin/sb/misc-fixes
> As it is originally for C++, that tool throws lots of
> false-positives (i.e. warns about null pointer dereferencing
> when it's not possible to be a null pointer) unfortunately.
>
> Also I hear llvm/clang comes with a good static code analyzer,
> which I tried today on a different project.
> Though I could not figure out how to use that on a pure C project
> such as git, as that tool seems to require a C++ compilation for
> doing its static code analysis.
>
> The blog post you linked to seems very interesting and promising. :)
>
> Regards,
> Stefan
>
A bit more searching also finds 
http://pdos.csail.mit.edu/~xi/papers/ub-apsys12.pdf and 
http://pdos.csail.mit.edu/~xi/papers/ub-apsys12-slides.pptx by the same 
author.

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16 21:18       ` Stefan Beller
  2013-07-16 22:18         ` Philip Oakley
@ 2013-07-17  5:47         ` Johannes Sixt
  1 sibling, 0 replies; 12+ messages in thread
From: Johannes Sixt @ 2013-07-17  5:47 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Philip Oakley, Jeff King, Ramsay Jones, Junio C Hamano, GIT Mailing-list

>>>> I question the value of this warning. Initialization with '=
>>>> {0}' is a well-established idiom, and sparse should know about
>>>> it.

Thanks everyone for your feedback. But I really wanted to call only the
warning in the case of the '= {0}' idiom into question, not about 0 vs.
NULL in general.

-- Hannes

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16 20:53     ` Philip Oakley
  2013-07-16 21:18       ` Stefan Beller
@ 2013-07-17 22:08       ` Stefan Beller
  2013-07-17 22:09         ` [PATCH] parse_object_buffer: Correct freeing the buffer Stefan Beller
                           ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Stefan Beller @ 2013-07-17 22:08 UTC (permalink / raw)
  To: Philip Oakley
  Cc: Jeff King, Johannes Sixt, Ramsay Jones, Junio C Hamano, GIT Mailing-list

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

On 07/16/2013 10:53 PM, Philip Oakley wrote:
> 
> Does anyone run the "new static checker called 'Stack' that precisely
> identifies unstable code"? [though the paper's conclusion says 'All
> Stack source code will be publicly available.' which suggests it's not
> yet available]
> 

So I started using the clang code analyzer on git. One of the 
first warnings actually is this:

object.c:241:7: warning: Branch condition evaluates to a garbage value
                if (!eaten)

So that part of object.c lookx like this:
	struct object *parse_object(const unsigned char *sha1) 
	{
		int eaten;
		...
		obj = parse_object_buffer(sha1, type, size, buffer, &eaten);
		if (!eaten)
			free(buffer);
	}

And the parse_object_buffer looks like this with respect to the eaten 
variable:
	struct object *parse_object_buffer(...)
	{
		int eaten = 0;
		if (something)
			return NULL;
		...
		if (something_different)
			eaten=1;
		*eaten_p = eaten;
	}

So what might happen is, that parse_object_buffer exits early, without
executing 
	
	*eaten_p = eaten;

Then in the parse_object function eaten was never initialized nor set 
inside the call to parse_object_buffer. Then it is obvious that the
free(buffer) is executed depending on garbage left on the stack.
Definitely something what we want to change.

The obvious way to repair this would be to just initialize the eaten variable
inside parse_object.
	struct object *parse_object(const unsigned char *sha1) 
	{
		int eaten=0;
		...

However I'd like to propose another solution:
In parse_object_buffer we do not have a local eaten variable, but
directly write to *eaten_p. That would be the following patch.

Was there a particular idea or goal behind first having a local eaten
variable, which later near the correct return of the function was used to set the 
eaten_p?

Thanks,
Stefan




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

* [PATCH] parse_object_buffer: Correct freeing the buffer.
  2013-07-17 22:08       ` Stefan Beller
@ 2013-07-17 22:09         ` Stefan Beller
  2013-07-17 22:16         ` [PATCH] Fix some sparse warnings Stefan Beller
  2013-07-17 23:22         ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Stefan Beller @ 2013-07-17 22:09 UTC (permalink / raw)
  To: philipoakley, peff, j.sixt, ramsay, gitster, git; +Cc: Stefan Beller

If we exit early in the function parse_object_buffer, we did not
write to *eaten_p. Then the calling function parse_object, which looks
like the following with respect to the eaten variable, cannot rely on a
proper value set in eaten, hence the freeing of the buffer depends
on random values in memory.

	struct object *parse_object(const unsigned char *sha1)
	{
		int eaten;
		...
		obj = parse_object_buffer(sha1, type, size, buffer, &eaten);
		if (!eaten)
			free(buffer);
	}

This change makes sure, the buffer freeing condition is deterministic.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 object.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/object.c b/object.c
index cbc7333..d8a4b1f 100644
--- a/object.c
+++ b/object.c
@@ -145,7 +145,7 @@ struct object *lookup_unknown_object(const unsigned char *sha1)
 struct object *parse_object_buffer(const unsigned char *sha1, enum object_type type, unsigned long size, void *buffer, int *eaten_p)
 {
 	struct object *obj;
-	int eaten = 0;
+	*eaten_p = 0;
 
 	obj = NULL;
 	if (type == OBJ_BLOB) {
@@ -164,7 +164,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 			if (!tree->object.parsed) {
 				if (parse_tree_buffer(tree, buffer, size))
 					return NULL;
-				eaten = 1;
+				*eaten_p = 1;
 			}
 		}
 	} else if (type == OBJ_COMMIT) {
@@ -174,7 +174,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 				return NULL;
 			if (!commit->buffer) {
 				commit->buffer = buffer;
-				eaten = 1;
+				*eaten_p = 1;
 			}
 			obj = &commit->object;
 		}
@@ -191,7 +191,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 	}
 	if (obj && obj->type == OBJ_NONE)
 		obj->type = type;
-	*eaten_p = eaten;
 	return obj;
 }
 
-- 
1.8.3.3.754.g9c3c367.dirty

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-17 22:08       ` Stefan Beller
  2013-07-17 22:09         ` [PATCH] parse_object_buffer: Correct freeing the buffer Stefan Beller
@ 2013-07-17 22:16         ` Stefan Beller
  2013-07-17 23:22         ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Stefan Beller @ 2013-07-17 22:16 UTC (permalink / raw)
  To: Philip Oakley
  Cc: Jeff King, Johannes Sixt, Ramsay Jones, Junio C Hamano, GIT Mailing-list

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On 07/18/2013 12:08 AM, Stefan Beller wrote:
> 
> So I started using the clang code analyzer on git. One of the 
> first warnings actually is this:
> 

So in case somebody else would also like to play around with the
clang static code analyzer:

# get clang:
cd <good location>
git clone http://llvm.org/git/llvm.git
cd llvm/tools
git clone http://llvm.org/git/clang
cd <good location>
mkdir build && cd build
cmake ..
# if desired make install

# in the Makefile of git change
CC = <good location>/llvm/tools/clang/tools/scan-build/ccc-analyzer
# then obviously:
make clean
make


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-17 22:08       ` Stefan Beller
  2013-07-17 22:09         ` [PATCH] parse_object_buffer: Correct freeing the buffer Stefan Beller
  2013-07-17 22:16         ` [PATCH] Fix some sparse warnings Stefan Beller
@ 2013-07-17 23:22         ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2013-07-17 23:22 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Philip Oakley, Jeff King, Johannes Sixt, Ramsay Jones, GIT Mailing-list

Stefan Beller <stefanbeller@googlemail.com> writes:

> And the parse_object_buffer looks like this with respect to the eaten 
> variable:
> 	struct object *parse_object_buffer(...)
> 	{
> 		int eaten = 0;
> 		if (something)
> 			return NULL;
> 		...
> 		if (something_different)
> 			eaten=1;
> 		*eaten_p = eaten;
> 	}
> ...
> Was there a particular idea or goal behind first having a local eaten
> variable, which later near the correct return of the function was used to set the 
> eaten_p?

I didn't run "blame" to see the evolution of this function, but I
suspect that the original code, when the "eaten" local variable was
introduced, very much tried to do exactly what you suspect.  The
early return codepaths you see in today's code may be much newer,
added without much thinking about the exact issue you are bringing
up.

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

* Re: [PATCH] Fix some sparse warnings
  2013-07-16  6:21   ` Jeff King
  2013-07-16 20:53     ` Philip Oakley
@ 2013-07-18 17:58     ` Ramsay Jones
  1 sibling, 0 replies; 12+ messages in thread
From: Ramsay Jones @ 2013-07-18 17:58 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Sixt, Junio C Hamano, GIT Mailing-list

Jeff King wrote:
> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
> 
>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>>> Sparse issues three "Using plain integer as NULL pointer" warnings.
>>> Each warning relates to the use of an '{0}' initialiser expression
>>> in the declaration of an 'struct object_info'.
>>
>> I question the value of this warning. Initialization with '= {0}' is a
>> well-established idiom, and sparse should know about it. Also, plain 0
>> *is* a null pointer constant.
> 
> I agree with you. It's not a bug, and I think sparse is being overly
> picky here; it is missing the forest for the trees in interpreting the
> idiom.

Yes, last time this came up, I looked at writing a patch to sparse to
allow this without complaint. It's still on my sparse TODO list, but
even if I finished it tonight, it would take a while to get into a
released version of sparse. ;-)

> Still, it may be worth tweaking in the name of eliminating compiler
> noise, since it does not cost us very much to do so (and I believe we
> have done so in the past, too).
> 
> We could also ask people with sparse to turn off the "use NULL instead
> of 0" warning, but I think it _is_ a useful warning elsewhere (even
> though it is never a bug, it violates our style guidelines and may be an
> indication of a bug).

Indeed, if it wasn't for this, I would be happy to turn this warning off.

ATB,
Ramsay Jones

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

end of thread, other threads:[~2013-07-18 20:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-15 17:31 [PATCH] Fix some sparse warnings Ramsay Jones
2013-07-16  5:57 ` Johannes Sixt
2013-07-16  6:21   ` Jeff King
2013-07-16 20:53     ` Philip Oakley
2013-07-16 21:18       ` Stefan Beller
2013-07-16 22:18         ` Philip Oakley
2013-07-17  5:47         ` Johannes Sixt
2013-07-17 22:08       ` Stefan Beller
2013-07-17 22:09         ` [PATCH] parse_object_buffer: Correct freeing the buffer Stefan Beller
2013-07-17 22:16         ` [PATCH] Fix some sparse warnings Stefan Beller
2013-07-17 23:22         ` Junio C Hamano
2013-07-18 17:58     ` Ramsay Jones

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.