[PATCH] Fix some sparse warnings

[PATCH] Fix some sparse warnings

[Atsushi Nemoto]

Make memcpy_fromio etc. more sparse-friendly.
Remove duplicate __user annotation from __copy_to_user.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/include/asm-mips/io.h b/include/asm-mips/io.h
--- a/include/asm-mips/io.h
+++ b/include/asm-mips/io.h
@@ -25,6 +25,7 @@
 #include <asm/page.h>
 #include <asm/pgtable-bits.h>
 #include <asm/processor.h>
+#include <asm/string.h>
 
 #include <ioremap.h>
 #include <mangle-port.h>
@@ -521,9 +522,18 @@ BUILDSTRING(q, u64)
 /* Depends on MIPS II instruction set */
 #define mmiowb() asm volatile ("sync" ::: "memory")
 
-#define memset_io(a,b,c)	memset((void *)(a),(b),(c))
-#define memcpy_fromio(a,b,c)	memcpy((a),(void *)(b),(c))
-#define memcpy_toio(a,b,c)	memcpy((void *)(a),(b),(c))
+static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
+{
+	memset((void __force *) addr, val, count);
+}
+static inline void memcpy_fromio(void *dst, const volatile void __iomem *src, int count)
+{
+	memcpy(dst, (void __force *) src, count);
+}
+static inline void memcpy_toio(volatile void __iomem *dst, const void *src, int count)
+{
+	memcpy((void __force *) dst, src, count);
+}
 
 /*
  * ISA space is 'always mapped' on currently supported MIPS systems, no need
diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -417,7 +417,7 @@ extern size_t __copy_user(void *__to, co
  */
 #define __copy_to_user(to,from,n)					\
 ({									\
-	void __user __user *__cu_to;					\
+	void __user *__cu_to;						\
 	const void *__cu_from;						\
 	long __cu_len;							\
 									\

Re: [PATCH] Fix some sparse warnings

[Ralf Baechle]

On Mon, Oct 03, 2005 at 01:16:37AM +0900, Atsushi Nemoto wrote:

> Make memcpy_fromio etc. more sparse-friendly.
> Remove duplicate __user annotation from __copy_to_user.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

Thanks, applied.

  Ralf

[PATCH] Fix some sparse warnings

[Atsushi Nemoto]

Fix some sparse warnings introduced by "[MIPS] Add support for MIPS
CMP platform." patch.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
Patch against linux-queue tree.

 arch/mips/kernel/traps.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 070f787..8686133 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -196,12 +196,12 @@ EXPORT_SYMBOL(dump_stack);
 static void show_code(unsigned int __user *pc)
 {
 	long i;
-	unsigned short *pc16 = NULL;
+	unsigned short __user *pc16 = NULL;
 
 	printk("\nCode:");
 
-	if ((unsigned int)pc & 1)
-		pc16 = (unsigned short *)((unsigned int)pc & ~1);
+	if ((unsigned long)pc & 1)
+		pc16 = (unsigned short __user *)((unsigned long)pc & ~1);
 	for(i = -3 ; i < 6 ; i++) {
 		unsigned int insn;
 		if (pc16 ? __get_user(insn, pc16 + i) : __get_user(insn, pc + i)) {

Re: [PATCH] Fix some sparse warnings

[Ralf Baechle]

On Sat, Apr 26, 2008 at 01:34:35AM +0900, Atsushi Nemoto wrote:

> Fix some sparse warnings introduced by "[MIPS] Add support for MIPS
> CMP platform." patch.

Also folded into the CMP patch.  Thanks!

  Ralf

[PATCH] Fix some sparse warnings

[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'. 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

Re: [PATCH] Fix some sparse warnings

[Johannes Sixt]

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

Re: [PATCH] Fix some sparse warnings

[Jeff King]

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

Re: [PATCH] Fix some sparse warnings

[Philip Oakley]

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

Re: [PATCH] Fix some sparse warnings

[Stefan Beller]

[-- 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 --]

Re: [PATCH] Fix some sparse warnings

[Philip Oakley]

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.

Re: [PATCH] Fix some sparse warnings

[Johannes Sixt]

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

Re: [PATCH] Fix some sparse warnings

[Stefan Beller]

[-- 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 --]

Re: [PATCH] Fix some sparse warnings

[Stefan Beller]

[-- 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 --]

Re: [PATCH] Fix some sparse warnings

[Junio C Hamano]

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.

Re: [PATCH] Fix some sparse warnings

[Ramsay Jones]

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

[PATCH] Fix some sparse warnings

[Ramsay Jones]

Sparse issues some "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>
---

This was built on the next branch.

ATB,
Ramsay Jones


 sha1_file.c | 2 +-
 streaming.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH] Fix some sparse warnings

[Jeff King]

On Thu, Jul 18, 2013 at 09:25:50PM +0100, Ramsay Jones wrote:

> Sparse issues some "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>

Acked-by: Jeff King <peff@peff.net>

I thought at first we would need one more for the new callsite I added
in my series, but we use memset() in that instance, so it is fine.

-Peff

Re: [PATCH] Fix some sparse warnings

[Ramsay Jones]

Jeff King wrote:
> On Thu, Jul 18, 2013 at 09:25:50PM +0100, Ramsay Jones wrote:
> 
>> Sparse issues some "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>
> 
> Acked-by: Jeff King <peff@peff.net>
> 
> I thought at first we would need one more for the new callsite I added
> in my series, but we use memset() in that instance, so it is fine.

On an almost unrelated note ... I am now getting the following sparse
warnings:

    pack-revindex.c:105:23: warning: memset with byte count of 262144

This is a little annoying, since there is no way to turn this off. :(
(which I consider a bug in sparse).

Sparse has special-case code to check calls to memset(), memcpy(),
copy_to_user() and copy_from_user(). The code that checks the byte
count argument looks like:

static void check_byte_count(struct instruction *insn, pseudo_t count)
{
        if (!count)
                return;
        if (count->type == PSEUDO_VAL) {
                long long val = count->value;
                if (val <= 0 || val > 100000)
                        warning(insn->pos, "%s with byte count of %lld",
                                show_ident(insn->func->sym->ident), val);
                return;
        }
        /* OK, we could try to do the range analysis here */
}

I will just ignore this for now. ;-)

ATB,
Ramsay Jones

Re: [PATCH] Fix some sparse warnings

[Jonathan Nieder]

Hi,

Ramsay Jones wrote:

> Sparse issues some "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}'.

I agree that this is worth solving but the fix feels like a move in
the wrong direction.  Before this patch, the initialization says
"= {0}", which basically means "memset it to 0".  Afterward, the
initializer depends on the order of fields in the struct.

How about something like the following?

diff --git i/cache.h w/cache.h
index f2915509..ba028b75 100644
--- i/cache.h
+++ w/cache.h
@@ -1124,6 +1124,7 @@ struct object_info {
 		} packed;
 	} u;
 };
+#define OBJECT_INFO_INIT { NULL, NULL, OI_CACHED, { { NULL, 0, 0 } } }
 extern int sha1_object_info_extended(const unsigned char *, struct object_info *);
 
 /* Dumb servers support */
diff --git i/sha1_file.c w/sha1_file.c
index 6baed676..2d812b8d 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -2421,7 +2421,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 = OBJECT_INFO_INIT;
 
 	oi.sizep = sizep;
 	return sha1_object_info_extended(sha1, &oi);
diff --git i/streaming.c w/streaming.c
index cac282f0..b4c40ad9 100644
--- i/streaming.c
+++ w/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 = OBJECT_INFO_INIT;
 	const unsigned char *real = lookup_replace_object(sha1);
 	enum input_source src = istream_source(real, type, &oi);
 

Re: [PATCH] Fix some sparse warnings

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> How about something like the following?
>
> diff --git i/cache.h w/cache.h
> index f2915509..ba028b75 100644
> --- i/cache.h
> +++ w/cache.h
> @@ -1124,6 +1124,7 @@ struct object_info {
>  		} packed;
>  	} u;
>  };
> +#define OBJECT_INFO_INIT { NULL, NULL, OI_CACHED, { { NULL, 0, 0 } } }

There are quite a many hits from

	$ git grep '= { *NULL *};'

only some of which are "char *var[] = { NULL }", which is perfectly
fine, but others are to initialise "struct foo" (or "foo_t" which is
typedef'ed) that share the same "writing NULL means the initialiser
knows the order of the fields".

I doubt if the above #define is a maintainable solution, and worse
yet, thinking about the fact that such a macro is necessary only for
a structure whose first field happens to be of a pointer type, it
strongly suggests me that it is not conceptually the right thing to
do.

Can't we have sparse fixed (or skip this specific warning) instead?