[Xenomai-core] [PATCH][SOLO] add more warnings

[Xenomai-core] [PATCH][SOLO] add more warnings

[Robert Schwebel]

Contrary to what one would assume, -Wall doesn't warn on everything. It
turned out to add a few more options, like -Wsign-compare, -Wfloat-equal
and -Wformat-security, which is what the patch below does.

When running with these options, we get the following warnings:

kernelLib.c: In function 'kernelInit':
kernelLib.c:63: warning: comparison between signed and unsigned
msgQLib.c: In function 'msgQReceive':
msgQLib.c:165: warning: comparison between signed and unsigned
msgQLib.c: In function 'msgQSend':
msgQLib.c:227: warning: comparison between signed and unsigned
msgQLib.c:237: warning: comparison between signed and unsigned
taskLib.c: In function 'taskSpawn':
taskLib.c:382: warning: signed and unsigned type in conditional expression
pt.c: In function 'get_pt_from_id':
pt.c:56: warning: comparison between signed and unsigned
pt.c:58: warning: comparison between signed and unsigned
queue.c: In function 'get_queue_from_id':
queue.c:49: warning: comparison between signed and unsigned
queue.c: In function '__q_send':
queue.c:224: warning: comparison between signed and unsigned
queue.c: In function '__q_broadcast':
queue.c:311: warning: comparison between signed and unsigned
queue.c: In function '__q_receive':
queue.c:365: warning: comparison between signed and unsigned
rn.c: In function 'get_rn_from_id':
rn.c:41: warning: comparison between signed and unsigned
sem.c: In function 'get_sem_from_id':
sem.c:41: warning: comparison between signed and unsigned
task.c: In function 'find_psos_task':
task.c:54: warning: comparison between signed and unsigned
task.c: In function 'find_psos_task_or_self':
task.c:86: warning: comparison between signed and unsigned
task.c: In function 'get_psos_task':
task.c:110: warning: comparison between signed and unsigned
task.c: In function 'get_psos_task_or_self':
task.c:131: warning: comparison between signed and unsigned
task.c:140: warning: comparison between signed and unsigned
task.c: In function 'check_task_priority':
task.c:232: warning: comparison between signed and unsigned
tm.c: In function 'get_tm_from_id':
tm.c:43: warning: comparison between signed and unsigned

I've looked into the first ones only yet and it looks to me like the
type definitions have to be reviewed carefully. For example, TASK_ID is
defined to be unsigned long, whereas the vxworks documentation [1] looks
more like if we need 'int' there. Which also makes me wonder if vxworks
has a special idea about what 'int' is; guessing from [2] it looks like
it assumes sint32_t, so in order to fix this on 64 bit boxes, shouldn't
these cases be converted to use the inttypes.h definitions?

However, somebody with more vxworks knowledge than myself should have a
look at this.

Signed-off-by: Robert Schwebel <r.schwebel@domain.hid>

[1] http://spacegrant.colorado.edu/~dixonc/vxworks/docs/vxworks/ref/taskLib.html#taskActivate
[2] http://www.xs4all.nl/~borkhuis/vxworks/vxw_pt6.html

---
 configure.in |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: xenomai-solo/configure.in
===================================================================
--- xenomai-solo.orig/configure.in	2008-03-28 19:17:23.000000000 +0100
+++ xenomai-solo/configure.in	2008-03-28 19:19:15.000000000 +0100
@@ -163,7 +163,8 @@
 dnl Exported CFLAGS and LDFLAGS
 XENO_USER_APP_CFLAGS=$XENO_USER_CFLAGS
 XENO_USER_APP_LDFLAGS=$XENO_USER_LDFLAGS
-XENO_USER_CFLAGS="$XENO_USER_CFLAGS -Wall -pipe -fstrict-aliasing $gcc_w_noalias"
+XENO_USER_CFLAGS="$XENO_USER_CFLAGS -Wall -Wsign-compare -Wfloat-equal -Wformat-security"
+XENO_USER_CFLAGS="$XENO_USER_CFLAGS -pipe -fstrict-aliasing $gcc_w_noalias"
 
 if test x$debug_symbols = xy; then
    XENO_USER_CFLAGS="-g -O0 $XENO_USER_CFLAGS -D__SOLO_DEBUG__"

-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

Re: [Xenomai-core] [PATCH][SOLO] add more warnings

[Philippe Gerum]

Robert Schwebel wrote:
> Contrary to what one would assume, -Wall doesn't warn on everything. It
> turned out to add a few more options, like -Wsign-compare, -Wfloat-equal
> and -Wformat-security, which is what the patch below does.
> 
> When running with these options, we get the following warnings:
> 
> kernelLib.c: In function 'kernelInit':
> kernelLib.c:63: warning: comparison between signed and unsigned
> msgQLib.c: In function 'msgQReceive':
> msgQLib.c:165: warning: comparison between signed and unsigned
> msgQLib.c: In function 'msgQSend':
> msgQLib.c:227: warning: comparison between signed and unsigned
> msgQLib.c:237: warning: comparison between signed and unsigned
> taskLib.c: In function 'taskSpawn':
> taskLib.c:382: warning: signed and unsigned type in conditional expression
> pt.c: In function 'get_pt_from_id':
> pt.c:56: warning: comparison between signed and unsigned
> pt.c:58: warning: comparison between signed and unsigned
> queue.c: In function 'get_queue_from_id':
> queue.c:49: warning: comparison between signed and unsigned
> queue.c: In function '__q_send':
> queue.c:224: warning: comparison between signed and unsigned
> queue.c: In function '__q_broadcast':
> queue.c:311: warning: comparison between signed and unsigned
> queue.c: In function '__q_receive':
> queue.c:365: warning: comparison between signed and unsigned
> rn.c: In function 'get_rn_from_id':
> rn.c:41: warning: comparison between signed and unsigned
> sem.c: In function 'get_sem_from_id':
> sem.c:41: warning: comparison between signed and unsigned
> task.c: In function 'find_psos_task':
> task.c:54: warning: comparison between signed and unsigned
> task.c: In function 'find_psos_task_or_self':
> task.c:86: warning: comparison between signed and unsigned
> task.c: In function 'get_psos_task':
> task.c:110: warning: comparison between signed and unsigned
> task.c: In function 'get_psos_task_or_self':
> task.c:131: warning: comparison between signed and unsigned
> task.c:140: warning: comparison between signed and unsigned
> task.c: In function 'check_task_priority':
> task.c:232: warning: comparison between signed and unsigned
> tm.c: In function 'get_tm_from_id':
> tm.c:43: warning: comparison between signed and unsigned
> 
> I've looked into the first ones only yet and it looks to me like the
> type definitions have to be reviewed carefully. For example, TASK_ID is
> defined to be unsigned long, whereas the vxworks documentation [1] looks
> more like if we need 'int' there. Which also makes me wonder if vxworks
> has a special idea about what 'int' is;

VxWorks assumes 32bit and sizeof(void *) == sizeof(int), unfortunately. See
taskSpawn() for instance.

 guessing from [2] it looks like
> it assumes sint32_t, so in order to fix this on 64 bit boxes, shouldn't
> these cases be converted to use the inttypes.h definitions?
>

Did you mean stdint.h? What we need is an integer type which is able to carry a
pointer on 32/64bit platforms, so we should rather use intptr_t I guess, as per
C99, which expands as a long type. Object ids as unsigned long is a left over
from the co-kernel version, where we use actual integer handles returned from
kernel space, and not pointers in disguise. Will fix, thanks.

> However, somebody with more vxworks knowledge than myself should have a
> look at this.
> 
> Signed-off-by: Robert Schwebel <r.schwebel@domain.hid>
> 
> [1] http://spacegrant.colorado.edu/~dixonc/vxworks/docs/vxworks/ref/taskLib.html#taskActivate
> [2] http://www.xs4all.nl/~borkhuis/vxworks/vxw_pt6.html
> 
> ---
>  configure.in |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Merged, thanks.

> Index: xenomai-solo/configure.in
> ===================================================================
> --- xenomai-solo.orig/configure.in	2008-03-28 19:17:23.000000000 +0100
> +++ xenomai-solo/configure.in	2008-03-28 19:19:15.000000000 +0100
> @@ -163,7 +163,8 @@
>  dnl Exported CFLAGS and LDFLAGS
>  XENO_USER_APP_CFLAGS=$XENO_USER_CFLAGS
>  XENO_USER_APP_LDFLAGS=$XENO_USER_LDFLAGS
> -XENO_USER_CFLAGS="$XENO_USER_CFLAGS -Wall -pipe -fstrict-aliasing $gcc_w_noalias"
> +XENO_USER_CFLAGS="$XENO_USER_CFLAGS -Wall -Wsign-compare -Wfloat-equal -Wformat-security"
> +XENO_USER_CFLAGS="$XENO_USER_CFLAGS -pipe -fstrict-aliasing $gcc_w_noalias"
>  
>  if test x$debug_symbols = xy; then
>     XENO_USER_CFLAGS="-g -O0 $XENO_USER_CFLAGS -D__SOLO_DEBUG__"
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH][SOLO] add more warnings

[Philippe Gerum]

Robert Schwebel wrote:
> Contrary to what one would assume, -Wall doesn't warn on everything. It
> turned out to add a few more options, like -Wsign-compare, -Wfloat-equal
> and -Wformat-security, which is what the patch below does.
> 
> When running with these options, we get the following warnings:
> 

It should be better now.

-- 
Philippe.

Re: [Xenomai-core] [PATCH][SOLO] add more warnings

[Philippe Gerum]

Philippe Gerum wrote:
> Robert Schwebel wrote:

