Re: [PATCH v2] or1k: Fix compiler warnings

[Stafford Horne <shorne@gmail.com>]

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