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

[Julien Grall <julien.grall@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4973 bytes --]

Hi,

Sorry for the formatting.

On Tue, 8 Jan 2019, 13:09 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Tue, 8 Jan 2019, Stefano Stabellini wrote:
> > On Tue, 8 Jan 2019, Jan Beulich wrote:
> > > >>> On 07.01.19 at 19:29, <sstabellini@kernel.org> wrote:
> > > > On Mon, 7 Jan 2019, Jan Beulich wrote:
> > > >> >>> On 04.01.19 at 18:05, <sstabellini@kernel.org> wrote:
> > > >> > 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.
> > > >>
> > > >> I continue to fail to see how suitable hiding of the connection to
> the
> > > >> original symbol from the compiler makes code less standard compliant
> > > >> when comparing pointers: The compiler simply can't know whether
> > > >> the underlying object ills (in the extreme case) an array spanning
> the
> > > >> entire address space.
> > > >
> > > > That is because the requirement I am trying to address is MISRA-C
> > > > compliance, which in turns requires C language compliance for C
> language
> > > > (I think it allows mixing C with assembly code). I don't particularly
> > > > care whether the compiler can or cannot find the connection to the
> > > > original symbol.
> > > >
> > > > The important thing for me is to avoid comparisons (and subtractions)
> > > > between pointers pointing to different objects. If we compare
> unsigned
> > > > longs, it is easier to prove that the comparison is not between
> pointers
> > > > pointing to different objects, even if somehow the numeric values
> > > > indirectly come from pointers. If we compare pointers, even if they
> went
> > > > through some sort of assembly conversions, we are still comparing
> > > > pointers pointing to different objects. The compiler might not be
> able
> > > > to figure it out, but a MISRA-C compliance scanning tool, or a human,
> > > > might.
> > >
> > > This is absurd: We are similarly still comparing pointers to different
> > > objects when comparing their values casted to unsigned long. The
> > > cast is as much of a hiding technique as any other one. If you want
> > > to be C language compliant without any compromises, you'll have to
> > > do away with all *_end symbols.
> >
> > Basically, this is a matter of interpretation of the spec: it seems to
> > me that coming back from asm-land with pointers and comparing pointers
> > would be a worse offense than a (almost) harmless unsigned long
> > comparison of values returned from asm-land.
> >
> > But I am not particularly knowledgeable about MISRA-C compliance and
> > their interpretation of the rules.
> >
> > So, this is what I am going to do: I'll send a series update according
> > to your suggestion, with SYMBOL returning the native pointer type. As I
> > wrote earlier, although weaker, it is still an improvement from my point
> > of view.
>
> There is a problem with this though I didn't foresee :-(
>
> The native type of _start is not char* -- it is char[]. So I cannot
> actually return the native type from SYMBOL because I cannot cast to
> (char[]). I didn't notice it until I actually tried it.
>
> See the implementation of RELOC_HIDE:
>
>   #define RELOC_HIDE(ptr, off)                    \
>     ({ unsigned long __ptr;                       \
>       __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
>       (typeof(ptr)) (__ptr + (off)); })
>
> It casts to the type at the end, the error is:
>
>   error: cast specifies array type
>        (typeof(ptr)) (__ptr + (off)); })
>
> We have a few options:
>
> 1) use unsigned long as in this version of the series (the Linux kernel
> also uses this technique)
> Sorry if I insist, it is still the best I think :-)
>
> 2) casts the parameters of SYMBOL to the corresponding pointer type
> For instance:
>   SYMBOL((char *)_start)
>   SYMBOL((struct alt_instr *)__alt_instructions_end)
> This works, but it is ugly, I would say it makes the code worse than
> option 1)
>
> 2) always return void* from SYMBOL
> I don't think it is a good idea to use void* as a workaround here
>
> 3) pass the desired return type to SYMBOL
> For instance:
>   SYMBOL(_start, char *)
>   SYMBOL(__alt_instructions_end, struct alt_instr *)
> Then SYMBOL would automatically cast the return type to char * and
> struct alt_instr * according to the second parameter.
>
> Do you have any other suggestions?
>

Reading [1], I think casting back to the initial type is pointless and not
going to help the static analyzer or compiler. After all, you still
compare/substract 2 pointers...

So, I think the only solution is 1).

Cheers,

[1]
https://kristerw.blogspot.com/2016/12/pointer-comparison-invalid-optimization.html?m=1


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

[-- Attachment #1.2: Type: text/html, Size: 6798 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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