Re: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in igt_core.c

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx