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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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