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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 4 Jan 2019, Jan Beulich wrote:
> >>> On 02.01.19 at 19:20, <sstabellini@xxxxxxxxxx> wrote:
> > On Wed, 14 Nov 2018, Jan Beulich wrote:
> >> >>> On 13.11.18 at 23:02, <sstabellini@xxxxxxxxxx> wrote:
> >> > On Tue, 13 Nov 2018, Jan Beulich wrote:
> >> >> >>> On 13.11.18 at 14:17, <Julien.Grall@xxxxxxx> wrote:
> >> >> > On 13/11/2018 12:56, Jan Beulich wrote:
> >> >> >>>>> On 13.11.18 at 00:06, <sstabellini@xxxxxxxxxx> wrote:
> >> >> >>> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu)
> >> >> >>>       if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL )
> >> >> >>>           return -ENOMEM;
> >> >> >>>   
> >> >> >>> -    memset(p, 0, __per_cpu_data_end - __per_cpu_start);
> >> >> >>> -    __per_cpu_offset[cpu] = p - __per_cpu_start;
> >> >> >>> +    memset(p, 0, SYMBOL(__per_cpu_data_end) - 
> >> >> >>> SYMBOL(__per_cpu_start));
> >> >> >>> +    __per_cpu_offset[cpu] = (unsigned long)p - 
> >> >> >>> SYMBOL(__per_cpu_start);
> >> >> >> 
> >> >> >> Can't you make SYMBOL() retain the original type, such that casts
> >> >> >> like this one aren't needed? As soon as the compiler doesn't know
> >> >> >> anymore that particular globals (or statics) are used, it can't infer
> >> >> >> anymore that two pointers can't possibly point into the same array.
> >> >> > 
> >> >> > If SYMBOL() keeps the original type, then you will still substract 2 
> >> >> > pointers. If the compiler can't infer the cannot possibly point into 
> >> >> > the 
> >> >> > same array, it also cannot infer they point to the same. So that 
> >> >> > would 
> >> >> > be undefined, right?
> >> >> 
> >> >> Undefined behavior results if you _actually_ subtract pointers pointing
> >> >> into different objects. Subtracting of pointers is not generally 
> >> >> undefined.
> >> >> The compiler can use undefined-ness only if it can prove that both
> >> >> pointers do point into different objects.
> >> > 
> >> > Let's remember that we are not trying to work-around the compiler, we
> >> > are trying to make our code C standard compliant :-)  The compiler might
> >> > not be able to infer anymore that two pointers can't possibly point into
> >> > the same array, but we would still be not-compliant. It doesn't solve
> >> > our problem, especially in regards to certifications.
> >> 
> >> But then this entire patch is pointless: SYMBOL() is exclusively about
> >> deluding the compiler. To make the code standard compliant, you'd
> >> have to e.g. do away with all combined (start and end) uses (in C
> >> files) of symbols bounding sections. I at least cannot think of a
> >> standard compliant way of expressing these. Oddly enough I had
> >> once tried to deal with this issue (for other reasons), but the patch
> >> wasn't liked:
> >> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02718.html 
> >> All the remaining end symbols then could obviously go away in favor
> >> of using the size expressions, but as you see further C limitations
> >> make it necessary to use asm() for the ones which get converted.
> >> 
> >> Talking of asm()-s: C standard compliance, in a strict sense, would
> >> require dropping all of them as well. I'm afraid that when writing
> >> special purpose code like OS kernels or hypervisors are, if you
> >> want to avoid to resort extensively to assembly code, you'll have
> >> to accept to bend some of the language rules, just making sure
> >> that the compiler won't have means to mis-interpret the constructs
> >> used.
> >>
> >> > I is safer to use unsigned long as return type for SYMBOL and avoid
> >> > pointers comparisons completely. The code impact is very limited and
> >> > then we don't have to prove same or different "objectness" at all.
> >> 
> >> Well, that's one perspective to take. The other is that hidden or
> >> explicit casts are always a risk (and hence when reviewing code
> >> I'm quite picky about any ones introduced anew or even just
> >> retained without reason). Making constructs needing to cast
> >> things at least finally cast back to the original type often at least
> >> lowers this risk.
> > 
> > The new casts added actually cancel themselves out with the ones been
> > removed (some casts to unsigned long have been removed). I went through
> > the patch, these are the stats:
> > 
> > arch/arm: +4
> > arch/x86:  0
> > common:   -4
> > 
> > Overall the impact is basically null. Given the plus side of not having to
> > prove same or different "objectness", I think it is the best compromise
> > in this case. My preference is to use unsigned long as return type, as
> > done in this version of the patch.
> 
> But if I'm not misremembering there could be an overall win if you
> casted the result back to the original type, as suggested on the
> other sub-thread.

I think you are right. I am not saying that SYMBOL returning unsigned
long reduces the number of casts compared to SYMBOL returning original
type.

I am only saying that compared to before this series, the number of
casts required to introduce SYMBOL returning unsigned long is small.
Given that it is preferable to make unsigned long comparisons rather
than pointers comparisons, I think it is the best way forward.

In other words, with unsigned long comparisons Xen is more C compliant,
it helps with MISRA-C, it helps with us not having to figure out when
pointers point to different objects. All for the price of very few new
unsigned long casts (in fact, zero new casts in this version of the
series).


I realize that you are not convinced by these arguments, but let's find
a way forward. My preference would be to have SYMBOL returning unsigned
long and do unsigned long comparisons when pointers pointing to
different objects are involved.

If you are strongly opposed to it, I'll change the code to have SYMBOL
returning the original type and do pointers comparisons. It is
not clear to me whether this approach is actually C compliant and
MISRA-C compliant, but it is still better than what we have today. At
the very least it clearly highlights all the problematic sites.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel