[PATCH v2] or1k: Fix compiler warnings

[PATCH v2] or1k: Fix compiler warnings

[Stafford Horne]

In my build the below are treated as error now and causing failures.  I
have described the fixes of each warning below.

In newlib/libc/sys/or1k/mlock.c:

      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);
	  |                 ^~~~~~~~~~~~~~~~~

This patch adds prototypes for functions or1k_critical_begin and
or1k_critical_end to suppress the warning, inline with what we do for
or1k_sync_cas.

In libgloss/or1k/or1k_uart.c:

    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,
	  |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~

The public API is ‘void (*)(void *)' for our interrupt handlers.  The
function _or1k_uart_interrupt_hander is the internal default
implementation of the uart IRQ handler and it doesn't use the data
argument.

This patch updates the _or1k_uart_interrupt_handler argument type from
uint32_t to void* allowing the function prototype to match the required
prototype.

If we did have a 64-bit implementation it would be an ABI issue. But,
there never has been one, or1k is only 32-bit.

In libgloss/or1k/interrupts.c:

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

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.

In libgloss/or1k/sbrk.c:

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

This patch adds a cast, which is safe in or1k as the architecture in
32-bit only.  But this code would not be 64-compatible.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
Since v1:
 - Update the commit message to explain the reasoning behind each warning fix.

 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

Re: [PATCH v2] or1k: Fix compiler warnings

[Corinna Vinschen]

Hi Stafford,

I pushed your patch, thank you.

However, assuming that 64 bit *might* be supported in future, I can't
help noticing that or1k uses uint32_t as numerical replacement type for
pointers.

As an example, take sbrk.c:

On Dec 12 16:23, Stafford Horne wrote:
> In libgloss/or1k/sbrk.c:
> 
>     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;
> 	  |
> 
> This patch adds a cast, which is safe in or1k as the architecture in
> 32-bit only.  But this code would not be 64-compatible.
> [...]
> 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;

Just adding the cast silences the compiler, ok, but the question is, if
the code shouldn't use void * directly for actual pointer values, and
uintptr_t as numerical type.  Not only to future-proof for 64 bit, but
also for readability and correctness.

Also, even though all vars in the code are uint32_t anyway, the code
recasts them to uint32_t a lot, for instance, line 44:

    } while (or1k_sync_cas((void*) &_or1k_heap_end,
		    (uint32_t) prev_heap_end,
		    (uint32_t) (prev_heap_end + incr)) != (uint32_t) prev_heap_end);

So, still using sbrk.c as an example, what about this?

===== SNIP =====
extern void *end;
void *_or1k_heap_start = &end;
void *_or1k_heap_end;

void *
_sbrk_r (struct _reent * reent, ptrdiff_t incr)
{
	void *	prev_heap_end;

	// This needs to be atomic

	// Disable interrupts on this core
	uint32_t sr_iee = or1k_interrupts_disable();
	uint32_t sr_tee = or1k_timer_disable();

	// Initialize heap end to end if not initialized before
	or1k_sync_cas(&_or1k_heap_end, 0, (uintptr_t) _or1k_heap_start);

	do {
		// Read previous heap end
		prev_heap_end = _or1k_heap_end;
		// and try to set it to the new value as long as it has changed
	} while (or1k_sync_cas(&_or1k_heap_end,
			(uintptr_t) prev_heap_end,
			(uintptr_t) (prev_heap_end + incr))
		 != (uintptr_t) prev_heap_end);

	// Restore interrupts on this core
	or1k_timer_restore(sr_tee);
	or1k_interrupts_restore(sr_iee);

	return prev_heap_end;
}
===== SNAP =====

What do you think?

Thanks,
Corinna

Re: [PATCH v2] or1k: Fix compiler warnings

[Stafford Horne]

On Mon, Dec 16, 2024 at 11:12:48AM +0100, Corinna Vinschen wrote:
> Hi Stafford,
> 
> I pushed your patch, thank you.
> 
> However, assuming that 64 bit *might* be supported in future, I can't
> help noticing that or1k uses uint32_t as numerical replacement type for
> pointers.
> 
> As an example, take sbrk.c:
> 
> On Dec 12 16:23, Stafford Horne wrote:
> > In libgloss/or1k/sbrk.c:
> > 
> >     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;
> > 	  |
> > 
> > This patch adds a cast, which is safe in or1k as the architecture in
> > 32-bit only.  But this code would not be 64-compatible.
> > [...]
> > 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;
> 
> Just adding the cast silences the compiler, ok, but the question is, if
> the code shouldn't use void * directly for actual pointer values, and
> uintptr_t as numerical type.  Not only to future-proof for 64 bit, but
> also for readability and correctness.
> 
> Also, even though all vars in the code are uint32_t anyway, the code
> recasts them to uint32_t a lot, for instance, line 44:
> 
>     } while (or1k_sync_cas((void*) &_or1k_heap_end,
> 		    (uint32_t) prev_heap_end,
> 		    (uint32_t) (prev_heap_end + incr)) != (uint32_t) prev_heap_end);
> 
> So, still using sbrk.c as an example, what about this?

I agree 100%, I mentioned this in the commit about fixing compiler warnings. I
mention:

       23 | uint32_t _or1k_heap_start = &end;

    This patch adds a cast, which is safe in or1k as the architecture in
    32-bit only.  But this code would not be 64-compatible.

I think in general the code is full of issues using int32_t for pointer
arithmatic.  But it will take a big patch to fix everything.

> ===== SNIP =====
> extern void *end;
> void *_or1k_heap_start = &end;
> void *_or1k_heap_end;
> 
> void *
> _sbrk_r (struct _reent * reent, ptrdiff_t incr)
> {
> 	void *	prev_heap_end;
> 
> 	// This needs to be atomic
> 
> 	// Disable interrupts on this core
> 	uint32_t sr_iee = or1k_interrupts_disable();
> 	uint32_t sr_tee = or1k_timer_disable();
> 
> 	// Initialize heap end to end if not initialized before
> 	or1k_sync_cas(&_or1k_heap_end, 0, (uintptr_t) _or1k_heap_start);
> 
> 	do {
> 		// Read previous heap end
> 		prev_heap_end = _or1k_heap_end;
> 		// and try to set it to the new value as long as it has changed
> 	} while (or1k_sync_cas(&_or1k_heap_end,
> 			(uintptr_t) prev_heap_end,
> 			(uintptr_t) (prev_heap_end + incr))
> 		 != (uintptr_t) prev_heap_end);
> 
> 	// Restore interrupts on this core
> 	or1k_timer_restore(sr_tee);
> 	or1k_interrupts_restore(sr_iee);
> 
> 	return prev_heap_end;
> }
> ===== SNAP =====
> 
> What do you think?

Thanks, I think this is good, the public signature of the or1k_sync_cas function
is:

  uint32_t or1k_sync_cas(void *address, uint32_t compare, uint32_t swap)

So we may get warnings about the mismatch between uintptr_t and uint32_t.  The
function is implemented in assembly and the instructions it uses are strictly
32-bit even according to the proposted 64-bit spec. If we were using 64-bit
pointers we will need to have a 64-bit CAS operation.  The signature should be:

  unsigned long or1k_sync_cas(void *address,
                              unsigned long compare, unsigned long swap)

Is it desired to use uintptr_t everywhere instead of void*?

Either way I think your patch is in the correct direction.  Maybe will need to
keep the casts when passing to or1k_sync_cas for now.

Would you like me to test this out and send a patch?

