| Message ID | 1432034489-23373-1-git-send-email-derek.j.morton@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Morton, Derek J > Sent: Tuesday, May 19, 2015 12:21 PM > To: intel-gfx@lists.freedesktop.org > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in igt_core.c > > Fixed variables incorrectly declared as signed instead of unsigned. > > Fixed 'unused parameter' warning from signal handlers that were not using > the signal parameter. > > Signed-off-by: Derek Morton <derek.j.morton@intel.com> > --- > lib/igt_core.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..fb0b634 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -440,6 +440,8 @@ static void low_mem_killer_disable(bool disable) > bool igt_exit_called; static void common_exit_handler(int sig) { > + (void)sig; /* Not used, suppress warning */ > + > if (!igt_only_list_subtests()) { > low_mem_killer_disable(false); > } > @@ -1104,7 +1106,7 @@ static pid_t helper_process_pids[] = > > static void reset_helper_process_list(void) { > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) > + for (unsigned int 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) { > + (void)sig; /* Not used, suppress warning */ > + > /* Inside a signal handler, play safe */ > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { > + for (unsigned int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { > pid_t pid = helper_process_pids[i]; > if (pid != -1) { > kill(pid, SIGTERM); > @@ -1227,6 +1231,8 @@ static void children_exit_handler(int sig) { > int status; > > + (void)sig; /* Not used, suppress warning */ > + > /* The exit handler can be called from a fatal signal, so play safe */ > while (num_test_children-- && wait(&status)) > ; > @@ -1376,16 +1382,18 @@ static void restore_sig_handler(int sig_num) > > static void restore_all_sig_handler(void) { > - int i; > + unsigned int 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) { > int i; > > + (void)sig; /* Not used, suppress warning */ > + > if (!exit_handler_count) { > return; > } > @@ -1419,7 +1427,7 @@ static bool crash_signal(int sig) > > static void fatal_sig_handler(int sig) > { > - int i; > + unsigned int i; > > for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { > if (handled_signals[i].number != sig) @@ -1481,7 +1489,7 > @@ static void fatal_sig_handler(int sig) > */ > void igt_install_exit_handler(igt_exit_handler_t fn) { > - int i; > + unsigned int i; > > for (i = 0; i < exit_handler_count; i++) > if (exit_handler_fn[i] == fn) > @@ -1521,7 +1529,7 @@ err: > void igt_disable_exit_handler(void) > { > sigset_t set; > - int i; > + unsigned int i; > > if (exit_handler_disabled) > return; > @@ -1724,6 +1732,8 @@ out: > > static void igt_alarm_handler(int signal) { > + (void)signal; /* Not used, suppress warning */ > + > igt_info("Timed out\n"); > > /* exit with failure status */ > -- > 1.9.1 In two of the functions where you use (void)sig, sig is in fact used. In common_exit_handler it is used in the assert at the end. If this creates A warning (which I observe also) it indicates that asserts are off which we Probably don't want. The build explicitly uses "-include check-ndebug.h" To create a compile error if NDEBUG is set, but this does not seem to be Working, maybe due to the Android.mk also specifying -UNDEBUG. Sorting this is probably for a separate patch.but I think you should remove The "(void)sig" from common_exit_handler, so the resulting warning will Remind us to fix the assert issue. Also, in call_exit_handlers the sig parameter is used, so the (void)sig is Not needed. Tim
On Tue, May 19, 2015 at 01:35:41PM +0000, Gore, Tim wrote: > > > > -----Original Message----- > > From: Morton, Derek J > > Sent: Tuesday, May 19, 2015 12:21 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J > > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in igt_core.c > > > > Fixed variables incorrectly declared as signed instead of unsigned. Which kind of compiler warning is this? Imo for (unsigned int i = 0; i < something; i++) is just not C style, the int there is perfectly fine. We've had this problem before with Android going ridiculously overboard with compiler warnings, and the solution back then was to remove all the silly ones for igt. It smells like the same is appropriate for this one here too. > > Fixed 'unused parameter' warning from signal handlers that were not using > > the signal parameter. Yeah unused variable because you compile out assert isn't good, it will at least break a bunch of library unit tests (the ones in lib/tests). I guess you don't run them in your Android builds? -Daniel > > > > Signed-off-by: Derek Morton <derek.j.morton@intel.com> > > --- > > lib/igt_core.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..fb0b634 100644 > > --- a/lib/igt_core.c > > +++ b/lib/igt_core.c > > @@ -440,6 +440,8 @@ static void low_mem_killer_disable(bool disable) > > bool igt_exit_called; static void common_exit_handler(int sig) { > > + (void)sig; /* Not used, suppress warning */ > > + > > if (!igt_only_list_subtests()) { > > low_mem_killer_disable(false); > > } > > @@ -1104,7 +1106,7 @@ static pid_t helper_process_pids[] = > > > > static void reset_helper_process_list(void) { > > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) > > + for (unsigned int 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) { > > + (void)sig; /* Not used, suppress warning */ > > + > > /* Inside a signal handler, play safe */ > > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { > > + for (unsigned int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { > > pid_t pid = helper_process_pids[i]; > > if (pid != -1) { > > kill(pid, SIGTERM); > > @@ -1227,6 +1231,8 @@ static void children_exit_handler(int sig) { > > int status; > > > > + (void)sig; /* Not used, suppress warning */ > > + > > /* The exit handler can be called from a fatal signal, so play safe */ > > while (num_test_children-- && wait(&status)) > > ; > > @@ -1376,16 +1382,18 @@ static void restore_sig_handler(int sig_num) > > > > static void restore_all_sig_handler(void) { > > - int i; > > + unsigned int 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) { > > int i; > > > > + (void)sig; /* Not used, suppress warning */ > > + > > if (!exit_handler_count) { > > return; > > } > > @@ -1419,7 +1427,7 @@ static bool crash_signal(int sig) > > > > static void fatal_sig_handler(int sig) > > { > > - int i; > > + unsigned int i; > > > > for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { > > if (handled_signals[i].number != sig) @@ -1481,7 +1489,7 > > @@ static void fatal_sig_handler(int sig) > > */ > > void igt_install_exit_handler(igt_exit_handler_t fn) { > > - int i; > > + unsigned int i; > > > > for (i = 0; i < exit_handler_count; i++) > > if (exit_handler_fn[i] == fn) > > @@ -1521,7 +1529,7 @@ err: > > void igt_disable_exit_handler(void) > > { > > sigset_t set; > > - int i; > > + unsigned int i; > > > > if (exit_handler_disabled) > > return; > > @@ -1724,6 +1732,8 @@ out: > > > > static void igt_alarm_handler(int signal) { > > + (void)signal; /* Not used, suppress warning */ > > + > > igt_info("Timed out\n"); > > > > /* exit with failure status */ > > -- > > 1.9.1 > > In two of the functions where you use (void)sig, sig is in fact used. > In common_exit_handler it is used in the assert at the end. If this creates > A warning (which I observe also) it indicates that asserts are off which we > Probably don't want. The build explicitly uses "-include check-ndebug.h" > To create a compile error if NDEBUG is set, but this does not seem to be > Working, maybe due to the Android.mk also specifying -UNDEBUG. > Sorting this is probably for a separate patch.but I think you should remove > The "(void)sig" from common_exit_handler, so the resulting warning will > Remind us to fix the assert issue. > Also, in call_exit_handlers the sig parameter is used, so the (void)sig is > Not needed. > > Tim > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> > -----Original Message----- >> > From: Morton, Derek J >> > Sent: Tuesday, May 19, 2015 12:21 PM >> > To: intel-gfx@lists.freedesktop.org >> > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J >> > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in >> > igt_core.c >> > >> > Fixed variables incorrectly declared as signed instead of unsigned. > >Which kind of compiler warning is this? Imo > > for (unsigned int i = 0; i < something; i++) > >is just not C style, the int there is perfectly fine. We've had this problem before with Android going ridiculously overboard with >compiler warnings, and the solution back then was to remove all the silly ones for igt. It smells like the same is appropriate for this >one here too. > The warning occurs because 'something' is of an unsigned type. In this case the macro ARRAY_SIZE uses sizeof() which returns an unsigned type. Implicit conversion between signed and unsigned types has always resulted in compiler warnings in my experience. It is not something android specific. >> > Fixed 'unused parameter' warning from signal handlers that were not >> > using the signal parameter. > >Yeah unused variable because you compile out assert isn't good, it will at least break a bunch of library unit tests (the ones in >lib/tests). I guess you don't run them in your Android builds? >-Daniel I have no idea why the asserts are compiled out for android. Tim had some suggestions which need investigating. A subject for another patch I guess. We do not run the unit tests on android because it has not been possible to run them since they were moved to libs/test. The android make file was never updated when they were moved. I need to look at look at writing a new unit test for the fatal_signal_handler so will look at getting them to build for android at the same time. As the unused parameter changes are more controversial I will remove them for now and update the patch to just fix the signed / unsigned warnings. > >> > >> > Signed-off-by: Derek Morton <derek.j.morton@intel.com> >> > --- >> > lib/igt_core.c | 24 +++++++++++++++++------- >> > 1 file changed, 17 insertions(+), 7 deletions(-) >> > >> > diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..fb0b634 >> > 100644 >> > --- a/lib/igt_core.c >> > +++ b/lib/igt_core.c >> > @@ -440,6 +440,8 @@ static void low_mem_killer_disable(bool disable) >> > bool igt_exit_called; static void common_exit_handler(int sig) { >> > + (void)sig; /* Not used, suppress warning */ >> > + >> > if (!igt_only_list_subtests()) { >> > low_mem_killer_disable(false); >> > } >> > @@ -1104,7 +1106,7 @@ static pid_t helper_process_pids[] = >> > >> > static void reset_helper_process_list(void) { >> > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) >> > + for (unsigned int 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) { >> > + (void)sig; /* Not used, suppress warning */ >> > + >> > /* Inside a signal handler, play safe */ >> > - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { >> > + for (unsigned int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) >> > +{ >> > pid_t pid = helper_process_pids[i]; >> > if (pid != -1) { >> > kill(pid, SIGTERM); >> > @@ -1227,6 +1231,8 @@ static void children_exit_handler(int sig) { >> > int status; >> > >> > + (void)sig; /* Not used, suppress warning */ >> > + >> > /* The exit handler can be called from a fatal signal, so play safe */ >> > while (num_test_children-- && wait(&status)) >> > ; >> > @@ -1376,16 +1382,18 @@ static void restore_sig_handler(int sig_num) >> > >> > static void restore_all_sig_handler(void) { >> > - int i; >> > + unsigned int 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) { >> > int i; >> > >> > + (void)sig; /* Not used, suppress warning */ >> > + >> > if (!exit_handler_count) { >> > return; >> > } >> > @@ -1419,7 +1427,7 @@ static bool crash_signal(int sig) >> > >> > static void fatal_sig_handler(int sig) { >> > - int i; >> > + unsigned int i; >> > >> > for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { >> > if (handled_signals[i].number != sig) @@ -1481,7 +1489,7 @@ >> > static void fatal_sig_handler(int sig) >> > */ >> > void igt_install_exit_handler(igt_exit_handler_t fn) { >> > - int i; >> > + unsigned int i; >> > >> > for (i = 0; i < exit_handler_count; i++) >> > if (exit_handler_fn[i] == fn) >> > @@ -1521,7 +1529,7 @@ err: >> > void igt_disable_exit_handler(void) { >> > sigset_t set; >> > - int i; >> > + unsigned int i; >> > >> > if (exit_handler_disabled) >> > return; >> > @@ -1724,6 +1732,8 @@ out: >> > >> > static void igt_alarm_handler(int signal) { >> > + (void)signal; /* Not used, suppress warning */ >> > + >> > igt_info("Timed out\n"); >> > >> > /* exit with failure status */ >> > -- >> > 1.9.1 >> >> In two of the functions where you use (void)sig, sig is in fact used. >> In common_exit_handler it is used in the assert at the end. If this >> creates A warning (which I observe also) it indicates that asserts are >> off which we Probably don't want. The build explicitly uses "-include check-ndebug.h" >> To create a compile error if NDEBUG is set, but this does not seem to >> be Working, maybe due to the Android.mk also specifying -UNDEBUG. >> Sorting this is probably for a separate patch.but I think you should >> remove The "(void)sig" from common_exit_handler, so the resulting >> warning will Remind us to fix the assert issue. >> Also, in call_exit_handlers the sig parameter is used, so the >> (void)sig is Not needed. >> >> Tim >> _______________________________________________ >> 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 //Derek
On Wed, May 20, 2015 at 08:37:56AM +0000, Morton, Derek J wrote: > >> > -----Original Message----- > >> > From: Morton, Derek J > >> > Sent: Tuesday, May 19, 2015 12:21 PM > >> > To: intel-gfx@lists.freedesktop.org > >> > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J > >> > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in > >> > igt_core.c > >> > > >> > Fixed variables incorrectly declared as signed instead of unsigned. > > > >Which kind of compiler warning is this? Imo > > > > for (unsigned int i = 0; i < something; i++) > > > >is just not C style, the int there is perfectly fine. We've had this problem before with Android going ridiculously overboard with >compiler warnings, and the solution back then was to remove all the silly ones for igt. It smells like the same is appropriate for this >one here too. > > > > The warning occurs because 'something' is of an unsigned type. In this > case the macro ARRAY_SIZE uses sizeof() which returns an unsigned type. > Implicit conversion between signed and unsigned types has always > resulted in compiler warnings in my experience. It is not something > android specific. I don't have them here when building igt, and ARRAY_SIZE is also extensively used in the kernel, also with size_of. This really sounds like Android again going overboard with enabling compiler warnings to me. Also the warning is silly here, since ARRAY_SIZE is statically known and gcc can figure out that there's no overflow possible. The warning level in kernel is such that you get overflow warnings only when gcc can prove that there's an overflow. Which makes sense, but this here imo really doesn't. -Daniel
On Wed, May 20, 2015 at 08:37:56AM +0000, Morton, Derek J wrote: > > >> > -----Original Message----- > >> > From: Morton, Derek J > >> > Sent: Tuesday, May 19, 2015 12:21 PM > >> > To: intel-gfx@lists.freedesktop.org > >> > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J > >> > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in > >> > igt_core.c > >> > > >> > Fixed variables incorrectly declared as signed instead of unsigned. > > > >Which kind of compiler warning is this? Imo > > > > for (unsigned int i = 0; i < something; i++) > > > >is just not C style, the int there is perfectly fine. We've had this > >problem before with Android going ridiculously overboard with > >>compiler warnings, and the solution back then was to remove all the > >>silly ones for igt. It smells like the same is appropriate for this > >>>one here too. > > > > The warning occurs because 'something' is of an unsigned type. In this > case the macro ARRAY_SIZE uses sizeof() which returns an unsigned > type. Implicit conversion between signed and unsigned types has always > resulted in compiler warnings in my experience. It is not something > android specific. unsigned int like that is C99 and the sizeof operator is a size_t, so maybe use that instead of unsigned int? > > >> > Fixed 'unused parameter' warning from signal handlers that were > >> > not using the signal parameter. > > > >Yeah unused variable because you compile out assert isn't good, it > >will at least break a bunch of library unit tests (the ones in > >>lib/tests). I guess you don't run them in your Android builds? > >>-Daniel > > I have no idea why the asserts are compiled out for android. Tim had > some suggestions which need investigating. A subject for another patch > I guess. We do not run the unit tests on android because it has not > been possible to run them since they were moved to libs/test. The > android make file was never updated when they were moved. I need to > look at look at writing a new unit test for the fatal_signal_handler > so will look at getting them to build for android at the same time. > > As the unused parameter changes are more controversial I will remove > them for now and update the patch to just fix the signed / unsigned > warnings. FWIW, I'd used the gcc unused attribute for things like that instead of those void expressions. #define __unused __attribute__((unused)) void foo(int bar __unused) { ... }
> -----Original Message----- > From: Lespiau, Damien > Sent: Wednesday, May 20, 2015 10:48 AM > To: Morton, Derek J > Cc: Daniel Vetter; Gore, Tim; intel-gfx@lists.freedesktop.org; Wood, Thomas > Subject: Re: [Intel-gfx] [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in > igt_core.c > > On Wed, May 20, 2015 at 08:37:56AM +0000, Morton, Derek J wrote: > > > > >> > -----Original Message----- > > >> > From: Morton, Derek J > > >> > Sent: Tuesday, May 19, 2015 12:21 PM > > >> > To: intel-gfx@lists.freedesktop.org > > >> > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J > > >> > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in > > >> > igt_core.c > > >> > > > >> > Fixed variables incorrectly declared as signed instead of unsigned. > > > > > >Which kind of compiler warning is this? Imo > > > > > > for (unsigned int i = 0; i < something; i++) > > > > > >is just not C style, the int there is perfectly fine. We've had this > > >problem before with Android going ridiculously overboard with > > >>compiler warnings, and the solution back then was to remove all the > > >>silly ones for igt. It smells like the same is appropriate for this > > >>>one here too. > > > > > > > The warning occurs because 'something' is of an unsigned type. In this > > case the macro ARRAY_SIZE uses sizeof() which returns an unsigned > > type. Implicit conversion between signed and unsigned types has always > > resulted in compiler warnings in my experience. It is not something > > android specific. > > unsigned int like that is C99 and the sizeof operator is a size_t, so maybe use > that instead of unsigned int? > > > > > >> > Fixed 'unused parameter' warning from signal handlers that were > > >> > not using the signal parameter. > > > > > >Yeah unused variable because you compile out assert isn't good, it > > >will at least break a bunch of library unit tests (the ones in > > >>lib/tests). I guess you don't run them in your Android builds? > > >>-Daniel > > > > I have no idea why the asserts are compiled out for android. Tim had > > some suggestions which need investigating. A subject for another patch > > I guess. We do not run the unit tests on android because it has not > > been possible to run them since they were moved to libs/test. The > > android make file was never updated when they were moved. I need to > > look at look at writing a new unit test for the fatal_signal_handler > > so will look at getting them to build for android at the same time. > > > > As the unused parameter changes are more controversial I will remove > > them for now and update the patch to just fix the signed / unsigned > > warnings. > > FWIW, I'd used the gcc unused attribute for things like that instead of those > void expressions. > > #define __unused __attribute__((unused)) > > void foo(int bar __unused) > { > ... > } > > -- > Damien My penny's worth is this. Most of the time assigning between signed and unsigned is just due to sloppy coding. We all do it for sure, and I see it everywhere. But it can lead to problems and it is very kind of the compiler to warn you. By default GCC only warns you in C++, but I think Google must have tweaked their GCC build, I cannot see it turned on in our build command. Fixing these is trivial, does not introduce extra code and reduces the noise when building for Android. And its even good style (unlike this sentence). I agree with Damien that size_t is probably better in this case. Tim
On Wed, 20 May 2015, "Gore, Tim" <tim.gore@intel.com> wrote: >> -----Original Message----- >> From: Lespiau, Damien >> Sent: Wednesday, May 20, 2015 10:48 AM >> To: Morton, Derek J >> Cc: Daniel Vetter; Gore, Tim; intel-gfx@lists.freedesktop.org; Wood, Thomas >> Subject: Re: [Intel-gfx] [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in >> igt_core.c >> >> On Wed, May 20, 2015 at 08:37:56AM +0000, Morton, Derek J wrote: >> > >> > >> > -----Original Message----- >> > >> > From: Morton, Derek J >> > >> > Sent: Tuesday, May 19, 2015 12:21 PM >> > >> > To: intel-gfx@lists.freedesktop.org >> > >> > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J >> > >> > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in >> > >> > igt_core.c >> > >> > >> > >> > Fixed variables incorrectly declared as signed instead of unsigned. >> > > >> > >Which kind of compiler warning is this? Imo >> > > >> > > for (unsigned int i = 0; i < something; i++) >> > > >> > >is just not C style, the int there is perfectly fine. We've had this >> > >problem before with Android going ridiculously overboard with >> > >>compiler warnings, and the solution back then was to remove all the >> > >>silly ones for igt. It smells like the same is appropriate for this >> > >>>one here too. >> > > >> > >> > The warning occurs because 'something' is of an unsigned type. In this >> > case the macro ARRAY_SIZE uses sizeof() which returns an unsigned >> > type. Implicit conversion between signed and unsigned types has always >> > resulted in compiler warnings in my experience. It is not something >> > android specific. >> >> unsigned int like that is C99 and the sizeof operator is a size_t, so maybe use >> that instead of unsigned int? >> >> > >> > >> > Fixed 'unused parameter' warning from signal handlers that were >> > >> > not using the signal parameter. >> > > >> > >Yeah unused variable because you compile out assert isn't good, it >> > >will at least break a bunch of library unit tests (the ones in >> > >>lib/tests). I guess you don't run them in your Android builds? >> > >>-Daniel >> > >> > I have no idea why the asserts are compiled out for android. Tim had >> > some suggestions which need investigating. A subject for another patch >> > I guess. We do not run the unit tests on android because it has not >> > been possible to run them since they were moved to libs/test. The >> > android make file was never updated when they were moved. I need to >> > look at look at writing a new unit test for the fatal_signal_handler >> > so will look at getting them to build for android at the same time. >> > >> > As the unused parameter changes are more controversial I will remove >> > them for now and update the patch to just fix the signed / unsigned >> > warnings. >> >> FWIW, I'd used the gcc unused attribute for things like that instead of those >> void expressions. >> >> #define __unused __attribute__((unused)) >> >> void foo(int bar __unused) >> { >> ... >> } >> >> -- >> Damien > > My penny's worth is this. Most of the time assigning between signed and unsigned > is just due to sloppy coding. We all do it for sure, and I see it everywhere. But it can > lead to problems and it is very kind of the compiler to warn you. By default GCC only > warns you in C++, but I think Google must have tweaked their GCC build, I cannot > see it turned on in our build command. Fixing these is trivial, does not introduce > extra code and reduces the noise when building for Android. And its even good > style (unlike this sentence). I agree with Damien that size_t is probably better > in this case. Like both Chris and I said in reply to different versions of the patch, having to use an explicit cast because of the signed->unsigned change is bad too. Worse, I think. BR, Jani. > Tim > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/lib/igt_core.c b/lib/igt_core.c index 8a1a249..fb0b634 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -440,6 +440,8 @@ static void low_mem_killer_disable(bool disable) bool igt_exit_called; static void common_exit_handler(int sig) { + (void)sig; /* Not used, suppress warning */ + if (!igt_only_list_subtests()) { low_mem_killer_disable(false); } @@ -1104,7 +1106,7 @@ static pid_t helper_process_pids[] = static void reset_helper_process_list(void) { - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) + for (unsigned int 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) { + (void)sig; /* Not used, suppress warning */ + /* Inside a signal handler, play safe */ - for (int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { + for (unsigned int i = 0; i < ARRAY_SIZE(helper_process_pids); i++) { pid_t pid = helper_process_pids[i]; if (pid != -1) { kill(pid, SIGTERM); @@ -1227,6 +1231,8 @@ static void children_exit_handler(int sig) { int status; + (void)sig; /* Not used, suppress warning */ + /* The exit handler can be called from a fatal signal, so play safe */ while (num_test_children-- && wait(&status)) ; @@ -1376,16 +1382,18 @@ static void restore_sig_handler(int sig_num) static void restore_all_sig_handler(void) { - int i; + unsigned int 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) { int i; + (void)sig; /* Not used, suppress warning */ + if (!exit_handler_count) { return; } @@ -1419,7 +1427,7 @@ static bool crash_signal(int sig) static void fatal_sig_handler(int sig) { - int i; + unsigned int i; for (i = 0; i < ARRAY_SIZE(handled_signals); i++) { if (handled_signals[i].number != sig) @@ -1481,7 +1489,7 @@ static void fatal_sig_handler(int sig) */ void igt_install_exit_handler(igt_exit_handler_t fn) { - int i; + unsigned int i; for (i = 0; i < exit_handler_count; i++) if (exit_handler_fn[i] == fn) @@ -1521,7 +1529,7 @@ err: void igt_disable_exit_handler(void) { sigset_t set; - int i; + unsigned int i; if (exit_handler_disabled) return; @@ -1724,6 +1732,8 @@ out: static void igt_alarm_handler(int signal) { + (void)signal; /* Not used, suppress warning */ + igt_info("Timed out\n"); /* exit with failure status */
Fixed variables incorrectly declared as signed instead of unsigned. Fixed 'unused parameter' warning from signal handlers that were not using the signal parameter. Signed-off-by: Derek Morton <derek.j.morton@intel.com> --- lib/igt_core.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)