Re: [PATCH v2] or1k: Fix compiler warnings

[Stafford Horne <shorne@gmail.com>]

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