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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel