Re: [PATCH v2] or1k: Fix compiler warnings

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