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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-10-23 at 08:24 -0600, Jan Beulich wrote:
> > > > On 23.10.15 at 16:03, <ian.campbell@xxxxxxxxxx> wrote:
> > On Fri, 2015-10-23 at 07:52 -0600, Jan Beulich wrote:
> > > > > > On 23.10.15 at 15:30, <ian.campbell@xxxxxxxxxx> wrote:
> > > > On Fri, 2015-10-23 at 14:13 +0100, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 04/10/15 20:24, Julien Grall wrote:
> > > > > > The keyword typeof is not portable:
> > > > > > 
> > > > > > /usr/src/freebsd/sys/xen/hypervisor.h:93:2: error: implicit
> > > > > > declaration
> > > > > > of function 'typeof' is invalid in C99
> > > > > > [-Werror,-Wimplicit-function-declaration]
> > > > > 
> > > > > Ping? Aside the fact that other bits of the header may not be iso
> > > > > compliant, I still think this patch is valid.
> > > > 
> > > > Yes, I agree.
> > > > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > 
> > > > Jan, after your earlier comments are you happy to go ahead with
> > > > this
> > > > for
> > > > now and sort the other possible issues separately?
> > > 
> > > Well - it's an improvement, sure, so I'm not intending to block it
> > > going in if no better way can be determined in its place right away.
> > > What makes me hesitant is that I'm not sure there indeed will be a
> > > follow up to this any time soon.
> > 
> > Are you saying with "better way" that Julien's fix is incorrect and
> > that
> > there is potentially a "proper" fix for this specific case? i.e. a
> > followup
> > to fix the use of __typeof__ in set_xen_guest_handle_raw which this
> > patch
> > introduces is expected?
> > 
> > I don't think you are, in which case are you suggesting that having
> > fixed
> > this one issue that Julien should then be on the hook for fixing all
> > similar/related issues in these header?
> > 
> > I don't think it is right to mandate that Julien put this followup work
> > onto his TODO list as a condition of accepting this patch, if this is
> > not a
> > case of Julien's change being incorrect and requiring a "proper" fix,
> > but
> > that there are other similar things wrong elsewhere.
> 
> I'm not meaning to mandate Julien to do all other follow up work. If
> anyone was to be mandated, then whoever introduced use of gcc
> extensions into public headers. But as we know, that doesn't
> normally work (on ARM perhaps everyone who helped with the
> bringup may still be around, but outside of ARM that's less likely, so
> arguably not a workable model).
> 
> The patch is an improvement, yes. So it may go in if right now
> no-one can't think of a proper fix for the problem at hand.

If no-one can think of a proper fix now I'm not sure why we would then
expect they would be able to in a follow up, after all if they could they
would have suggested it here and not in a follow up.

Perhaps this needs to be explicitly asked then, So, given:
#define set_xen_guest_handle_raw(hnd, val)                  \
    do {                                                    \
        typeof(&(hnd)) _sxghr_tmp = &(hnd);                 \
        _sxghr_tmp->q = 0;                                  \
        _sxghr_tmp->p = val;                                \
    } while ( 0 )

where typeof is used to avoid the multiple evaluations of hnd which
assigning to hnd.q and hnd.p would involve, is there some way to do the
same in pure ANSI C?

Changing the arguments to set_xen_guest_handle_raw (e.g. to include a type)
would be a prohibitively large change to all of the callers I think, we
should probably just rule that out now.

>  But
> in the end all the patch does is papering over an issue, instead
> of eliminating it. And that's the grounds on which I wasn't (and
> still am not really) happy to see it go in.

If it is to go in then I think under the circumstances the limitations of
the change merely from typeof to __typeof__ ought to be spelled out. i.e.
that the latter is no more standard, just perhaps somewhat more portable.

Ian.

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