[PATCH v3] score: PR1607: Add and use CPU_SIZEOF_POINTER

[Ralf Corsepius]

On 11/08/2012 11:43 AM, Sebastian Huber wrote:
> On 11/08/2012 11:25 AM, Ralf Corsepius wrote:
>> On 11/08/2012 10:52 AM, Sebastian Huber wrote:
>>> On 11/08/2012 10:33 AM, Ralf Corsepius wrote:
>>>> On 11/08/2012 09:27 AM, Sebastian Huber wrote:
>>>>> Add and use new CPU port define CPU_SIZEOF_POINTER.  It must be an
>>>>> integer literal that can be used by the assembler.  This value will be
>>>>> used to calculate offsets of structure members.  These offsets will be
>>>>> used in assembler code.
>>>>>
>>>>> The size of a pointer is part of the application binary interface
>>>>> (ABI)
>>>>> and thus independent of the actual programming language.  The compiler
>>>>> will provide defines to determine the current ABI.  We use these
>>>>> defines
>>>>> to select the appropriate CPU_SIZEOF_POINTER value.
>>>>>
>>>>> Static assertions in the new file "cpukit/score/src/percpuasm.c" will
>>>>> ensure that the value of CPU_SIZEOF_POINTER is consistent with the
>>>>> current compiler settings.  Also the offset values used by assembler
>>>>> code are verfied.
>>>>
>>>> Again, this approach lacks generality and is fundametally flawed.
>>>
>>> Please be more specific.  I don't understand what you mean.
>>
>> The size of a pointers is not a per-cpu constant. It depends upon various
>> compilation parameters/compiler arguments and can also vary between
>> compiler
>> versions and compilers.
>>
>> => Any hardcoded assumption on pointer sizes is wrong.
>
> This is not a per CPU constant!  It is a per ABI constant.

You are nit picking at words. We are looking at the same topic from 
different angles.

>> => It cannot be encoded into _any_ general or per-cpu installed
>> header. It can
>> only be encoded into a per-BSP header.
>
> We already use the same mechanism e.g. on PowerPC to select the CPU
> context for SPE or AltiVec and also for soft-float vs. hard-float.

I don't know about SPE/AlitVec, but for soft/hard-float, this should not 
be the case.

These are supposed to be detected from preprocessor defines. If this 
doesn't apply anymore, RTEMS has regressed.

>>
>>>
>>>>
>>>> It's just a random accident it appears to work for some targets.
>>>
>>> Please provide a counter example.
>>>
>>
>> In general, any target can exhibit changes to sizeof pointers.
>>
>> Here are j3 real world examples:
>>
>> # find -name config.h -exec grep -H '^#.*SIZEOF_VOID' {} \;
>> ./h8300-rtems4.11/config.h:#define SIZEOF_VOIDP 2
>> ./h8300-rtems4.11/h8300s/config.h:#define SIZEOF_VOIDP 4
>> ./h8300-rtems4.11/h8300s/int32/config.h:#define SIZEOF_VOIDP 4
>> ./h8300-rtems4.11/h8sx/config.h:#define SIZEOF_VOIDP 4
>> ./h8300-rtems4.11/h8sx/int32/config.h:#define SIZEOF_VOIDP 4
>> ./h8300-rtems4.11/h8300h/config.h:#define SIZEOF_VOIDP 4
>> ./h8300-rtems4.11/h8300h/int32/config.h:#define SIZEOF_VOIDP 4
>> ./m32c-rtems4.11/config.h:#define SIZEOF_VOIDP 2
>> ./m32c-rtems4.11/m32cm/config.h:#define SIZEOF_VOIDP 4
>
> Yes, but have a look at the patch:
>
> [...]
> diff --git a/cpukit/score/cpu/h8300/rtems/score/cpu.h
> b/cpukit/score/cpu/h8300/rtems/score/cpu.h
> index ea9d443..1811ce7 100644
> --- a/cpukit/score/cpu/h8300/rtems/score/cpu.h
> +++ b/cpukit/score/cpu/h8300/rtems/score/cpu.h
> @@ -497,6 +497,12 @@ SCORE_EXTERN Context_Control_fp  _CPU_Null_fp_context;
>
>   #define CPU_STACK_MINIMUM_SIZE          (1536)
>
> +#if defined(__H8300H__) || defined(__H8300S__) || defined(__H8300SX__)
> +  #define CPU_SIZEOF_POINTER 4
> +#else
> +  #define CPU_SIZEOF_POINTER 2
> +#endif
> +
>   /*
>    *  CPU's worst alignment requirement for data types on a byte
> boundary.  This
>    *  alignment does not take into account the requirements for the stack.
> [...]
> diff --git a/cpukit/score/cpu/m32c/rtems/score/cpu.h
> b/cpukit/score/cpu/m32c/rtems/score/cpu.h
> index c1d1db8..d83e93d 100644
> --- a/cpukit/score/cpu/m32c/rtems/score/cpu.h
> +++ b/cpukit/score/cpu/m32c/rtems/score/cpu.h
> @@ -575,6 +575,12 @@ typedef struct {
>    */
>   #define CPU_STACK_MINIMUM_SIZE          (2048L)
>
> +#ifdef __m32cm_cpu__
> +  #define CPU_SIZEOF_POINTER 4
> +#else
> +  #define CPU_SIZEOF_POINTER 2
> +#endif

Hard-coding what could be automatically detected rsp. is compiler 
defined - A truely silly approach.

In longer terms this hard-coding is error-prone and unmaintainable. In 
fact you are duplicating and hard-coding what already is specified 
elsewhere.

>   /**
>    *  CPU's worst alignment requirement for data types on a byte
> boundary.  This
>    *  alignment does not take into account the requirements for the stack.
> [...]
>
>> ./x86_64-redhat-linux/config.h:#define SIZEOF_VOIDP 8
>> ./x86_64-redhat-linux/32/config.h:#define SIZEOF_VOIDP 4
>
> Yes, exactly a 32-bit ABI and a 64-bit ABI.
Again, you are nit-picking on words.

On the user side, this is all about sizes of types. ABIs are _completely 
irrelevant_.

>> One way to avoid the whole problem would be to not use asm but to use
>> c-inline-asm routines, to which you can pass c-constants.
>
> Who has the resources to do this?
In C you can use "sizeof(void)" and thus do not have to care about 
hard-code constants.

> Also I doubt that it is a good idea
> to replace the well tested existing assembler code with inline asm which
> is highly compiler specific and error prone.
Rubbish. It's trivial to verify such inline-asm code.

>> Another, RTEMS specific way would be to move all such constants and
>> routines
>> outside of cpukit.
>
> Why?  If you link a cpukit library with pointer size == 2 and a BSP
> library with pointer size == 4, then I don't think this works.
If properly written, such configuration will fail to build much earlier 
at least flood you with warnings. If not, compilation will succeed silently.

Ralf