Re: [Xen-devel] [PATCH for-4.6] xen/public: arm: Use __typeof__ rather than typeof

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 23/10/15 15:55, Ian Campbell wrote:
> On Fri, 2015-10-23 at 15:44 +0100, Julien Grall wrote:
>> On 23/10/15 15:37, Jan Beulich wrote:
>>>>>> On 23.10.15 at 16:31, <ian.campbell@xxxxxxxxxx> wrote:
>>>> On Fri, 2015-10-23 at 08:16 -0600, Jan Beulich wrote:
>>>>> No, the validating script is a nice-to-have, but nothing more. What
>>>>> I was referring to was a patch to drop the use of this gcc
>>>>> extension.
>>>>
>>>> Then I'm confused. This patch turns a typeof into a __typeof__. In <
>>>> 56126D8702000078000A80AC@xxxxxxxxxxxxxxxxxxxxxxx> you said "typeof()
>>>> is a
>>>> gcc extension".
>>>>
>>>> Are you now saying that __typeof__ also a gcc extension too?
>>>>
>>>> I was under the impression that __typeof__ was standard (by some cxx
>>>> at
>>>> least) and your mail reinforced that (possibly wrong) impression.
>>>
>>> There's no typeof or __typeof__ in C11 or any earlier standard.
>>> I'm sorry if earlier replies of mine gave a different impression.
>>>
>>>> https://gcc.gnu.org/onlinedocs/gcc/Typeof.html also says that "If you
>>>> are
>>>> writing a header file that must work when included in ISO C programs,
>>>> write
>>>> __typeof__ instead of typeof", which also lead me to believe
>>>> __typeof__ was
>>>> OK from this PoV.
>>>
>>> That's solely to prevent name space issues - __typeof__ is a
>>> reserved name, while typeof isn't.
>>
>> Thank you for the explanation. I think we can do the same as x86 does
>> i.e:
>>
>> #define set_xen_guest_handle_raw(hnd, val)                  \
>>     do { if ( sizeof(hnd) == 8 ) *(uint64_t *)&(hnd) = 0;   \
>>          (hnd).p = val;                                     \
>>     } while ( 0 )
> 
> This evaluates hnd twice, which I assumed we wanted to avoid.
> 
> But if that is OK for x86 in this situation then there is no harm doing it
> on ARM too[0].
> 
> But in that case I think we would just do
>       (hnd).q = val ; (hnd).p = val
> rather than messing with &, casts and *.

Which is, based on the ISO C spec [1], unspecified. See 6.2.6.1#7:

"When a value is stored in a member of an object of union type, the
bytes of the object representation that do not correspond to that member
but do correspond to other members take unspecified values, but the
value of the union object shall not thereby become a trap
representation."

I've been looking to the solution suggested by Ian Jackson on IRC
friday. I.e smth like:

typedef union {
 uint64_t actual;
 type *for_check }




#define set_xen_guest_handle_raw(hnd, val)      \
  do {                                          \
        hnd.actual = (uint64_t)(val);           \
        sizeof((val) == ((hnd).for_check)
  }

Unfortunately this may not work because we would have to use for_check
for type safety when the pointer is retrieved as we may not have the
type in hand (for instance in get_xen_guest_handle) and accessing
another member than the currently used is not clearly specified (see [2])

However, IIUC, the get_xen_guest_handle is only exposed to the tools and
therefore possible to modify it in order to pass the type. Am I wrong?
FWIW, I haven't seen any usage in the tools directory.

Note that I've also explored the solution to use a structure for ARM32
with contain a padding. I.e smth like:
   struct {
      type *p;
      uint32_t pad;
   }

But it's not possible to set all the fields in one assignation with
(type) { val, 0 } because we don't have the type in hand in
set_xen_guest_handle_raw.

So if we can modify the get_xen_guest_handle to pass a type, Ian's
solution would be the best way forward. Any opinions?

Regards,

[1] http://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel