[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 2/2] xen: use SYMBOL when required

On Wed, 2 Jan 2019, Stefano Stabellini wrote:
> On Tue, 13 Nov 2018, Jan Beulich wrote:
> > >>> On 13.11.18 at 00:06, <sstabellini@xxxxxxxxxx> wrote:
> > > --- a/xen/arch/x86/alternative.c
> > > +++ b/xen/arch/x86/alternative.c
> > > @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct 
> > > alt_instr *start,
> > >       * So be careful if you want to change the scan order to any other
> > >       * order.
> > >       */
> > > -    for ( a = base = start; a < end; a++ )
> > > +    for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ )
> > 
> > At this point all is fine: end is allowed to point to the end of start[].
> > If anything you want to change the invocations (where the
> > questionable symbols are used). I'm also not convinced you need
> > to touch both sides of the comparison or subtraction expressions.
> > 
> > In order for people to not start wondering what the purpose of
> > SYMBOL() is at any of its use sites, you really want to use it on
> > the problematic symbols themselves, not somewhere on a derived
> > variable or parameter.
> I wasn't sure about what to do about derived variables and decided to
> err on the safe side. I am happy to remove those changes, because I
> agree that it would be far clearer if SYMBOL() is only used on the
> problematic symbols.

I replied too quickly. I agree that changing only the problematic
symbols, and not the derived variables, is easier to understand from the
code point of view, and should still be safe and compliant.

However, I would also prefer to keep the comparisons as unsigned long
comparisons, like it is done in this patch. I think it is safer and more
compliant to do unsigned long comparisons to avoid completely the issue
of having to understand when pointers point to same or different
objects. It is certainly more in the spirit of the spec. 

So, in the case you mentioned above (and a bunch of similar cases), to
do unsigned long comparisons only and apply SYMBOL() only to the
problematic symbols, we need one more cast to unsigned long at the
derived variable side:

  for ( a = base = start; (unsigned long)a < SYMBOL(end); a++ )

FYI I realized I was using SYMBOL() conversions more than strictly
necessary at the derived variables side, but I though it would be nicer
to have:

  SYMBOL(derived) < SYMBOL(problem_variable)

rather than

  (unsigned long)derived < SYMBOL(problem_variable)

everywhere. I am happy either way as both should be compliant.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.