I also looked around other bits and found:

  libgloss/or1k/board.h - uint32_t uses for addresses
  libgloss/or1k/include/or1k-support.h
   void or1k_icache_flush(uint32_t entry) - it 64-bit this should be 64-bit
   void or1k_dcache_flush(unsigned long entry) - actually ok if 64-bit,
                                                 incosistent
  void or1k_mtspr (uint32_t spr, uint32_t value)
  uint32_t or1k_mfspr (uint32_t spr)
    - these two are related the MT (move to) and MF (move from) special purpose
      registers should use 64-bit ints (unsigned long) on 64-bit
      implementations.
    * Note the spec mentions mtspr in 64-bit will only move 32-bits which is
      wrong and the spec needs fixing.

I think many of these are fixable, though the signatures of public headers will
change, the ABI will be compatible though.  What do you think?

-Stafford

Re: [PATCH v2] or1k: Fix compiler warnings

[Corinna Vinschen]

On Dec 16 12:57, Stafford Horne wrote:
> On Mon, Dec 16, 2024 at 11:12:48AM +0100, Corinna Vinschen wrote:
> > [...]
> > Just adding the cast silences the compiler, ok, but the question is, if
> > the code shouldn't use void * directly for actual pointer values, and
> > uintptr_t as numerical type.  Not only to future-proof for 64 bit, but
> > also for readability and correctness.
> > 
> > Also, even though all vars in the code are uint32_t anyway, the code
> > recasts them to uint32_t a lot, for instance, line 44:
> > 
> >     } while (or1k_sync_cas((void*) &_or1k_heap_end,
> > 		    (uint32_t) prev_heap_end,
> > 		    (uint32_t) (prev_heap_end + incr)) != (uint32_t) prev_heap_end);
> > 
> > So, still using sbrk.c as an example, what about this?
> 
> I agree 100%, I mentioned this in the commit about fixing compiler warnings. I
> mention:
> 
>        23 | uint32_t _or1k_heap_start = &end;
> 
>     This patch adds a cast, which is safe in or1k as the architecture in
>     32-bit only.  But this code would not be 64-compatible.
> 
> I think in general the code is full of issues using int32_t for pointer
> arithmatic.  But it will take a big patch to fix everything.
> 
> > ===== SNIP =====
> > [...]
> > ===== SNAP =====
> > 
> > What do you think?
> 
> Thanks, I think this is good, the public signature of the or1k_sync_cas function
> is:
> 
>   uint32_t or1k_sync_cas(void *address, uint32_t compare, uint32_t swap)
> 
> So we may get warnings about the mismatch between uintptr_t and uint32_t.  The
> function is implemented in assembly and the instructions it uses are strictly
> 32-bit even according to the proposted 64-bit spec. If we were using 64-bit
> pointers we will need to have a 64-bit CAS operation.  The signature should be:
> 
>   unsigned long or1k_sync_cas(void *address,
>                               unsigned long compare, unsigned long swap)
> 
> Is it desired to use uintptr_t everywhere instead of void*?
> 
> Either way I think your patch is in the correct direction.  Maybe will need to
> keep the casts when passing to or1k_sync_cas for now.
> 
> Would you like me to test this out and send a patch?
> 
> I also looked around other bits and found:
> 
>   libgloss/or1k/board.h - uint32_t uses for addresses
>   libgloss/or1k/include/or1k-support.h
>    void or1k_icache_flush(uint32_t entry) - it 64-bit this should be 64-bit
>    void or1k_dcache_flush(unsigned long entry) - actually ok if 64-bit,
>                                                  incosistent
>   void or1k_mtspr (uint32_t spr, uint32_t value)
>   uint32_t or1k_mfspr (uint32_t spr)
>     - these two are related the MT (move to) and MF (move from) special purpose
>       registers should use 64-bit ints (unsigned long) on 64-bit
>       implementations.
>     * Note the spec mentions mtspr in 64-bit will only move 32-bits which is
>       wrong and the spec needs fixing.
> 
> I think many of these are fixable, though the signatures of public headers will
> change, the ABI will be compatible though.  What do you think?

