#2128 closed defect (fixed)

[Patch] io.h: Use uint32_t and co. instead of "unsigned int"

Reported by: Nick Withers Owned by: Joel Sherrill
Priority: highest Milestone: 4.11
Component: score Version: 4.11
Severity: trivial Keywords:
Cc: sebastian.huber@…, nick.withers@… Blocked By:
Blocking:

Description (last modified by Joel Sherrill)

In at least the mvme3100's libcpu/io.h, inputs and outputs for in_be16() and co. are declared as unsigned ints, shorts and so forth.

This can lead to spurious warnings during application compile if the same types are not used (and they shouldn't be, IMHO); e.g.:

powerpc-rtems4.11-gcc --pipe -std=c99 -g -Wall -mcpu=powerpc -msoft-float -Dppc_generic -I. -I/home/nick/rtems/rtems-4.11/powerpc-rtems4.11/mvme3100/lib/include -I/home/nick/rtems/RTEMS_gdb_stub_1_5 -O0 -DDEBUG=1 -DDEBUG_GDB_STUB=1 -DDEBUG_INIT_DONE_RESET=0 -DDEBUG_HALT_ON_FATAL=0 -c -o modules/v785.o modules/v785.c
modules/v785.c: In function 'v785_read_data':
modules/v785.c:200:2: warning: passing argument 1 of 'in_be32' from incompatible pointer type [enabled by default]

if (((output_buffer_word = in_be32((volatile uint32_t *) ((uintptr_t) (card->a24_addr ? card->a24_addr : card->a16_addr) + (uintptr_t) v785_addr_output_buffer))) | v785_output_buffer_header_type_mask) == v785_output_buffer_header_type_invalid)

In file included from modules/v785.c:20:0:
/home/nick/rtems/rtems-4.11/powerpc-rtems4.11/mvme3100/lib/include/libcpu/io.h:118:24: note: expected 'volatile unsigned int *' but argument is of type 'volatile uint32_t *'

static inline unsigned in_be32(volatile unsigned *addr)



This patch (only touching the MVME3100's io.h) changes the parameter types to uint8_t (or similar) and return types to uint_least8_t (or similar):

diff --git a/c/src/lib/libcpu/powerpc/shared/include/io.h b/c/src/lib/libcpu/powerpc/shared/include/io.h
index ebc405d..9deb61d 100644
--- a/c/src/lib/libcpu/powerpc/shared/include/io.h
+++ b/c/src/lib/libcpu/powerpc/shared/include/io.h
@@ -31,18 +31,20 @@

#include <bsp.h> /* for _IO_BASE & friends */


+#include <stdint.h> /* for uint8_t & friends */
+

/* NOTE: The use of these macros is DISCOURAGED.

  • you should consider e.g. using in_xxx / out_xxx
  • with a device specific base address that is
  • defined by the BSP. This makes drivers easier
  • to port. */

-#define inb(port) in_8((unsigned char *)((port)+_IO_BASE))
-#define outb(val, port) out_8((unsigned char *)((port)+_IO_BASE), (val))
-#define inw(port) in_le16((unsigned short *)((port)+_IO_BASE))
-#define outw(val, port) out_le16((unsigned short *)((port)+_IO_BASE), (val))
-#define inl(port) in_le32((unsigned *)((port)+_IO_BASE))
-#define outl(val, port) out_le32((unsigned *)((port)+_IO_BASE), (val))
+#define inb(port) in_8((uint8_t *)((port)+_IO_BASE))
+#define outb(val, port) out_8((uint8_t *)((port)+_IO_BASE), (val))
+#define inw(port) in_le16((uint16_t *)((port)+_IO_BASE))
+#define outw(val, port) out_le16((uint16_t *)((port)+_IO_BASE), (val))
+#define inl(port) in_le32((uint32_t *)((port)+_IO_BASE))
+#define outl(val, port) out_le32((uint32_t *)((port)+_IO_BASE), (val))

/*

  • Enforce In-order Execution of I/O:

@@ -65,7 +67,7 @@ static inline void eieio(void)

/*

  • 8, 16 and 32 bit, big and little endian I/O operations, with barrier. */

-static inline int in_8(volatile unsigned char *addr)
+static inline uint_least8_t in_8(volatile uint8_t *addr)

{

int ret;


@@ -73,12 +75,12 @@ static inline int in_8(volatile unsigned char *addr)

return ret;

}


-static inline void out_8(volatile unsigned char *addr, int val)
+static inline void out_8(volatile uint8_t *addr, uint_least8_t val)

{

asm volatile("stb%U0%X0 %1,%0; eieio" : "=m" (*addr) : "r" (val));

}


-static inline int in_le16(volatile unsigned short *addr)
+static inline uint_least16_t in_le16(volatile uint16_t *addr)

{

int ret;


@@ -87,7 +89,7 @@ static inline int in_le16(volatile unsigned short *addr)

return ret;

}


-static inline int in_be16(volatile unsigned short *addr)
+static inline uint_least16_t in_be16(volatile uint16_t *addr)

{

int ret;


@@ -95,18 +97,18 @@ static inline int in_be16(volatile unsigned short *addr)

return ret;

}


-static inline void out_le16(volatile unsigned short *addr, int val)
+static inline void out_le16(volatile uint16_t *addr, uint_least16_t val)

{

asm volatile("sthbrx %1,0,%2; eieio" : "=m" (*addr) :

"r" (val), "r" (addr));

}


-static inline void out_be16(volatile unsigned short *addr, int val)
+static inline void out_be16(volatile uint16_t *addr, uint_least16_t val)

{

asm volatile("sth%U0%X0 %1,%0; eieio" : "=m" (*addr) : "r" (val));

}


-static inline unsigned in_le32(volatile unsigned *addr)
+static inline uint_least32_t in_le32(volatile uint32_t *addr)

{

unsigned ret;


@@ -115,7 +117,7 @@ static inline unsigned in_le32(volatile unsigned *addr)

return ret;

}


-static inline unsigned in_be32(volatile unsigned *addr)
+static inline uint_least32_t in_be32(volatile uint32_t *addr)

{

unsigned ret;


@@ -123,13 +125,13 @@ static inline unsigned in_be32(volatile unsigned *addr)

return ret;

}


-static inline void out_le32(volatile unsigned *addr, int val)
+static inline void out_le32(volatile uint32_t *addr, uint_least32_t val)

{

asm volatile("stwbrx %1,0,%2; eieio" : "=m" (*addr) :

"r" (val), "r" (addr));

}


-static inline void out_be32(volatile unsigned *addr, int val)
+static inline void out_be32(volatile uint32_t *addr, uint_least32_t val)

{

asm volatile("stw%U0%X0 %1,%0; eieio" : "=m" (*addr) : "r" (val));

}


Attachments (1)

io.h-C99.patch (49.1 KB) - added by Nick Withers on 12/01/14 at 06:15:10.

Download all attachments as: .zip

Change History (13)

comment:1 Changed on 07/16/13 at 11:50:23 by Sebastian Huber

Cc: Sebastian Huber added

comment:2 Changed on 07/16/13 at 23:18:28 by Nick Withers

Replying to comment:1:

What is the reason for using the least types?

No particularly good reason, I just thought it might be "more proper" :-)

comment:3 Changed on 08/09/13 at 04:23:55 by Nick Withers

Cc: Nick Withers added

comment:4 Changed on 11/23/14 at 00:16:37 by Joel Sherrill

Description: modified (diff)

Nick... is this still an issue? I don't think the mvme3100 has any warnings when building so I am prone to say you should stick to the types the method requires.

We are trying to close tickets so close it or let's discuss this.

Thanks.

comment:5 Changed on 11/24/14 at 00:36:47 by Nick Withers

Hi Joel, thanks for taking the time!

The argument here is philosophical, I suppose. The methods in question operate on fixed-width data types by their nature, so fixed width data type parameters and returns would seem appropriate to me.

That said, these methods are PowerPC-specific where presumably for RTEMS-supported BSPs unsigned int is always uint32_t, unsigned short uint16_t, etc. and so there isn't really a problem, strictly speaking. I'd argue they should, if present at all, be more generic and therefore would have to use the fixed-width types or be macros, but that's another debate.

comment:6 Changed on 11/24/14 at 15:34:04 by Gedare Bloom

I'm in Nick's boat here. We could conceivably define similar macros on other arch's and share the code, which is much easier with fixed-width sizes.

comment:7 Changed on 11/24/14 at 18:58:28 by Gedare Bloom

Version: HEAD4.11

Replace Version=HEAD with Version=4.11 for the tickets with Milestone >= 4.11

comment:8 Changed on 11/24/14 at 19:22:09 by Joel Sherrill

I am not opposed to the change if exact types are used. If you can propose a patch for exact types, test on the platforms you have, and help ensure there are no warnings, I am happy to see the patch. This is just not likely to get done otherwise. And I can see it introducing warnings.

comment:9 Changed on 11/24/14 at 22:39:06 by Nick Withers

Righto - I'll try to have a look at it this week.

Sorry I haven't been much help with warnings in general, sadly I've been occupied on non-RTEMS/C-side things!

Changed on 12/01/14 at 06:15:10 by Nick Withers

Attachment: io.h-C99.patch added

comment:10 Changed on 12/01/14 at 06:21:12 by Nick Withers

New patch attached, tested with:

  • MVME3100 BSP and my code, which "seems to work" - it doesn't itself use in_8() or out_8(), though. For what it's worth, I've actually had my io.h (and just it) patched with the fixed-width types since the ticket was opened
  • gmake all (configured with --enable-rtemsbsp="mvme3100 mvme5500 beatnik" --enable-tests=yes), redirecting stdout and stderr and comparing before and after - there are no new warnings

Couple of notes:

  • I'm not totally sure that passing in a variable of less width than a CPU register to an asm statement is kosher, e.g.:
    -	int ret;
    +	uint8_t ret;
    
    	__asm__ __volatile__("lbz%U1%X1 %0,%1; eieio" : "=r" (ret) : "m" (*addr));
    	return ret;
    
  • I only added the stdint.h header (or inttypes.h if the PRIx macros were used) to files which didn't already use the C99 fixed-width types. Those that were already missing it still are

comment:11 Changed on 12/19/14 at 05:06:18 by Gedare Bloom

Priority: normalhighest

Bump priority to highest for tickets with a fix attached or seemingly simple fix proposed in the description or comments.

comment:12 Changed on 12/24/14 at 03:40:55 by Nick Withers <nick.withers@…>

Resolution: fixed
Status: newclosed

In 2d5c48691453a05ffb3a264f75e71490166f819a/rtems:

Use fixed-width C99 types for PowerPC in_be16() and co.

Also use the const qualifier on the address pointer's target in in_*()

Closes #2128

Note: See TracTickets for help on using tickets.