[PATCH] evaluate: reject post-ops on void*

[PATCH] evaluate: reject post-ops on void*

[Jan Pokorný]

Following example haven't been linearized correctly:

	static void *inc_ptr(void *a) {
		return ++a;
	}

test-linearize:

	<entry-point>
	add.32      %r2 <- %arg1, $-1
	ret.32      %r2

Apparently, something went wrong with -1 value to be added to the original
pointer.  When void* substituted for int*, the result is as expected
(considering 32b int):

	<entry-point>
	add.32      %r2 <- %arg1, $4
	ret.32      %r2

The proposed patch turns post-ops on void* operand into an error
(taking the same route as with function operand).

When running check with C=2 on my old linux-3.0.6 configuration,
this detected a few occurencies:

	drivers/scsi/scsi_proc.c:405:31: error: bad argument type for ++/--
	drivers/scsi/scsi_proc.c:413:23: error: bad argument type for ++/--
	net/bluetooth/hci_core.c:1545:29: error: bad argument type for ++/--
	net/bluetooth/l2cap_core.c:1825:37: error: bad argument type for ++/--
	net/bluetooth/bnep/core.c:239:13: error: bad argument type for ++/--
	lib/sort.c:25:27: error: bad argument type for ++/--
	lib/sort.c:26:27: error: bad argument type for ++/--
	lib/check_signature.c:21:24: error: bad argument type for ++/--

All of them seems trivially fixable.

Alternatively, I can turn it to emit an warning only (should be
configurable/default?).  To be noted that GCC will only emit an warning,
and only if -pedantic or -Wpointer-arith specified, but I consider
it rather a convenient exception.

When this gets clear, I'll add also the respective test-case.

Signed-off-by: Jan Pokorny <pokorny_jan@seznam.cz>
---
 evaluate.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index bebe968..b6d706b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1711,7 +1711,8 @@ static struct symbol *evaluate_postop(struct expression *expr)
 		multiply = 1;
 	} else if (class == TYPE_PTR) {
 		struct symbol *target = examine_pointer_target(ctype);
-		if (!is_function(target))
+		/* beside function, reject also void* (due to sizeof(void)) */
+		if (target != &void_ctype && !is_function(target))
 			multiply = bits_to_bytes(target->bit_size);
 	}
 
-- 
1.7.3.4

[RFC] evaluate: pointer arithmetics on plain void* (was: [PATCH] evaluate: reject post-ops on void*)

[Jan Pokorný]

Hm, I was talking about post-ops only, the example showed a failing pre-op
...both is a single case as evaluate_preop boils down to evaluate_postop.

I went through evaluate.c again and I think these cases should be somehow
unified (stating current state):

1. evaluate_postop:
  a. function: error ("bad argument type for ++/--")
  b. void*:    nothing, but buggy evaluation (add/sub -1)

2. evaluate_ptr_add
  a. function: warning ("arithmetics on pointers to functions")
  b. void*:    nothing, sizeof(void) implicitly considered as 1

3. evaluate_sizeof:
  a. function: warning ("expression using sizeof on a function")
  b. void:     warning ("expression using sizeof(void)")
  (c. bool:     warning ("expression using sizeof bool"))

4. evaluate_ptrsizeof:
  - ?


The original patch makes 1b. an equivalent to 1a.  The alternative
is to handle 1b. in a similar way as 2b.  Maybe both would deserve
a warning.

Still, there is at least one inconsistency: 1a. vs 2a. (error vs warning).

On 01/23/2012 12:31 AM, Jan Pokorný wrote:
> Following example haven't been linearized correctly:
> 
> 	static void *inc_ptr(void *a) {
> 		return ++a;
> 	}
> 
> test-linearize:
> 
> 	<entry-point>
> 	add.32      %r2 <- %arg1, $-1
> 	ret.32      %r2
> 
> Apparently, something went wrong with -1 value to be added to the original
> pointer.  When void* substituted for int*, the result is as expected
> (considering 32b int):
> 
> 	<entry-point>
> 	add.32      %r2 <- %arg1, $4
> 	ret.32      %r2
> 
> The proposed patch turns post-ops on void* operand into an error
> (taking the same route as with function operand).
> 
> When running check with C=2 on my old linux-3.0.6 configuration,
> this detected a few occurencies:
> 
> 	drivers/scsi/scsi_proc.c:405:31: error: bad argument type for ++/--
> 	drivers/scsi/scsi_proc.c:413:23: error: bad argument type for ++/--
> 	net/bluetooth/hci_core.c:1545:29: error: bad argument type for ++/--
> 	net/bluetooth/l2cap_core.c:1825:37: error: bad argument type for ++/--
> 	net/bluetooth/bnep/core.c:239:13: error: bad argument type for ++/--
> 	lib/sort.c:25:27: error: bad argument type for ++/--
> 	lib/sort.c:26:27: error: bad argument type for ++/--
> 	lib/check_signature.c:21:24: error: bad argument type for ++/--
> 
> All of them seems trivially fixable.
> 
> Alternatively, I can turn it to emit an warning only (should be
> configurable/default?).  To be noted that GCC will only emit an warning,
> and only if -pedantic or -Wpointer-arith specified, but I consider
> it rather a convenient exception.
> 
> When this gets clear, I'll add also the respective test-case.
> 
> Signed-off-by: Jan Pokorny <pokorny_jan@seznam.cz>
> ---
>  evaluate.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/evaluate.c b/evaluate.c
> index bebe968..b6d706b 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1711,7 +1711,8 @@ static struct symbol *evaluate_postop(struct expression *expr)
>  		multiply = 1;
>  	} else if (class == TYPE_PTR) {
>  		struct symbol *target = examine_pointer_target(ctype);
> -		if (!is_function(target))
> +		/* beside function, reject also void* (due to sizeof(void)) */
> +		if (target != &void_ctype && !is_function(target))
>  			multiply = bits_to_bytes(target->bit_size);
>  	}
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] evaluate: pointer arithmetics on plain void*