I just wanted to point it out, it's entirely your call if you want to
change this.  After all, it works, so there's no pressure.


Corinna

Re: [PATCH v2] or1k: Fix compiler warnings

[Stafford Horne]

On Mon, Dec 16, 2024 at 04:48:44PM +0100, Corinna Vinschen wrote:
> On Dec 16 12:57, Stafford Horne wrote:
> > On Mon, Dec 16, 2024 at 11:12:48AM +0100, Corinna Vinschen wrote:
> > > [...]
> > > Just adding the cast silences the compiler, ok, but the question is, if
> > > the code shouldn't use void * directly for actual pointer values, and
> > > uintptr_t as numerical type.  Not only to future-proof for 64 bit, but
> > > also for readability and correctness.
> > > 
> > > Also, even though all vars in the code are uint32_t anyway, the code
> > > recasts them to uint32_t a lot, for instance, line 44:
> > > 
> > >     } while (or1k_sync_cas((void*) &_or1k_heap_end,
> > > 		    (uint32_t) prev_heap_end,
> > > 		    (uint32_t) (prev_heap_end + incr)) != (uint32_t) prev_heap_end);
> > > 
> > > So, still using sbrk.c as an example, what about this?
> > 
> > I agree 100%, I mentioned this in the commit about fixing compiler warnings. I
> > mention:
> > 
> >        23 | uint32_t _or1k_heap_start = &end;
> > 
> >     This patch adds a cast, which is safe in or1k as the architecture in
> >     32-bit only.  But this code would not be 64-compatible.
> > 
> > I think in general the code is full of issues using int32_t for pointer
> > arithmatic.  But it will take a big patch to fix everything.
> > 
> > > ===== SNIP =====
> > > [...]
> > > ===== SNAP =====
> > > 
> > > What do you think?
> > 
> > Thanks, I think this is good, the public signature of the or1k_sync_cas function
> > is:
> > 
> >   uint32_t or1k_sync_cas(void *address, uint32_t compare, uint32_t swap)
> > 
> > So we may get warnings about the mismatch between uintptr_t and uint32_t.  The
> > function is implemented in assembly and the instructions it uses are strictly
> > 32-bit even according to the proposted 64-bit spec. If we were using 64-bit
> > pointers we will need to have a 64-bit CAS operation.  The signature should be:
> > 
> >   unsigned long or1k_sync_cas(void *address,
> >                               unsigned long compare, unsigned long swap)
> > 
> > Is it desired to use uintptr_t everywhere instead of void*?
> > 
> > Either way I think your patch is in the correct direction.  Maybe will need to
> > keep the casts when passing to or1k_sync_cas for now.
> > 
> > Would you like me to test this out and send a patch?
> > 
> > I also looked around other bits and found:
> > 
> >   libgloss/or1k/board.h - uint32_t uses for addresses
> >   libgloss/or1k/include/or1k-support.h
> >    void or1k_icache_flush(uint32_t entry) - it 64-bit this should be 64-bit
> >    void or1k_dcache_flush(unsigned long entry) - actually ok if 64-bit,
> >                                                  incosistent
> >   void or1k_mtspr (uint32_t spr, uint32_t value)
> >   uint32_t or1k_mfspr (uint32_t spr)
> >     - these two are related the MT (move to) and MF (move from) special purpose
> >       registers should use 64-bit ints (unsigned long) on 64-bit
> >       implementations.
> >     * Note the spec mentions mtspr in 64-bit will only move 32-bits which is
> >       wrong and the spec needs fixing.
> > 
> > I think many of these are fixable, though the signatures of public headers will
> > change, the ABI will be compatible though.  What do you think?
> 
> I just wanted to point it out, it's entirely your call if you want to
> change this.  After all, it works, so there's no pressure.

Thank you,

It's good that you know about it now. It will make subitting patches to fix it
easier to review.  I will try to clean it up and send some patches soon.

-Stafford