| Message ID | 1434105457-4158-1-git-send-email-derek.j.morton@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 12, 2015 at 11:37:37AM +0100, Derek Morton wrote: > Fixed variables incorrectly declared as int instead of size_t. > > v2: Addressed comments from Tim Gore > v3: Removed 'unused parameter' changes > v4: Changed to size_t > v5: Moved declarations out of for loops > > Signed-off-by: Derek Morton <derek.j.morton@intel.com> > --- > lib/igt_core.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 8a1a249..eb0cb21 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -1104,7 +1104,9 @@ static pid_t helper_process_pids[] = > > static void reset_helper_process_list(void) > { > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) > + size_t i; > + > + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) I still maintain that your gcc is silly since it's trivial to prove that i will fit into an int. > helper_process_pids[i] = -1; > helper_process_count = 0; > } > @@ -1121,8 +1123,10 @@ static int __waitpid(pid_t pid) > > static void fork_helper_exit_handler(int sig) > { > + size_t i; > + > /* Inside a signal handler, play safe */ > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { > + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { > pid_t pid = helper_process_pids[i]; > if (pid != -1) { > kill(pid, SIGTERM); > @@ -1376,10 +1380,10 @@ static void restore_sig_handler(int sig_num) > > static void restore_all_sig_handler(void) > { > - int i; > + size_t i; > > for (i = 0; i < ARRAY_SIZE(orig_sig); i++) > - restore_sig_handler(i); > + restore_sig_handler((int)i); Even more so if you have to cast back to the real value like here. -Daniel > } > > static void call_exit_handlers(int sig) > @@ -1419,7 +1423,7 @@ static bool crash_signal(int sig) > > static void fatal_sig_handler(int sig) > { > - int i; > + size_t i; > > for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { > if (handled_signals[i].number != sig) > @@ -1481,7 +1485,7 @@ static void fatal_sig_handler(int sig) > */ > void igt_install_exit_handler(igt_exit_handler_t fn) > { > - int i; > + size_t i; > > for (i = 0; i < exit_handler_count; i++) > if (exit_handler_fn[i] == fn) > @@ -1521,7 +1525,7 @@ err: > void igt_disable_exit_handler(void) > { > sigset_t set; > - int i; > + size_t i; > > if (exit_handler_disabled) > return; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Monday, June 15, 2015 3:39 PM >To: Morton, Derek J >Cc: intel-gfx@lists.freedesktop.org; Wood, Thomas >Subject: Re: [Intel-gfx] [PATCH i-g-t v5] libs/igt_core.c: Fix compile warnings in igt_core.c > >On Fri, Jun 12, 2015 at 11:37:37AM +0100, Derek Morton wrote: >> Fixed variables incorrectly declared as int instead of size_t. >> >> v2: Addressed comments from Tim Gore >> v3: Removed 'unused parameter' changes >> v4: Changed to size_t >> v5: Moved declarations out of for loops >> >> Signed-off-by: Derek Morton <derek.j.morton@intel.com> >> --- >> lib/igt_core.c | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..eb0cb21 >> 100644 >> --- a/lib/igt_core.c >> +++ b/lib/igt_core.c >> @@ -1104,7 +1104,9 @@ static pid_t helper_process_pids[] = >> >> static void reset_helper_process_list(void) { >> - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) >> + size_t i; >> + >> + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) > >I still maintain that your gcc is silly since it's trivial to prove that i will fit into an int. I don't think your compiler makes any checks to see if it will fit. It would just silently truncate the data if it did not. This is about good coding practice in general. Refusing to fix false positives hides real warnings in the sea of compile routput. > >> helper_process_pids[i] = -1; >> helper_process_count = 0; >> } >> @@ -1121,8 +1123,10 @@ static int __waitpid(pid_t pid) >> >> static void fork_helper_exit_handler(int sig) { >> + size_t i; >> + >> /* Inside a signal handler, play safe */ >> - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { >> + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { >> pid_t pid = helper_process_pids[i]; >> if (pid != -1) { >> kill(pid, SIGTERM); >> @@ -1376,10 +1380,10 @@ static void restore_sig_handler(int sig_num) >> >> static void restore_all_sig_handler(void) { >> - int i; >> + size_t i; >> >> for (i = 0; i < ARRAY_SIZE(orig_sig); i++) >> - restore_sig_handler(i); >> + restore_sig_handler((int)i); > >Even more so if you have to cast back to the real value like here. Would you prefer I cast the ARRAY_SIZE to an int instead? //Derek >-Daniel > >> } >> >> static void call_exit_handlers(int sig) @@ -1419,7 +1423,7 @@ static >> bool crash_signal(int sig) >> >> static void fatal_sig_handler(int sig) { >> - int i; >> + size_t i; >> >> for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { >> if (handled_signals[i].number != sig) @@ -1481,7 +1485,7 @@ static >> void fatal_sig_handler(int sig) >> */ >> void igt_install_exit_handler(igt_exit_handler_t fn) { >> - int i; >> + size_t i; >> >> for (i = 0; i < exit_handler_count; i++) >> if (exit_handler_fn[i] == fn) >> @@ -1521,7 +1525,7 @@ err: >> void igt_disable_exit_handler(void) >> { >> sigset_t set; >> - int i; >> + size_t i; >> >> if (exit_handler_disabled) >> return; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch >
On Tue, Jun 23, 2015 at 03:15:40PM +0000, Morton, Derek J wrote: > > > > > >-----Original Message----- > >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > >Sent: Monday, June 15, 2015 3:39 PM > >To: Morton, Derek J > >Cc: intel-gfx@lists.freedesktop.org; Wood, Thomas > >Subject: Re: [Intel-gfx] [PATCH i-g-t v5] libs/igt_core.c: Fix compile warnings in igt_core.c > > > >On Fri, Jun 12, 2015 at 11:37:37AM +0100, Derek Morton wrote: > >> Fixed variables incorrectly declared as int instead of size_t. > >> > >> v2: Addressed comments from Tim Gore > >> v3: Removed 'unused parameter' changes > >> v4: Changed to size_t > >> v5: Moved declarations out of for loops > >> > >> Signed-off-by: Derek Morton <derek.j.morton@intel.com> > >> --- > >> lib/igt_core.c | 18 +++++++++++------- > >> 1 file changed, 11 insertions(+), 7 deletions(-) > >> > >> diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..eb0cb21 > >> 100644 > >> --- a/lib/igt_core.c > >> +++ b/lib/igt_core.c > >> @@ -1104,7 +1104,9 @@ static pid_t helper_process_pids[] = > >> > >> static void reset_helper_process_list(void) { > >> - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) > >> + size_t i; > >> + > >> + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) > > > >I still maintain that your gcc is silly since it's trivial to prove that i will fit into an int. > > I don't think your compiler makes any checks to see if it will fit. It would just silently truncate the data if it did not. This is about good coding practice in general. Refusing to fix false positives hides real warnings in the sea of compile routput. test.c:5:16: warning: comparison is always true due to limited range of data type [-Wtype-limits] for (int i = 0; i < (unsigned)-1; i++) If gcc can do it, why can't your static analyser? -Chris
diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..eb0cb21 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -1104,7 +1104,9 @@ static pid_t helper_process_pids[] = static void reset_helper_process_list(void) { - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) + size_t i; + + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) helper_process_pids[i] = -1; helper_process_count = 0; } @@ -1121,8 +1123,10 @@ static int __waitpid(pid_t pid) static void fork_helper_exit_handler(int sig) { + size_t i; + /* Inside a signal handler, play safe */ - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { + for (i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { pid_t pid = helper_process_pids[i]; if (pid != -1) { kill(pid, SIGTERM); @@ -1376,10 +1380,10 @@ static void restore_sig_handler(int sig_num) static void restore_all_sig_handler(void) { - int i; + size_t i; for (i = 0; i < ARRAY_SIZE(orig_sig); i++) - restore_sig_handler(i); + restore_sig_handler((int)i); } static void call_exit_handlers(int sig) @@ -1419,7 +1423,7 @@ static bool crash_signal(int sig) static void fatal_sig_handler(int sig) { - int i; + size_t i; for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { if (handled_signals[i].number != sig) @@ -1481,7 +1485,7 @@ static void fatal_sig_handler(int sig) */ void igt_install_exit_handler(igt_exit_handler_t fn) { - int i; + size_t i; for (i = 0; i < exit_handler_count; i++) if (exit_handler_fn[i] == fn) @@ -1521,7 +1525,7 @@ err: void igt_disable_exit_handler(void) { sigset_t set; - int i; + size_t i; if (exit_handler_disabled) return;
Fixed variables incorrectly declared as int instead of size_t. v2: Addressed comments from Tim Gore v3: Removed 'unused parameter' changes v4: Changed to size_t v5: Moved declarations out of for loops Signed-off-by: Derek Morton <derek.j.morton@intel.com> --- lib/igt_core.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)