Re: [Xen-devel] [PATCH v2] libxl: ocaml: guard x86-specific functions behind an ifdef

[Anil Madhavapeddy <anil@xxxxxxxxxx>]

On Tue, Jan 14, 2014 at 02:43:49PM +0000, Ian Campbell wrote:
> On Sun, 2014-01-12 at 19:17 +0000, Dave Scott wrote:
> > Hi Anil,
> > 
> > Thanks for getting oxenstored on ARM working!
> > 
> > I'm happy with a simple 'Failure not implemented' exception for the
> > moment. I think that once we're using the libxl bindings everywhere we
> > can probably remove these libxc bindings anyway.
> > 
> > Acked-by: David Scott <dave.scott@xxxxxxxxxxxxx>
> 
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> Release hat: This is pretty tightly targeted, and the worst that could
> happen is a build failure, which is generally pretty easy to detect
> before a release, although perhaps less so in the case of ocaml which
> isn't enabled by everyone.
> 
> Although I have two misgivings in that regard:
> 
> > > diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > b/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > index f5cf0ed..ff29b47 100644
> > > --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > @@ -714,6 +714,7 @@ CAMLprim value stub_xc_domain_cpuid_set(value
> > > xch, value domid,  {
> > >   CAMLparam4(xch, domid, input, config);
> > >   CAMLlocal2(array, tmp);
> 
> Aren't these variables now unused? Does the compiler not complain about
> this (with -Werror => build failure)?
> 
> Perhaps CAMLlocal2 both defines and references the variables keeping
> this issue at bay?

That's right.  CAMLlocal2 creates a stack variable and registers it with
the garbage collector as a root (to ensure that it's not collected during
the lifetime of the function).  This keeps it live and always used from
the perspective of the C compiler.

I confirmed this with a 'make V=1' with '-Wall -Werror' to make sure, and
it compiles fine on gcc-4.8.1.  There's an unrelated signed/unsigned error
if -Wextra is included that I'll look at separately and is unrelated to
this patch.

> > > +#if defined(__i386__) || defined(__x86_64__)
> > >   int r;
> > >   unsigned int c_input[2];
> > >   char *c_config[4], *out_config[4];
> > > @@ -742,17 +743,24 @@ CAMLprim value stub_xc_domain_cpuid_set(value
> > > xch, value domid,
> > >                    c_input, (const char **)c_config, out_config);
> > >   if (r < 0)
> > >           failwith_xc(_H(xch));
> > > +#else
> > > + caml_failwith("xc_domain_cpuid_set: not implemented");
> > > #endif
> > >   CAMLreturn(array);
> 
> In the !__i386__ && !__x86_64__ case this code is unreachable, right
> because caml_failwith is marked Noreturn?
> 
> Is there any chance that some compiler version might pick up on this and
> complain about the dead code? Or perhaps complain about the
> uninitialised use of array?
> 
> I suppose putting the CAMLreturn inside the x86 case runs the opposite
> risk of a compiler which doesn't pay proper attention to Noreturn and
> therefore things we are reaching the end of a non-void function? Would
> CAMLreturn(unit) be appropriate in that case?

Yeah, I'm not aware of any compiler that doesn't respect the noreturn
attribute and also emits unused variable warnings. I didn't modify the
CAMLreturn in favour of minimising the x86/ARM differences, but you could
modify the #endif to be an #else/#endif to only return on x86.  I'd prefer
to keep these bindings as straight-line as possible for the 4.4 release
though, and to refactor oxenstored to not depend on them at all in the
future (it only uses a small part of libxc and these cpuid functions
aren't used at all).

(snip)
> > > 
> > > +#else
> > > + caml_failwith("xc_domain_cpuid_check: not implemented");
> > > #endif
> > >   CAMLreturn(ret);
> 
> Unreached?

See above.

cheers,
Anil

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