or1k: Fix compiler warnings

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

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?


Corinna



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;
>  #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
>   */
> -void _or1k_uart_interrupt_handler(uint32_t data);
> +void _or1k_uart_interrupt_handler(void *data);
>  
>  /**
>   * 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;
>  
>  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);
> +
>  /**
>   * Recursive lock of the malloc
>   */
> -- 
> 2.47.0
  

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
  

Hi Stafford,

On Dec 11 14:39, Stafford Horne wrote:
> 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:
> [...]
> 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.

IIUC or1k is designed with 32 bit and 64 bit implementations in mind.
However, if there's no existing 64 bit toolchain for or1k yet, my
concern doesn't really matter.  Even better if the ABI is 64 bit clean
before such toolchain is created.

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

Sounds good to me.  Just go ahead with a v2 as per your suggestion.


Thanks,
Corinna
  

On Wed, Dec 11, 2024 at 11:40:06PM +0100, Corinna Vinschen wrote:
> Hi Stafford,
> 
> On Dec 11 14:39, Stafford Horne wrote:
> > 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:
> > [...]
> > 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.
> 
> IIUC or1k is designed with 32 bit and 64 bit implementations in mind.

Right, the architecture spec was expanded to include 64-bit but that has never
taken off.

> However, if there's no existing 64 bit toolchain for or1k yet, my
> concern doesn't really matter.  Even better if the ABI is 64 bit clean
> before such toolchain is created.
> 
> > 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?
> 
> Sounds good to me.  Just go ahead with a v2 as per your suggestion.

OK, thanks

-Stafford