[PATCH] or1k: Fix compiler warnings
Stafford Horne
shorne@gmail.com
Wed Dec 11 14:39:22 GMT 2024
On Wed, Dec 11, 2024 at 02:10:59PM +0100, Corinna Vinschen wrote:
> Hi Stafford,
>
> I'm not engaged in this stuff, so maybe this is dumb...
>
> Changing the parameter from uint32_t to void* sounds like it would have
> been the right thing from the start, but it's also an ABI change on 64
> bit or1k. I could imagine the uint32_t is just a pointer value from the
> lower 4G space on 64 bit. Do we even support 64 bit or1k?
Hi Corinna,
I think it's a good conncern. I put some comments on the patch below with my
thoughts.
If this is OK, I will send a v2 with a better description of each change.
See below:
>
> On Dec 10 12:58, Stafford Horne wrote:
> > In my build the below are treated as error now and causing failures.
> >
> > CC libc/sys/or1k/libc_a-mlock.o
> > newlib/libc/sys/or1k/mlock.c: In function ‘__malloc_lock’:
> > newlib/libc/sys/or1k/mlock.c:56:19: warning: implicit declaration of function ‘or1k_critical_begin’ [-Wimplicit-function-declaration]
> > 56 | restore = or1k_critical_begin();
> > | ^~~~~~~~~~~~~~~~~~~
> > newlib/libc/sys/or1k/mlock.c: In function ‘__malloc_unlock’:
> > newlib/libc/sys/or1k/mlock.c:93:17: warning: implicit declaration of function ‘or1k_critical_end’ [-Wimplicit-function-declaration]
> > 93 | or1k_critical_end(restore);
> > | ^~~~~~~~~~~~~~~~~
> >
> > libgloss/or1k/or1k_uart.c: In function ‘or1k_uart_set_read_cb’:
> > libgloss/or1k/or1k_uart.c:163:25: warning: passing argument 2 of ‘or1k_interrupt_handler_add’ from incompatible pointer type [-Wincompatible-pointer-types]
> > 163 | _or1k_uart_interrupt_handler, 0);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > | |
> > | void (*)(uint32_t) {aka void (*)(long unsigned int)}
> > In file included from libgloss/or1k/or1k_uart.c:19:
> > libgloss/or1k/include/or1k-support.h:97:45: note: expected ‘or1k_interrupt_handler_fptr’ {aka ‘void (*)(void *)’} but argument is of type ‘void (*)(uint32_t)’ {aka ‘void (*)(long unsigned int)’}
> > 97 | or1k_interrupt_handler_fptr handler,
> > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> >
> > libgloss/or1k/interrupts.c: In function ‘or1k_interrupt_handler_add’:
> > libgloss/or1k/interrupts.c:41:52: warning: assignment to ‘void *’ from ‘long unsigned int’ makes pointer from integer without a cast [-Wint-conversion]
> > 41 | _or1k_interrupt_handler_data_ptr_table[id] = (uint32_t) data_ptr;
> > | ^
> >
> > libgloss/or1k/sbrk.c:23:29: warning: initialization of ‘uint32_t’ {aka ‘long unsigned int’} from ‘uint32_t *’ {aka ‘long unsigned int *’} makes integer from pointer without a cast [-Wint-conversion]
> > 23 | uint32_t _or1k_heap_start = &end;
> > |
> >
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> > libgloss/or1k/interrupts.c | 4 ++--
> > libgloss/or1k/or1k_uart.c | 2 +-
> > libgloss/or1k/or1k_uart.h | 2 +-
> > libgloss/or1k/sbrk.c | 2 +-
> > newlib/libc/sys/or1k/mlock.c | 3 +++
> > 5 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/libgloss/or1k/interrupts.c b/libgloss/or1k/interrupts.c
> > index 6badc497c..516d74be3 100644
> > --- a/libgloss/or1k/interrupts.c
> > +++ b/libgloss/or1k/interrupts.c
> > @@ -35,10 +35,10 @@ void or1k_interrupt_handler_add(uint32_t id,
> > {
> > #ifdef __OR1K_MULTICORE__
> > _or1k_interrupt_handler_table[or1k_coreid()][id] = handler;
> > - _or1k_interrupt_handler_data_ptr_table[or1k_coreid()][id] = (uint32_t) data_ptr;
> > + _or1k_interrupt_handler_data_ptr_table[or1k_coreid()][id] = data_ptr;
> > #else
> > _or1k_interrupt_handler_table[id] = handler;
> > - _or1k_interrupt_handler_data_ptr_table[id] = (uint32_t) data_ptr;
> > + _or1k_interrupt_handler_data_ptr_table[id] = data_ptr;
Fix for:
- warning: assignment to ‘void *’ from ‘long unsigned int’ makes pointer from integer without a cast [-Wint-conversion]
The table _or1k_interrupt_handler_data_ptr_table is an array of void* and
data_ptr is void*. There is no need for the cast so remove it.
> > #endif
> > }
> >
> > diff --git a/libgloss/or1k/or1k_uart.c b/libgloss/or1k/or1k_uart.c
> > index 0a991e6ba..1391d565c 100644
> > --- a/libgloss/or1k/or1k_uart.c
> > +++ b/libgloss/or1k/or1k_uart.c
> > @@ -90,7 +90,7 @@ void (*_or1k_uart_read_cb)(char c);
> > * This is the interrupt handler that is registered for the callback
> > * function.
> > */
> > -void _or1k_uart_interrupt_handler(uint32_t data)
> > +void _or1k_uart_interrupt_handler(void *data)
> > {
> > uint8_t iir = REG8(IIR);
> >
> > diff --git a/libgloss/or1k/or1k_uart.h b/libgloss/or1k/or1k_uart.h
> > index 4cbb68350..201b7749f 100644
> > --- a/libgloss/or1k/or1k_uart.h
> > +++ b/libgloss/or1k/or1k_uart.h
> > @@ -30,7 +30,7 @@ extern void (*_or1k_uart_read_cb)(char c);
> > /**
> > * The UART interrupt handler
> > */x
> > -void _or1k_uart_interrupt_handler(uint32_t data);
> > +void _or1k_uart_interrupt_handler(void *data);
These two are fixes for:
- ‘or1k_interrupt_handler_fptr’ {aka ‘void (*)(void *)’} but argument is of type ‘void (*)(uint32_t)’ {aka ‘void (*)(long unsigned int)’}
The public API is ‘void (*)(void *)' for our interrupt handlers. The symbol
_or1k_uart_interrupt_hander is the internal default implementation of the uart
IRQ handler and it doen't use the data argument. _There already is an ABI
mismatch_ and when the handler is called in libgloss it will pass a void *.
void or1k_interrupt_handler_add(uint32_t line,
or1k_interrupt_handler_fptr handler,
void* data);
I agree that if we did have a 64-bit implementation it may be an issue. But
there never has been one, or1k is only 32-bit. I think the ABI does not change
with this patch.
Maybe I am wrong, but I think this is safe because:
- There is no ABI change as void* and uint32_t are the same in or1k
- There is no API change as or1k_uart.h is not a public API
What do you think?
> > /**
> > * Initialize UART
> > diff --git a/libgloss/or1k/sbrk.c b/libgloss/or1k/sbrk.c
> > index 0c3e66e87..ca196d228 100644
> > --- a/libgloss/or1k/sbrk.c
> > +++ b/libgloss/or1k/sbrk.c
> > @@ -20,7 +20,7 @@
> > #include "include/or1k-support.h"
> >
> > extern uint32_t end; /* Set by linker. */
> > -uint32_t _or1k_heap_start = &end;
> > +uint32_t _or1k_heap_start = (uint32_t) &end;
> > uint32_t _or1k_heap_end;
Fix for:
- warning: initialization of ‘uint32_t’ {aka ‘long unsigned int’} from ‘uint32_t *’ {aka ‘long unsigned int *’} makes integer from pointer without a cast [-Wint-conversion]
Add a cast, which is safe in or1k as the architecture in 32-bit only. But this
code would not be 64-compatible.
I could try to convert these _start, _end symbols all to void*. But I will
leave it as is for now.
> > void *
> > diff --git a/newlib/libc/sys/or1k/mlock.c b/newlib/libc/sys/or1k/mlock.c
> > index ccb840161..a0c038335 100644
> > --- a/newlib/libc/sys/or1k/mlock.c
> > +++ b/newlib/libc/sys/or1k/mlock.c
> > @@ -38,6 +38,9 @@ volatile uint32_t _or1k_malloc_lock_restore;
> >
> > extern uint32_t or1k_sync_cas(void *address, uint32_t compare, uint32_t swap);
> >
> > +extern uint32_t or1k_critical_begin();
> > +extern void or1k_critical_end(uint32_t restore);
> > +
Fixes for:
- warning: implicit declaration of function ‘or1k_critical_begin’ [-Wimplicit-function-declaration]
- warning: implicit declaration of function ‘or1k_critical_end’ [-Wimplicit-function-declaration]
Defining these as extern is inline with what we do for or1k_sync_cas.
-Stafford
More information about the Newlib
mailing list