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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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