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

[Sebastian Huber]

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.

>
> => 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.

>
>>
>>>
>>> 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
+
  /**
   *  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.

>
>
>> Independent of potential problems with this patch, I think it is better than
>> the current state.
>>
> I could not disagree more. All your patch does is to replace one broken and
> flawed approach which managed to furtherly infect RTEMS with another, similarly
> broken approach.
>
> 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?  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.

>
> 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.

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.