[Jan Pokorný]

Hi again,

On 01/23/2012 11:32 AM, Jan Pokorný wrote:
> Hm, I was talking about post-ops only, the example showed a failing pre-op
> ...both is a single case as evaluate_preop boils down to evaluate_postop.
> 
> I went through evaluate.c again and I think these cases should be somehow
> unified (stating current state):
> 
> 1. evaluate_postop:
>   a. function: error ("bad argument type for ++/--")
>   b. void*:    nothing, but buggy evaluation (add/sub -1)

that "buggy evaluation" is a kind of regression that can be tracked down
to commit [1], which first started the "void != char" idea and which
missed that evaluate_postop function.

> 2. evaluate_ptr_add
>   a. function: warning ("arithmetics on pointers to functions")
>   b. void*:    nothing, sizeof(void) implicitly considered as 1
> 
> 3. evaluate_sizeof:
>   a. function: warning ("expression using sizeof on a function")
>   b. void:     warning ("expression using sizeof(void)")
>   (c. bool:     warning ("expression using sizeof bool"))
> 
> 4. evaluate_ptrsizeof:
>   - ?
> 
> 
> The original patch makes 1b. an equivalent to 1a.  The alternative
> is to handle 1b. in a similar way as 2b.  Maybe both would deserve
> a warning.

The most moderate alternative is to take what what used in
evaluate_ptr_add into evaluate_postop.

However, the question whether to follow assumptions that GCC makes
emerges.  I tried also clang -pedantic, I got:

	warning: arithmetic on a pointer to void is a GNU extension
	         [-pedantic,-Wpointer-arith]

> Still, there is at least one inconsistency: 1a. vs 2a. (error vs warning).

[1] http://git.kernel.org/?p=devel/sparse/sparse.git;a=commit;h=405cd6e

-- 
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] evaluate: pointer arithmetics on plain void* (was: [PATCH] evaluate: reject post-ops on void*)

[Christopher Li]

On Mon, Jan 23, 2012 at 2:32 AM, Jan Pokorný <pokorny_jan@seznam.cz> wrote:
> Hm, I was talking about post-ops only, the example showed a failing pre-op
> ...both is a single case as evaluate_preop boils down to evaluate_postop.
>
> I went through evaluate.c again and I think these cases should be somehow
> unified (stating current state):

I am leaning towards make the explicit sizeof(void) as error.
Implicate sizeof(void) used in the pointer arithmetic are (optional) warnings.
The question is we give warning or not. Hmm.

However, forcing the type cast to get rid of the warning is very ugly.
"(void*)(++(char*)a)" is harder to read than "++a".

It is too late tonight for me to look at the patch right now. I will
take a closer look
tomorrow.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] evaluate: reject post-ops on void*

[Christopher Li]

Sorry for the late reply.

On Sun, Jan 22, 2012 at 3:31 PM, Jan Pokorný <pokorny_jan@seznam.cz> wrote:
> Following example haven't been linearized correctly:
>
>        static void *inc_ptr(void *a) {
>                return ++a;
>        }
>
> test-linearize:
>
>        <entry-point>
>        add.32      %r2 <- %arg1, $-1
>        ret.32      %r2

Yes, you discover a bug in the area we did not treat
type size consistently.

> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1711,7 +1711,8 @@ static struct symbol *evaluate_postop(struct expression *expr)
>                multiply = 1;
>        } else if (class == TYPE_PTR) {
>                struct symbol *target = examine_pointer_target(ctype);
> -               if (!is_function(target))
> +               /* beside function, reject also void* (due to sizeof(void)) */
> +               if (target != &void_ctype && !is_function(target))
>                        multiply = bits_to_bytes(target->bit_size);
>        }

Good catch, but it does not cover all the special case yet.
If you look at evaluate_sizeof(), there are 3 different types need special
handling: void, bool, function.

Same thing should be apply to implicit sizeof operations. They should
share some code rather than enumerate the special case in two different
places. Let me think about it more.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html