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

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

[Derek Morton]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Gore, Tim]

> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Morton, Derek J]

 
>> > -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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
-- 
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

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

[Damien Lespiau]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Gore, Tim]

> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Derek Morton]

Fixed variables incorrectly declared as signed instead of unsigned.

Fixed 'unused parameter' warning from signal handlers that were
not using the signal parameter.

v2: Addressed comments from Tim Gore

Signed-off-by: Derek Morton <derek.j.morton@intel.com>
---
 lib/igt_core.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 8a1a249..62b1e6a 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1104,7 +1104,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 +1121,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 +1229,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,10 +1380,10 @@ 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)
@@ -1419,7 +1423,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 +1485,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 +1525,7 @@ err:
 void igt_disable_exit_handler(void)
 {
 	sigset_t set;
-	int i;
+	unsigned int i;
 
 	if (exit_handler_disabled)
 		return;
@@ -1724,6 +1728,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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Tue, 19 May 2015, Derek Morton <derek.j.morton@intel.com> wrote:
> Fixed variables incorrectly declared as signed instead of unsigned.
>
> Fixed 'unused parameter' warning from signal handlers that were
> not using the signal parameter.
>
> v2: Addressed comments from Tim Gore
>
> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
> ---
>  lib/igt_core.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 8a1a249..62b1e6a 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -1104,7 +1104,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 +1121,10 @@ static int __waitpid(pid_t pid)
>  
>  static void fork_helper_exit_handler(int sig)
>  {
> +	(void)sig; /* Not used, suppress warning */

No, this is not the way to go.

If you really want to get rid of the warning (not enabled by default),
you should define a macro:

#define unused(x) x __attribute__ ((unused))

and make the change:

-static void fork_helper_exit_handler(int sig)
+static void fork_helper_exit_handler(unused(int sig))

And to make that right, you should define unused depending on the
compiler you're using.

> +
>  	/* 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 +1229,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,10 +1380,10 @@ 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);

I think the warning is much better than having an explicit cast in the
code like this.

BR,
Jani.


>  }
>  
>  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;
> +	unsigned int 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;
> +	unsigned int 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;
> +	unsigned int i;
>  
>  	if (exit_handler_disabled)
>  		return;
> @@ -1724,6 +1728,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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx