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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 8 Jan 2019, Stefano Stabellini wrote:
> On Tue, 8 Jan 2019, Jan Beulich wrote:
> > >>> On 07.01.19 at 19:29, <sstabellini@xxxxxxxxxx> wrote:
> > > On Mon, 7 Jan 2019, Jan Beulich wrote:
> > >> >>> On 04.01.19 at 18:05, <sstabellini@xxxxxxxxxx> 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 is (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?

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