* [PATCH] usage: add NORETURN to BUG() function definitions
@ 2017-05-21 22:25 Ramsay Jones
2017-05-22 1:43 ` Junio C Hamano
2017-05-22 11:19 ` [PATCH] usage: add NORETURN to BUG() function definitions Jeff King
0 siblings, 2 replies; 24+ messages in thread
From: Ramsay Jones @ 2017-05-21 22:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
BUG() functions and macros as a replacement for calls to die("BUG: ..").
The use of NORETURN on the declarations (in git-compat-util.h) and the
lack of NORETURN on the function definitions, however, leads sparse to
complain thus:
SP usage.c
usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
(originally declared at git-compat-util.h:1074) - different modifiers
In order to suppress the sparse error, add the NORETURN to the function
definitions.
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Junio,
This is built on 'next', which has now merged the 'jk/bug-to-abort'
branch.
Thanks!
ATB,
Ramsay Jones
usage.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/usage.c b/usage.c
index 7e6cb2028..1f63e033e 100644
--- a/usage.c
+++ b/usage.c
@@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
}
#ifdef HAVE_VARIADIC_MACROS
-void BUG_fl(const char *file, int line, const char *fmt, ...)
+NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
@@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...)
va_end(ap);
}
#else
-void BUG(const char *fmt, ...)
+NORETURN void BUG(const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
--
2.13.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-21 22:25 [PATCH] usage: add NORETURN to BUG() function definitions Ramsay Jones
@ 2017-05-22 1:43 ` Junio C Hamano
2017-05-22 2:13 ` Ramsay Jones
2017-05-22 11:19 ` [PATCH] usage: add NORETURN to BUG() function definitions Jeff King
1 sibling, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2017-05-22 1:43 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
> BUG() functions and macros as a replacement for calls to die("BUG: ..").
> The use of NORETURN on the declarations (in git-compat-util.h) and the
> lack of NORETURN on the function definitions, however, leads sparse to
> complain thus:
>
> SP usage.c
> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
> (originally declared at git-compat-util.h:1074) - different modifiers
>
> In order to suppress the sparse error, add the NORETURN to the function
> definitions.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Junio,
>
> This is built on 'next', which has now merged the 'jk/bug-to-abort'
> branch.
Thanks.
I cannot seem to cause sparse (v0.5.0-207-g14964df) to complain the
same way, though.
>
> Thanks!
>
> ATB,
> Ramsay Jones
>
> usage.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/usage.c b/usage.c
> index 7e6cb2028..1f63e033e 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
> }
>
> #ifdef HAVE_VARIADIC_MACROS
> -void BUG_fl(const char *file, int line, const char *fmt, ...)
> +NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
> {
> va_list ap;
> va_start(ap, fmt);
> @@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...)
> va_end(ap);
> }
> #else
> -void BUG(const char *fmt, ...)
> +NORETURN void BUG(const char *fmt, ...)
> {
> va_list ap;
> va_start(ap, fmt);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-22 1:43 ` Junio C Hamano
@ 2017-05-22 2:13 ` Ramsay Jones
2017-05-22 2:35 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-05-22 2:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
On 22/05/17 02:43, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
>> BUG() functions and macros as a replacement for calls to die("BUG: ..").
>> The use of NORETURN on the declarations (in git-compat-util.h) and the
>> lack of NORETURN on the function definitions, however, leads sparse to
>> complain thus:
>>
>> SP usage.c
>> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
>> (originally declared at git-compat-util.h:1074) - different modifiers
>>
>> In order to suppress the sparse error, add the NORETURN to the function
>> definitions.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>>
>> Hi Junio,
>>
>> This is built on 'next', which has now merged the 'jk/bug-to-abort'
>> branch.
>
> Thanks.
>
> I cannot seem to cause sparse (v0.5.0-207-g14964df) to complain the
> same way, though.
Hmm, interesting... I have an older version installed:
$ sparse --version
v0.5.0-60-g6c283a0
$ sparse usage.c
usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
(originally declared at git-compat-util.h:1074) - different modifiers
$
... but a more up-to-date version agrees:
$ ~/sparse/sparse --version
v0.5.0-237-g51de1cc
$ ~/sparse/sparse usage.c
usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
(originally declared at git-compat-util.h:1074) - different modifiers
$
So, I don't know. Wait let me try your specific version:
$ ~/sparse/sparse --version
v0.5.0-207-g14964df
$ ~/sparse/sparse usage.c
usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers
$
Er, dunno. (This is on Linux Mint 18.1).
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-22 2:13 ` Ramsay Jones
@ 2017-05-22 2:35 ` Junio C Hamano
2017-05-22 2:46 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2017-05-22 2:35 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> So, I don't know. Wait let me try your specific version:
>
> $ ~/sparse/sparse --version
> v0.5.0-207-g14964df
> $ ~/sparse/sparse usage.c
> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers
> $
>
> Er, dunno. (This is on Linux Mint 18.1).
Oh, I don't question your expertise or competence. There must be
something I am doing wrong, and the version of sparse I happened to
have run was the easiest thing to point a finger at, but that does
not seem to be it.
Thanks for helping. I'll find time to dig deeper to find what's
breaking it for me.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-22 2:35 ` Junio C Hamano
@ 2017-05-22 2:46 ` Junio C Hamano
2017-05-22 14:02 ` Ramsay Jones
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2017-05-22 2:46 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
Junio C Hamano <gitster@pobox.com> writes:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> So, I don't know. Wait let me try your specific version:
>>
>> $ ~/sparse/sparse --version
>> v0.5.0-207-g14964df
>> $ ~/sparse/sparse usage.c
>> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers
>> $
>>
>> Er, dunno. (This is on Linux Mint 18.1).
>
> Oh, I don't question your expertise or competence. There must be
> something I am doing wrong, and the version of sparse I happened to
> have run was the easiest thing to point a finger at, but that does
> not seem to be it.
>
> Thanks for helping. I'll find time to dig deeper to find what's
> breaking it for me.
Hmph. I do not know what went wrong. The one I had in /usr/bin
that came from the distro was too old that it didn't give any useful
result and failed, and that was why I got v0.5.0-207-g14964df
installed in ~/gitstuff/bin/ which is early on my $PATH; I do not
think I did any other updates but now I am seeing happy results.
$ git checkout jk/bug-to-abort^1
$ make SP_OBJ=usage.sp sparse
GIT_VERSION = 2.13.0.3.g25cd291963
SP usage.c
usage.c:220:6: error: symbol 'BUG_fl' redeclared with diff...
And then with your fix, of course,
$ git checkout jk/bug-to-abort
$ make SP_OBJ=usage.sp sparse
GIT_VERSION = 2.13.0.4.g3d7dd2d3b6
SP usage.c
I am still puzzled but anyway now the problem is clearly on my end
and no longer reproduces, there is no reason to waste your time.
Sorry for the noise, and thanks for a fix again.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-21 22:25 [PATCH] usage: add NORETURN to BUG() function definitions Ramsay Jones
2017-05-22 1:43 ` Junio C Hamano
@ 2017-05-22 11:19 ` Jeff King
1 sibling, 0 replies; 24+ messages in thread
From: Jeff King @ 2017-05-22 11:19 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
On Sun, May 21, 2017 at 11:25:39PM +0100, Ramsay Jones wrote:
> Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
> BUG() functions and macros as a replacement for calls to die("BUG: ..").
> The use of NORETURN on the declarations (in git-compat-util.h) and the
> lack of NORETURN on the function definitions, however, leads sparse to
> complain thus:
>
> SP usage.c
> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
> (originally declared at git-compat-util.h:1074) - different modifiers
>
> In order to suppress the sparse error, add the NORETURN to the function
> definitions.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Thanks, I never ended up re-rolling the original, but from our previous
discussion this is obviously fine by me.
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-22 2:46 ` Junio C Hamano
@ 2017-05-22 14:02 ` Ramsay Jones
2017-05-23 3:32 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-05-22 14:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
On 22/05/17 03:46, Junio C Hamano wrote:
> Hmph. I do not know what went wrong. The one I had in /usr/bin
> that came from the distro was too old that it didn't give any useful
> result and failed, and that was why I got v0.5.0-207-g14964df
> installed in ~/gitstuff/bin/ which is early on my $PATH; I do not
> think I did any other updates but now I am seeing happy results.
>
> $ git checkout jk/bug-to-abort^1
> $ make SP_OBJ=usage.sp sparse
> GIT_VERSION = 2.13.0.3.g25cd291963
> SP usage.c
> usage.c:220:6: error: symbol 'BUG_fl' redeclared with diff...
To save yourself a little typing, if you want to run sparse over a
single file:
$ make usage.sp # ie simply replace '.c' with '.sp'
... is the recommended way to do it. This makes sure that the same
flags are passed to cgcc as are passed to gcc as part of the build.
Note that I would not normally run sparse directly on a source file
(except when messing around with different versions!), the idea is
to use cgcc as a frontend (as the Makefile does).
Having said that, I rarely run sparse over just one file (except
when fixing a sparse error/warning). On each branch (master->next->pu)
I do
$ make sparse >sp-out 2>&1 # nsp-out on 'next', psp-out on 'pu'
... so that I can diff the files from branch to branch. (I check the
master branch file by hand. There is a single warning on Linux that
is actually a sparse problem).
Just FYI, for today's fetch:
$ diff sp-out nsp-out
$ diff nsp-out psp-out
12a13
> SP blame.c
42a44,46
> diff.c:813:6: warning: symbol 'emit_line' was not declared. Should it be static?
> diff.c:828:6: warning: symbol 'emit_line_fmt' was not declared. Should it be static?
> diff.c:1865:6: warning: symbol 'print_stat_summary_0' was not declared. Should it be static?
54a59
> SP fsmonitor.c
137a143
> SP sub-process.c
170a177
> SP compat/fopen.c
276a284
> builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
296a305
> SP t/helper/test-dir-iterator.c
$
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-22 14:02 ` Ramsay Jones
@ 2017-05-23 3:32 ` Junio C Hamano
2017-05-23 20:47 ` Ramsay Jones
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2017-05-23 3:32 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Lars Schneider, GIT Mailing-list
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> Having said that, I rarely run sparse over just one file (except
> when fixing a sparse error/warning). On each branch (master->next->pu)
> I do
>
> $ make sparse >sp-out 2>&1 # nsp-out on 'next', psp-out on 'pu'
>
> ... so that I can diff the files from branch to branch. (I check the
> master branch file by hand. There is a single warning on Linux that
> is actually a sparse problem).
>
> Just FYI, for today's fetch:
>
> $ diff sp-out nsp-out
> $ diff nsp-out psp-out
> 12a13
> > SP blame.c
> 42a44,46
> > diff.c:813:6: warning: symbol 'emit_line' was not declared. Should it be static?
> > diff.c:828:6: warning: symbol 'emit_line_fmt' was not declared. Should it be static?
> > diff.c:1865:6: warning: symbol 'print_stat_summary_0' was not declared. Should it be static?
> 54a59
> > SP fsmonitor.c
> 137a143
> > SP sub-process.c
> 170a177
> > SP compat/fopen.c
> 276a284
> > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
> 296a305
> > SP t/helper/test-dir-iterator.c
> $
Interesting. One thing that I found somewhat suboptimal is that we
do not get signalled by non-zero exit. Otherwise it would make a
good addition to the "Static Analysis" task in .travis.yml file.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usage: add NORETURN to BUG() function definitions
2017-05-23 3:32 ` Junio C Hamano
@ 2017-05-23 20:47 ` Ramsay Jones
2017-06-01 20:27 ` [PATCH 0/3] -Wmemcpy-max-count & friends Luc Van Oostenryck
0 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-05-23 20:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, GIT Mailing-list
On 23/05/17 04:32, Junio C Hamano wrote:
> Interesting. One thing that I found somewhat suboptimal is that we
> do not get signalled by non-zero exit.
Warnings don't lead to non-zero exit, but similarly to -Werror, you can
provide a -Wsparse-error to turn warnings into errors:
$ make builtin/worktree.sp
SP builtin/worktree.c
builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
$
$ make SPARSE_FLAGS=-Wsparse-error builtin/worktree.sp
SP builtin/worktree.c
builtin/worktree.c:539:38: error: Using plain integer as NULL pointer
Makefile:2370: recipe for target 'builtin/worktree.sp' failed
make: *** [builtin/worktree.sp] Error 1
$
Unfortunately, that does not help too much because, as I mentioned before,
one warning is actually a sparse problem (and you can't turn it off):
$ make pack-revindex.sp
SP pack-revindex.c
pack-revindex.c:64:23: warning: memset with byte count of 262144
$
This is caused by sparse _unconditionally_ complaining about the byte count
used in calls to memset(), memcpy(), copy_to_user() and copy_from_user().
In addition, the byte count limits are hard-coded (v <= 0 || v > 100000).
About a decade ago, I wrote a patch to enable/set the limit value from the
command line, but didn't get around to sending the patch upstream. :-D
[There is actually another problem warning, if you build with NO_REGEX=1].
Since cgcc was intended to be used as proxy for gcc, you might think you
could use CC=cgcc on a regular build, but that has problems of it's own:
$ make clean >/dev/null 2>&1 # on 'pu' branch, build output in 'pout'
$ make CC=cgcc >pout1 2>&1
$ diff pout pout1
99a100
> pack-revindex.c:64:23: warning: memset with byte count of 262144
199a201
> imap-send.c:1439:9: warning: expression using sizeof on a function
200a203,207
> http.c:675:9: warning: expression using sizeof on a function
> http.c:1676:25: warning: expression using sizeof on a function
> http.c:1681:25: warning: expression using sizeof on a function
> http.c:2082:9: warning: expression using sizeof on a function
> http.c:2249:9: warning: expression using sizeof on a function
219a227
> http-walker.c:377:9: warning: expression using sizeof on a function
222a231,233
> http-push.c:189:9: warning: expression using sizeof on a function
> http-push.c:200:9: warning: expression using sizeof on a function
> http-push.c:202:9: warning: expression using sizeof on a function
228a240,243
> remote-curl.c:524:9: warning: expression using sizeof on a function
> remote-curl.c:605:17: warning: expression using sizeof on a function
> remote-curl.c:608:17: warning: expression using sizeof on a function
> remote-curl.c:676:9: warning: expression using sizeof on a function
374a390
> builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
...
$
See commit 9371322a60 (sparse: suppress some "using sizeof on a function"
warnings, 06-10-2013) for an explanation of the additional warnings.
I chose the SPARSE_FLAGS method to suppress those warnings, precisely
because I don't build git that way. (git grep -n SPARSE_FLAGS).
So, using CC='cgcc -Wsparse-error' as it stands isn't much help:
$ make clean >/dev/null 2>&1
$ make CC='cgcc -Wsparse-error'
GIT_VERSION = 2.13.0.530.g896b4ae59
* new build flags
CC credential-store.o
* new link flags
CC common-main.o
...
CC pack-objects.o
CC pack-revindex.o
pack-revindex.c:64:23: error: memset with byte count of 262144
Makefile:2036: recipe for target 'pack-revindex.o' failed
make: *** [pack-revindex.o] Error 1
$
> Otherwise it would make a
> good addition to the "Static Analysis" task in .travis.yml file.
Unfortunately, some additional work required. :-P
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/3] -Wmemcpy-max-count & friends
2017-05-23 20:47 ` Ramsay Jones
@ 2017-06-01 20:27 ` Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 1/3] memcpy()'s byte count is unsigned Luc Van Oostenryck
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-01 20:27 UTC (permalink / raw)
To: linux-sparse; +Cc: Chris Li, Ramsay Jones, Junio C Hamano, Luc Van Oostenryck
sparse will warn if memcpy() (and some others memcpy-like
functions) is called with a very large static byte count.
But this warning cannot be disabled and the limit is arbitrary
fixed at 100000.
The goal of this series is to allow to disable this warning if
found too unconvenient or to allow to configure its limit.
The series can also be found on the tree:
git://github.com/lucvoo/sparse.git memcpy-max-count
Luc Van Oostenryck (3):
memcpy()'s byte count is unsigned
add support for -Wmemcpy-max-count
add support for -fmemcpy-max-count
lib.c | 18 ++++++++++++++++++
lib.h | 2 ++
sparse.1 | 18 ++++++++++++++++++
sparse.c | 6 +++---
4 files changed, 41 insertions(+), 3 deletions(-)
--
2.13.0
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] memcpy()'s byte count is unsigned
2017-06-01 20:27 ` [PATCH 0/3] -Wmemcpy-max-count & friends Luc Van Oostenryck
@ 2017-06-01 20:27 ` Luc Van Oostenryck
2017-06-02 0:16 ` Ramsay Jones
2017-06-01 20:27 ` [PATCH 2/3] add support for -Wmemcpy-max-count Luc Van Oostenryck
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-01 20:27 UTC (permalink / raw)
To: linux-sparse; +Cc: Chris Li, Ramsay Jones, Junio C Hamano, Luc Van Oostenryck
The checker part of sparse does some checking on memcpy(),
memset(), copy_{from,to}_user() byte count and warn if the
value is known to be too large. The comparison is done with
signed numbers and it also warns if the value is negative.
However these functions take an unsigned byte count (size_t)
and so the value can't really be negative.
Additionaly, the number of bits used by sparse internally may not
be the same as the one used for the target's size_t. So sparse's
check against negative value may not be the same as checking if
the target's value would be so-large-than-the-upper-bit-is-set.
Change this by removing the test for negative values and simply
do an unsigned compare.
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
sparse.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sparse.c b/sparse.c
index 02ab97743..1cb90e20d 100644
--- a/sparse.c
+++ b/sparse.c
@@ -152,9 +152,9 @@ 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",
+ unsigned long long val = count->value;
+ if (val > 100000ULL)
+ warning(insn->pos, "%s with byte count of %llu",
show_ident(insn->func->sym->ident), val);
return;
}
--
2.13.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/3] add support for -Wmemcpy-max-count
2017-06-01 20:27 ` [PATCH 0/3] -Wmemcpy-max-count & friends Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 1/3] memcpy()'s byte count is unsigned Luc Van Oostenryck
@ 2017-06-01 20:27 ` Luc Van Oostenryck
2017-06-02 0:23 ` Ramsay Jones
2017-06-01 20:27 ` [PATCH 3/3] add support for -fmemcpy-max-count Luc Van Oostenryck
2017-06-02 0:11 ` [PATCH 0/3] -Wmemcpy-max-count & friends Ramsay Jones
3 siblings, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-01 20:27 UTC (permalink / raw)
To: linux-sparse; +Cc: Chris Li, Ramsay Jones, Junio C Hamano, Luc Van Oostenryck
sparse will warn if memcpy() (or memset(), copy_from_user(),
copy_to_user()) is called with a very large static byte-count.
But this warning is given unconditionaly while there are projects
where this warning may not be not desired.
Change this by making this warning conditional on a new warning
flag: -Wmemcpy-max-count=COUNT.
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
lib.c | 2 ++
lib.h | 1 +
sparse.1 | 8 ++++++++
sparse.c | 3 ++-
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/lib.c b/lib.c
index e1e6a1cbf..90fd2b494 100644
--- a/lib.c
+++ b/lib.c
@@ -229,6 +229,7 @@ int Wdo_while = 0;
int Winit_cstring = 0;
int Wenum_mismatch = 1;
int Wsparse_error = 0;
+int Wmemcpy_max_count = 1;
int Wnon_pointer_null = 1;
int Wold_initializer = 1;
int Wone_bit_signed_bitfield = 1;
@@ -506,6 +507,7 @@ static const struct warning {
{ "enum-mismatch", &Wenum_mismatch },
{ "sparse-error", &Wsparse_error },
{ "init-cstring", &Winit_cstring },
+ { "memcpy-max-count", &Wmemcpy_max_count },
{ "non-pointer-null", &Wnon_pointer_null },
{ "old-initializer", &Wold_initializer },
{ "one-bit-signed-bitfield", &Wone_bit_signed_bitfield },
diff --git a/lib.h b/lib.h
index 2c8529f93..8090fe247 100644
--- a/lib.h
+++ b/lib.h
@@ -116,6 +116,7 @@ extern int Wdo_while;
extern int Wenum_mismatch;
extern int Wsparse_error;
extern int Winit_cstring;
+extern int Wmemcpy_max_count;
extern int Wnon_pointer_null;
extern int Wold_initializer;
extern int Wone_bit_signed_bitfield;
diff --git a/sparse.1 b/sparse.1
index c924b3a59..efbd78d01 100644
--- a/sparse.1
+++ b/sparse.1
@@ -210,6 +210,14 @@ trouble.
Sparse does not issue these warnings by default.
.
.TP
+.B \-Wmemcpy\-max\-count
+Warn about call of \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
+\fBcopy_to_user()\fR with a large compile-time byte count.
+
+Sparse issues these warnings by default. To turn them off, use
+\fB\-Wno\-memcpy\-max\-count\fR.
+.
+.TP
.B \-Wnon\-pointer\-null
Warn about the use of 0 as a NULL pointer.
diff --git a/sparse.c b/sparse.c
index 1cb90e20d..aa5979f1a 100644
--- a/sparse.c
+++ b/sparse.c
@@ -153,7 +153,8 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
return;
if (count->type == PSEUDO_VAL) {
unsigned long long val = count->value;
- if (val > 100000ULL)
+ if (Wmemcpy_max_count && val > 100000ULL)
+
warning(insn->pos, "%s with byte count of %llu",
show_ident(insn->func->sym->ident), val);
return;
--
2.13.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/3] add support for -fmemcpy-max-count
2017-06-01 20:27 ` [PATCH 0/3] -Wmemcpy-max-count & friends Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 1/3] memcpy()'s byte count is unsigned Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 2/3] add support for -Wmemcpy-max-count Luc Van Oostenryck
@ 2017-06-01 20:27 ` Luc Van Oostenryck
2017-06-02 0:30 ` Ramsay Jones
2017-06-02 0:11 ` [PATCH 0/3] -Wmemcpy-max-count & friends Ramsay Jones
3 siblings, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-01 20:27 UTC (permalink / raw)
To: linux-sparse; +Cc: Chris Li, Ramsay Jones, Junio C Hamano, Luc Van Oostenryck
By default, sparse will warn if memcpy() (or memset(),
copy_from_user(), copy_to_user()) is called with a very large
static byte-count.
But the limit is currently fixed at 100000, which may be fine
for some uses but not for others. For example, this value is
too low for sparse to be used on the git tree where, for example,
some array used to sort the index is cleared with memset().
Change this by making the limit configurable via a new flag:
-fmemcpy-max-count.
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
lib.c | 16 ++++++++++++++++
lib.h | 1 +
sparse.1 | 10 ++++++++++
sparse.c | 3 +--
4 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/lib.c b/lib.c
index 90fd2b494..1378cc243 100644
--- a/lib.c
+++ b/lib.c
@@ -256,6 +256,7 @@ int dbg_dead = 0;
int fmem_report = 0;
int fdump_linearize;
+unsigned long fmemcpy_max_count = 100000;
int preprocess_only;
@@ -670,6 +671,19 @@ static char **handle_switch_O(char *arg, char **next)
return next;
}
+static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
+{
+ unsigned long val;
+ char *end;
+
+ val = strtoul(arg, &end, 0);
+ if (*end != '\0' || end == arg)
+ die("error: missing argument to \"-fmemcpy-max-count=\"");
+
+ fmemcpy_max_count = val;
+ return next;
+}
+
static char **handle_switch_ftabstop(char *arg, char **next)
{
char *end;
@@ -713,6 +727,8 @@ static char **handle_switch_f(char *arg, char **next)
return handle_switch_ftabstop(arg+8, next);
if (!strncmp(arg, "dump-", 5))
return handle_switch_fdump(arg+5, next);
+ if (!strncmp(arg, "memcpy-max-count=", 17))
+ return handle_switch_fmemcpy_max_count(arg+17, next);
/* handle switches w/ arguments above, boolean and only boolean below */
if (handle_simple_switch(arg, "mem-report", &fmem_report))
diff --git a/lib.h b/lib.h
index 8090fe247..b7cb451e0 100644
--- a/lib.h
+++ b/lib.h
@@ -143,6 +143,7 @@ extern int dbg_dead;
extern int fmem_report;
extern int fdump_linearize;
+extern unsigned long fmemcpy_max_count;
extern int arch_m64;
diff --git a/sparse.1 b/sparse.1
index efbd78d01..932ac82ef 100644
--- a/sparse.1
+++ b/sparse.1
@@ -216,6 +216,9 @@ Warn about call of \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
Sparse issues these warnings by default. To turn them off, use
\fB\-Wno\-memcpy\-max\-count\fR.
+
+The limit can be changed with \fB\-fmemcpy\-max\-count=COUNT\fR,
+the default being \fB100000\fR.
.
.TP
.B \-Wnon\-pointer\-null
@@ -364,6 +367,13 @@ Report some statistics about memory allocation used by the tool.
.
.SH OTHER OPTIONS
.TP
+.B \-fmemcpy-limit=COUNT
+By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
+\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
+(known at compile-time) byte-count. COUNT is the value under which
+no such warning will be given. The default limit is 100000.
+.
+.TP
.B \-ftabstop=WIDTH
Set the distance between tab stops. This helps sparse report correct
column numbers in warnings or errors. If the value is less than 1 or
diff --git a/sparse.c b/sparse.c
index aa5979f1a..bceacd94e 100644
--- a/sparse.c
+++ b/sparse.c
@@ -153,8 +153,7 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
return;
if (count->type == PSEUDO_VAL) {
unsigned long long val = count->value;
- if (Wmemcpy_max_count && val > 100000ULL)
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 0/3] -Wmemcpy-max-count & friends
2017-06-01 20:27 ` [PATCH 0/3] -Wmemcpy-max-count & friends Luc Van Oostenryck
` (2 preceding siblings ...)
2017-06-01 20:27 ` [PATCH 3/3] add support for -fmemcpy-max-count Luc Van Oostenryck
@ 2017-06-02 0:11 ` Ramsay Jones
2017-06-02 0:26 ` Luc Van Oostenryck
3 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-06-02 0:11 UTC (permalink / raw)
To: Luc Van Oostenryck, linux-sparse; +Cc: Chris Li, Junio C Hamano
On 01/06/17 21:27, Luc Van Oostenryck wrote:
> sparse will warn if memcpy() (and some others memcpy-like
> functions) is called with a very large static byte count.
> But this warning cannot be disabled and the limit is arbitrary
> fixed at 100000.
>
> The goal of this series is to allow to disable this warning if
> found too unconvenient or to allow to configure its limit.
>
>
> The series can also be found on the tree:
> git://github.com/lucvoo/sparse.git memcpy-max-count
Heh, I think you know I wrote a similar patch about a decade ago ...
[I tried to find it the other day but couldn't find it - I think
it is on my old laptop, which doesn't get booted up these days!]
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] memcpy()'s byte count is unsigned
2017-06-01 20:27 ` [PATCH 1/3] memcpy()'s byte count is unsigned Luc Van Oostenryck
@ 2017-06-02 0:16 ` Ramsay Jones
2017-06-02 1:17 ` Luc Van Oostenryck
0 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-06-02 0:16 UTC (permalink / raw)
To: Luc Van Oostenryck, linux-sparse; +Cc: Chris Li, Junio C Hamano
On 01/06/17 21:27, Luc Van Oostenryck wrote:
> The checker part of sparse does some checking on memcpy(),
> memset(), copy_{from,to}_user() byte count and warn if the
> value is known to be too large. The comparison is done with
> signed numbers and it also warns if the value is negative.
>
> However these functions take an unsigned byte count (size_t)
> and so the value can't really be negative.
My patch wasn't as careful as this. (and I was too lazy to check
that the kernel functions took a size_t parameter).
Assuming all functions take a size_t parameter, looks good.
ATB,
Ramsay Jones
>
> Additionaly, the number of bits used by sparse internally may not
> be the same as the one used for the target's size_t. So sparse's
> check against negative value may not be the same as checking if
> the target's value would be so-large-than-the-upper-bit-is-set.
>
> Change this by removing the test for negative values and simply
> do an unsigned compare.
>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
> sparse.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/sparse.c b/sparse.c
> index 02ab97743..1cb90e20d 100644
> --- a/sparse.c
> +++ b/sparse.c
> @@ -152,9 +152,9 @@ 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",
> + unsigned long long val = count->value;
> + if (val > 100000ULL)
> + warning(insn->pos, "%s with byte count of %llu",
> show_ident(insn->func->sym->ident), val);
> return;
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] add support for -Wmemcpy-max-count
2017-06-01 20:27 ` [PATCH 2/3] add support for -Wmemcpy-max-count Luc Van Oostenryck
@ 2017-06-02 0:23 ` Ramsay Jones
2017-06-02 0:33 ` Luc Van Oostenryck
0 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-06-02 0:23 UTC (permalink / raw)
To: Luc Van Oostenryck, linux-sparse; +Cc: Chris Li, Junio C Hamano
On 01/06/17 21:27, Luc Van Oostenryck wrote:
> sparse will warn if memcpy() (or memset(), copy_from_user(),
> copy_to_user()) is called with a very large static byte-count.
>
> But this warning is given unconditionaly while there are projects
> where this warning may not be not desired.
>
> Change this by making this warning conditional on a new warning
> flag: -Wmemcpy-max-count=COUNT.
As always, the hardest part for me is naming. I can't remember exactly
what I called it, but I wanted something which would kinda sum up
all the functions. So, it was something like -Wmem-move-limit=n, or
something like that, and so the test was disabled by setting it to
zero.
-Wmemcpy-max-count=count is probably a better name! ;-)
>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
> lib.c | 2 ++
> lib.h | 1 +
> sparse.1 | 8 ++++++++
> sparse.c | 3 ++-
> 4 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/lib.c b/lib.c
> index e1e6a1cbf..90fd2b494 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -229,6 +229,7 @@ int Wdo_while = 0;
> int Winit_cstring = 0;
> int Wenum_mismatch = 1;
> int Wsparse_error = 0;
> +int Wmemcpy_max_count = 1;
> int Wnon_pointer_null = 1;
> int Wold_initializer = 1;
> int Wone_bit_signed_bitfield = 1;
> @@ -506,6 +507,7 @@ static const struct warning {
> { "enum-mismatch", &Wenum_mismatch },
> { "sparse-error", &Wsparse_error },
> { "init-cstring", &Winit_cstring },
> + { "memcpy-max-count", &Wmemcpy_max_count },
> { "non-pointer-null", &Wnon_pointer_null },
> { "old-initializer", &Wold_initializer },
> { "one-bit-signed-bitfield", &Wone_bit_signed_bitfield },
> diff --git a/lib.h b/lib.h
> index 2c8529f93..8090fe247 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -116,6 +116,7 @@ extern int Wdo_while;
> extern int Wenum_mismatch;
> extern int Wsparse_error;
> extern int Winit_cstring;
> +extern int Wmemcpy_max_count;
> extern int Wnon_pointer_null;
> extern int Wold_initializer;
> extern int Wone_bit_signed_bitfield;
> diff --git a/sparse.1 b/sparse.1
> index c924b3a59..efbd78d01 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -210,6 +210,14 @@ trouble.
> Sparse does not issue these warnings by default.
> .
> .TP
> +.B \-Wmemcpy\-max\-count
> +Warn about call of \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
^^^^^^^ ^^^^^^^^
memcpy() and memset()
ATB,
Ramsay Jones
> +\fBcopy_to_user()\fR with a large compile-time byte count.
> +
> +Sparse issues these warnings by default. To turn them off, use
> +\fB\-Wno\-memcpy\-max\-count\fR.
> +.
> +.TP
> .B \-Wnon\-pointer\-null
> Warn about the use of 0 as a NULL pointer.
>
> diff --git a/sparse.c b/sparse.c
> index 1cb90e20d..aa5979f1a 100644
> --- a/sparse.c
> +++ b/sparse.c
> @@ -153,7 +153,8 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
> return;
> if (count->type == PSEUDO_VAL) {
> unsigned long long val = count->value;
> - if (val > 100000ULL)
> + if (Wmemcpy_max_count && val > 100000ULL)
> +
> warning(insn->pos, "%s with byte count of %llu",
> show_ident(insn->func->sym->ident), val);
> return;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/3] -Wmemcpy-max-count & friends
2017-06-02 0:11 ` [PATCH 0/3] -Wmemcpy-max-count & friends Ramsay Jones
@ 2017-06-02 0:26 ` Luc Van Oostenryck
0 siblings, 0 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-02 0:26 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Linux-Sparse, Chris Li, Junio C Hamano
On Fri, Jun 2, 2017 at 2:11 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 01/06/17 21:27, Luc Van Oostenryck wrote:
>> sparse will warn if memcpy() (and some others memcpy-like
>> functions) is called with a very large static byte count.
>> But this warning cannot be disabled and the limit is arbitrary
>> fixed at 100000.
>>
>> The goal of this series is to allow to disable this warning if
>> found too unconvenient or to allow to configure its limit.
>>
>>
>> The series can also be found on the tree:
>> git://github.com/lucvoo/sparse.git memcpy-max-count
>
> Heh, I think you know I wrote a similar patch about a decade ago ...
>
> [I tried to find it the other day but couldn't find it - I think
> it is on my old laptop, which doesn't get booted up these days!]
More or less, yes. I saw it in the email where you and Junio talked
about how this memset() warning was a bit blocking for git to use
sparse, which is the motivation of these 3 patches.
-- Luc
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] add support for -fmemcpy-max-count
2017-06-01 20:27 ` [PATCH 3/3] add support for -fmemcpy-max-count Luc Van Oostenryck
@ 2017-06-02 0:30 ` Ramsay Jones
2017-06-02 0:37 ` Ramsay Jones
2017-06-02 0:38 ` Luc Van Oostenryck
0 siblings, 2 replies; 24+ messages in thread
From: Ramsay Jones @ 2017-06-02 0:30 UTC (permalink / raw)
To: Luc Van Oostenryck, linux-sparse; +Cc: Chris Li, Junio C Hamano
On 01/06/17 21:27, Luc Van Oostenryck wrote:
> By default, sparse will warn if memcpy() (or memset(),
> copy_from_user(), copy_to_user()) is called with a very large
> static byte-count.
>
> But the limit is currently fixed at 100000, which may be fine
> for some uses but not for others. For example, this value is
> too low for sparse to be used on the git tree where, for example,
> some array used to sort the index is cleared with memset().
>
> Change this by making the limit configurable via a new flag:
> -fmemcpy-max-count.
>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
> lib.c | 16 ++++++++++++++++
> lib.h | 1 +
> sparse.1 | 10 ++++++++++
> sparse.c | 3 +--
> 4 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/lib.c b/lib.c
> index 90fd2b494..1378cc243 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -256,6 +256,7 @@ int dbg_dead = 0;
>
> int fmem_report = 0;
> int fdump_linearize;
> +unsigned long fmemcpy_max_count = 100000;
>
> int preprocess_only;
>
> @@ -670,6 +671,19 @@ static char **handle_switch_O(char *arg, char **next)
> return next;
> }
>
> +static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
> +{
> + unsigned long val;
> + char *end;
> +
> + val = strtoul(arg, &end, 0);
> + if (*end != '\0' || end == arg)
> + die("error: missing argument to \"-fmemcpy-max-count=\"");
> +
> + fmemcpy_max_count = val;
> + return next;
> +}
> +
> static char **handle_switch_ftabstop(char *arg, char **next)
> {
> char *end;
> @@ -713,6 +727,8 @@ static char **handle_switch_f(char *arg, char **next)
> return handle_switch_ftabstop(arg+8, next);
> if (!strncmp(arg, "dump-", 5))
> return handle_switch_fdump(arg+5, next);
> + if (!strncmp(arg, "memcpy-max-count=", 17))
> + return handle_switch_fmemcpy_max_count(arg+17, next);
>
> /* handle switches w/ arguments above, boolean and only boolean below */
> if (handle_simple_switch(arg, "mem-report", &fmem_report))
> diff --git a/lib.h b/lib.h
> index 8090fe247..b7cb451e0 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -143,6 +143,7 @@ extern int dbg_dead;
>
> extern int fmem_report;
> extern int fdump_linearize;
> +extern unsigned long fmemcpy_max_count;
>
> extern int arch_m64;
>
> diff --git a/sparse.1 b/sparse.1
> index efbd78d01..932ac82ef 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -216,6 +216,9 @@ Warn about call of \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
>
> Sparse issues these warnings by default. To turn them off, use
> \fB\-Wno\-memcpy\-max\-count\fR.
> +
> +The limit can be changed with \fB\-fmemcpy\-max\-count=COUNT\fR,
> +the default being \fB100000\fR.
> .
> .TP
> .B \-Wnon\-pointer\-null
> @@ -364,6 +367,13 @@ Report some statistics about memory allocation used by the tool.
> .
> .SH OTHER OPTIONS
> .TP
> +.B \-fmemcpy-limit=COUNT
> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
> +(known at compile-time) byte-count. COUNT is the value under which
> +no such warning will be given. The default limit is 100000.
> +.
> +.TP
So, in addition to -Wno-memcpy-max-count, you could turn the warning
off with just -fmemcpy-limit=0. cool.
Thanks!
ATB,
Ramsay Jones
> .B \-ftabstop=WIDTH
> Set the distance between tab stops. This helps sparse report correct
> column numbers in warnings or errors. If the value is less than 1 or
> diff --git a/sparse.c b/sparse.c
> index aa5979f1a..bceacd94e 100644
> --- a/sparse.c
> +++ b/sparse.c
> @@ -153,8 +153,7 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
> return;
> if (count->type == PSEUDO_VAL) {
> unsigned long long val = count->value;
> - if (Wmemcpy_max_count && val > 100000ULL)
> -
> + if (Wmemcpy_max_count && val > fmemcpy_max_count)
> warning(insn->pos, "%s with byte count of %llu",
> show_ident(insn->func->sym->ident), val);
> return;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] add support for -Wmemcpy-max-count
2017-06-02 0:23 ` Ramsay Jones
@ 2017-06-02 0:33 ` Luc Van Oostenryck
0 siblings, 0 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-02 0:33 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Linux-Sparse, Chris Li, Junio C Hamano
On Fri, Jun 2, 2017 at 2:23 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 01/06/17 21:27, Luc Van Oostenryck wrote:
>> sparse will warn if memcpy() (or memset(), copy_from_user(),
>> copy_to_user()) is called with a very large static byte-count.
>>
>> But this warning is given unconditionaly while there are projects
>> where this warning may not be not desired.
>>
>> Change this by making this warning conditional on a new warning
>> flag: -Wmemcpy-max-count=COUNT.
>
> As always, the hardest part for me is naming. I can't remember exactly
> what I called it, but I wanted something which would kinda sum up
> all the functions. So, it was something like -Wmem-move-limit=n, or
> something like that, and so the test was disabled by setting it to
> zero.
Yes, the name is far from ideal. I used 'memcpy' for the name because
I think it's the most representative but I would like to have a better name.
I considered for a while something like '-Wmem-operations' or '-Wmem-functions',
now I'm just thinking about '-Wmem-transfert-limit=...', I dunno.
> -Wmemcpy-max-count=count is probably a better name! ;-)
>> diff --git a/sparse.1 b/sparse.1
>> index c924b3a59..efbd78d01 100644
>> --- a/sparse.1
>> +++ b/sparse.1
>> @@ -210,6 +210,14 @@ trouble.
>> Sparse does not issue these warnings by default.
>> .
>> .TP
>> +.B \-Wmemcpy\-max\-count
>> +Warn about call of \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
> ^^^^^^^ ^^^^^^^^
> memcpy() and memset()
Ah yes, good catch. Thanks.
-- Luc
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] add support for -fmemcpy-max-count
2017-06-02 0:30 ` Ramsay Jones
@ 2017-06-02 0:37 ` Ramsay Jones
2017-06-02 0:38 ` Luc Van Oostenryck
1 sibling, 0 replies; 24+ messages in thread
From: Ramsay Jones @ 2017-06-02 0:37 UTC (permalink / raw)
To: Luc Van Oostenryck, linux-sparse; +Cc: Chris Li, Junio C Hamano
On 02/06/17 01:30, Ramsay Jones wrote:
>
>
> On 01/06/17 21:27, Luc Van Oostenryck wrote:
>> By default, sparse will warn if memcpy() (or memset(),
>> copy_from_user(), copy_to_user()) is called with a very large
>> static byte-count.
>>
>> But the limit is currently fixed at 100000, which may be fine
>> for some uses but not for others. For example, this value is
>> too low for sparse to be used on the git tree where, for example,
>> some array used to sort the index is cleared with memset().
>>
>> Change this by making the limit configurable via a new flag:
>> -fmemcpy-max-count.
>>
>> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
>> ---
>> lib.c | 16 ++++++++++++++++
>> lib.h | 1 +
>> sparse.1 | 10 ++++++++++
>> sparse.c | 3 +--
>> 4 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib.c b/lib.c
>> index 90fd2b494..1378cc243 100644
>> --- a/lib.c
>> +++ b/lib.c
>> @@ -256,6 +256,7 @@ int dbg_dead = 0;
>>
>> int fmem_report = 0;
>> int fdump_linearize;
>> +unsigned long fmemcpy_max_count = 100000;
>>
>> int preprocess_only;
>>
>> @@ -670,6 +671,19 @@ static char **handle_switch_O(char *arg, char **next)
>> return next;
>> }
>>
>> +static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
>> +{
>> + unsigned long val;
>> + char *end;
>> +
>> + val = strtoul(arg, &end, 0);
>> + if (*end != '\0' || end == arg)
>> + die("error: missing argument to \"-fmemcpy-max-count=\"");
>> +
>> + fmemcpy_max_count = val;
>> + return next;
>> +}
>> +
>> static char **handle_switch_ftabstop(char *arg, char **next)
>> {
>> char *end;
>> @@ -713,6 +727,8 @@ static char **handle_switch_f(char *arg, char **next)
>> return handle_switch_ftabstop(arg+8, next);
>> if (!strncmp(arg, "dump-", 5))
>> return handle_switch_fdump(arg+5, next);
>> + if (!strncmp(arg, "memcpy-max-count=", 17))
>> + return handle_switch_fmemcpy_max_count(arg+17, next);
>>
>> /* handle switches w/ arguments above, boolean and only boolean below */
>> if (handle_simple_switch(arg, "mem-report", &fmem_report))
>> diff --git a/lib.h b/lib.h
>> index 8090fe247..b7cb451e0 100644
>> --- a/lib.h
>> +++ b/lib.h
>> @@ -143,6 +143,7 @@ extern int dbg_dead;
>>
>> extern int fmem_report;
>> extern int fdump_linearize;
>> +extern unsigned long fmemcpy_max_count;
>>
>> extern int arch_m64;
>>
>> diff --git a/sparse.1 b/sparse.1
>> index efbd78d01..932ac82ef 100644
>> --- a/sparse.1
>> +++ b/sparse.1
>> @@ -216,6 +216,9 @@ Warn about call of \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
>>
>> Sparse issues these warnings by default. To turn them off, use
>> \fB\-Wno\-memcpy\-max\-count\fR.
>> +
>> +The limit can be changed with \fB\-fmemcpy\-max\-count=COUNT\fR,
>> +the default being \fB100000\fR.
>> .
>> .TP
>> .B \-Wnon\-pointer\-null
>> @@ -364,6 +367,13 @@ Report some statistics about memory allocation used by the tool.
>> .
>> .SH OTHER OPTIONS
>> .TP
>> +.B \-fmemcpy-limit=COUNT
>> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
>> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
>> +(known at compile-time) byte-count. COUNT is the value under which
>> +no such warning will be given. The default limit is 100000.
>> +.
>> +.TP
>
> So, in addition to -Wno-memcpy-max-count, you could turn the warning
> off with just -fmemcpy-limit=0. cool.
heh, so I obviously didn't read the code! Ahem. :-D
Thanks again.
ATB,
Ramsay Jones
>> .B \-ftabstop=WIDTH
>> Set the distance between tab stops. This helps sparse report correct
>> column numbers in warnings or errors. If the value is less than 1 or
>> diff --git a/sparse.c b/sparse.c
>> index aa5979f1a..bceacd94e 100644
>> --- a/sparse.c
>> +++ b/sparse.c
>> @@ -153,8 +153,7 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
>> return;
>> if (count->type == PSEUDO_VAL) {
>> unsigned long long val = count->value;
>> - if (Wmemcpy_max_count && val > 100000ULL)
>> -
>> + if (Wmemcpy_max_count && val > fmemcpy_max_count)
>> warning(insn->pos, "%s with byte count of %llu",
>> show_ident(insn->func->sym->ident), val);
>> return;
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] add support for -fmemcpy-max-count
2017-06-02 0:30 ` Ramsay Jones
2017-06-02 0:37 ` Ramsay Jones
@ 2017-06-02 0:38 ` Luc Van Oostenryck
2017-06-02 1:42 ` Ramsay Jones
1 sibling, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-02 0:38 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Linux-Sparse, Chris Li, Junio C Hamano
On Fri, Jun 2, 2017 at 2:30 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>> .SH OTHER OPTIONS
>> .TP
>> +.B \-fmemcpy-limit=COUNT
>> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
>> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
>> +(known at compile-time) byte-count. COUNT is the value under which
>> +no such warning will be given. The default limit is 100000.
>> +.
>> +.TP
>
> So, in addition to -Wno-memcpy-max-count, you could turn the warning
> off with just -fmemcpy-limit=0. cool.
>
> Thanks!
>
> ATB,
> Ramsay Jones
Well, for now setting the limit to 0 would just warn about any
non-zero memcpy/memset
but it's something that could very easily be added, sure.
-- Luc
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] memcpy()'s byte count is unsigned
2017-06-02 0:16 ` Ramsay Jones
@ 2017-06-02 1:17 ` Luc Van Oostenryck
0 siblings, 0 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-02 1:17 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Linux-Sparse, Chris Li, Junio C Hamano
On Fri, Jun 2, 2017 at 2:16 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Assuming all functions take a size_t parameter, looks good.
Even if they/some don't, it will be OK because it's how sparse
interpret the constant
that matter.
-- Luc
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] add support for -fmemcpy-max-count
2017-06-02 0:38 ` Luc Van Oostenryck
@ 2017-06-02 1:42 ` Ramsay Jones
2017-06-02 1:45 ` Luc Van Oostenryck
0 siblings, 1 reply; 24+ messages in thread
From: Ramsay Jones @ 2017-06-02 1:42 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse, Chris Li, Junio C Hamano
On 02/06/17 01:38, Luc Van Oostenryck wrote:
> On Fri, Jun 2, 2017 at 2:30 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>
>>> .SH OTHER OPTIONS
>>> .TP
>>> +.B \-fmemcpy-limit=COUNT
>>> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
>>> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
>>> +(known at compile-time) byte-count. COUNT is the value under which
>>> +no such warning will be given. The default limit is 100000.
>>> +.
>>> +.TP
>>
>> So, in addition to -Wno-memcpy-max-count, you could turn the warning
>> off with just -fmemcpy-limit=0. cool.
>>
>> Thanks!
>>
>> ATB,
>> Ramsay Jones
>
> Well, for now setting the limit to 0 would just warn about any
> non-zero memcpy/memset
> but it's something that could very easily be added, sure.
Yes, as I noted in another email, I didn't read the code correctly!
(and in my patch I had a single -Wmem-limit=n argument which _did_
disable the check when n = 0).
Naming issues aside (and I'm bad at naming, so don't listen to me
on that), I like your -W[no-]memcpy-max-count and -fmemcpy-limit=n
split.
[You may want to remove the '=COUNT' from the commit message of
the 2/3 patch].
Thanks again.
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] add support for -fmemcpy-max-count
2017-06-02 1:42 ` Ramsay Jones
@ 2017-06-02 1:45 ` Luc Van Oostenryck
0 siblings, 0 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2017-06-02 1:45 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Linux-Sparse, Chris Li, Junio C Hamano
On Fri, Jun 2, 2017 at 3:42 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>> Well, for now setting the limit to 0 would just warn about any
>> non-zero memcpy/memset
>> but it's something that could very easily be added, sure.
>
> Yes, as I noted in another email, I didn't read the code correctly!
> (and in my patch I had a single -Wmem-limit=n argument which _did_
> disable the check when n = 0).
>
> Naming issues aside (and I'm bad at naming, so don't listen to me
> on that), I like your -W[no-]memcpy-max-count and -fmemcpy-limit=n
> split.
>
> [You may want to remove the '=COUNT' from the commit message of
> the 2/3 patch].
Oh yes, indeed.
Thanks.
-- Luc
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-06-02 1:45 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-21 22:25 [PATCH] usage: add NORETURN to BUG() function definitions Ramsay Jones
2017-05-22 1:43 ` Junio C Hamano
2017-05-22 2:13 ` Ramsay Jones
2017-05-22 2:35 ` Junio C Hamano
2017-05-22 2:46 ` Junio C Hamano
2017-05-22 14:02 ` Ramsay Jones
2017-05-23 3:32 ` Junio C Hamano
2017-05-23 20:47 ` Ramsay Jones
2017-06-01 20:27 ` [PATCH 0/3] -Wmemcpy-max-count & friends Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 1/3] memcpy()'s byte count is unsigned Luc Van Oostenryck
2017-06-02 0:16 ` Ramsay Jones
2017-06-02 1:17 ` Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 2/3] add support for -Wmemcpy-max-count Luc Van Oostenryck
2017-06-02 0:23 ` Ramsay Jones
2017-06-02 0:33 ` Luc Van Oostenryck
2017-06-01 20:27 ` [PATCH 3/3] add support for -fmemcpy-max-count Luc Van Oostenryck
2017-06-02 0:30 ` Ramsay Jones
2017-06-02 0:37 ` Ramsay Jones
2017-06-02 0:38 ` Luc Van Oostenryck
2017-06-02 1:42 ` Ramsay Jones
2017-06-02 1:45 ` Luc Van Oostenryck
2017-06-02 0:11 ` [PATCH 0/3] -Wmemcpy-max-count & friends Ramsay Jones
2017-06-02 0:26 ` Luc Van Oostenryck
2017-05-22 11:19 ` [PATCH] usage: add NORETURN to BUG() function definitions Jeff King
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.