[snip]

>> I've looked into the first ones only yet and it looks to me like the
>> type definitions have to be reviewed carefully. For example, TASK_ID is
>> defined to be unsigned long, whereas the vxworks documentation [1] looks
>> more like if we need 'int' there. Which also makes me wonder if vxworks
>> has a special idea about what 'int' is;
> 
> VxWorks assumes 32bit and sizeof(void *) == sizeof(int), unfortunately. See
> taskSpawn() for instance.
>

Sorry: s,taskSpawn,taskInit,

-- 
Philippe.

Re: [Xenomai-core] [PATCH][SOLO] add more warnings

[Robert Schwebel]

On Fri, Mar 28, 2008 at 08:52:26PM +0100, Philippe Gerum wrote:
> > I've looked into the first ones only yet and it looks to me like the
> > type definitions have to be reviewed carefully. For example, TASK_ID is
> > defined to be unsigned long, whereas the vxworks documentation [1] looks
> > more like if we need 'int' there. Which also makes me wonder if vxworks
> > has a special idea about what 'int' is;
>
> VxWorks assumes 32bit and sizeof(void *) == sizeof(int), unfortunately. See
> taskSpawn() for instance.

Yes, which probably means that using 'int' and friends is not a good
idea.

> Did you mean stdint.h?

Probably better, yes.

> What we need is an integer type which is able to carry a pointer on
> 32/64bit platforms, so we should rather use intptr_t I guess, as per
> C99, which expands as a long type. Object ids as unsigned long is a
> left over from the co-kernel version, where we use actual integer
> handles returned from kernel space, and not pointers in disguise. Will
> fix, thanks.

Hmm, if vxworks assumes "int" to be 32 bit, couldn't it lead to problems
if the pointer size itself isn't 32 bit as well?

> Merged, thanks.

Doesn't show up on http://www.denx.de/cgi-bin/gitweb.cgi?p=xenomai-solo.git;a=summary

?

Robert
-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9

Re: [Xenomai-core] [PATCH][SOLO] add more warnings

[Wolfgang Denk]

In message <20080328204631.GC9009@domain.hid> you wrote:
>
> Doesn't show up on http://www.denx.de/cgi-bin/gitweb.cgi?p=xenomai-solo.git;a=summary

Please try again.

The cron job to push stuff to the public server runs not that often.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@domain.hid
Respect is a rational process
	-- McCoy, "The Galileo Seven", stardate 2822.3

Re: [Xenomai-core] [PATCH][SOLO] add more warnings

[Philippe Gerum]

Robert Schwebel wrote:
> On Fri, Mar 28, 2008 at 08:52:26PM +0100, Philippe Gerum wrote:
>>> I've looked into the first ones only yet and it looks to me like the
>>> type definitions have to be reviewed carefully. For example, TASK_ID is
>>> defined to be unsigned long, whereas the vxworks documentation [1] looks
>>> more like if we need 'int' there. Which also makes me wonder if vxworks
>>> has a special idea about what 'int' is;
>> VxWorks assumes 32bit and sizeof(void *) == sizeof(int), unfortunately. See
>> taskSpawn() for instance.
> 
> Yes, which probably means that using 'int' and friends is not a good
> idea.
> 
>> Did you mean stdint.h?
> 
> Probably better, yes.
> 
>> What we need is an integer type which is able to carry a pointer on
>> 32/64bit platforms, so we should rather use intptr_t I guess, as per
>> C99, which expands as a long type. Object ids as unsigned long is a
>> left over from the co-kernel version, where we use actual integer
>> handles returned from kernel space, and not pointers in disguise. Will
>> fix, thanks.
> 
> Hmm, if vxworks assumes "int" to be 32 bit, couldn't it lead to problems
> if the pointer size itself isn't 32 bit as well?
>

VxWorks only documents that one may assume TASK_ID == pointer to TCB, which
implies sizeof(int) == sizeof(struct WIND_TCB *) as per the taskInit
documentation and the original taskSpawn prototype. But using a different scalar
type than int to underlay TASK_ID, which would guarantee sizeof(TASK_ID) ==
sizeof(struct WIND_TCB *) is logically ok (and actually always worked so far in
actual ports of legacy apps over the VxWorks skin).

Said differently, any application assuming that a task identifier must be 32bit
wide would only work in a 32bit environment anyway, as implied by the initial
VxWorks assumption that sizeof(TASK_ID) == sizeof(struct WIND_TCB *). Given that
sizeof(intptr_t) is 32bit in such environment, we would be fine in this case as
well.

>> Merged, thanks.
> 
> Doesn't show up on http://www.denx.de/cgi-bin/gitweb.cgi?p=xenomai-solo.git;a=summary
> 

My GIT tree is mirrored by a cron job to the public repo. You will see the
changes after the next refresh has taken place.

> ?
> 
> Robert


-- 
Philippe.