Re: [PATCH v2] or1k: Fix compiler warnings

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