Re: [Xen-devel] libxc: Casting of xen virtual address type xen_vaddr_t to signed int64 type: (int64_t)vaddr

[Viktor Mitin <viktor.mitin.19@xxxxxxxxx>]

Hi Jan and Andrew, All

From standard:
The result of E1 >> E2 is E1 right-shifted E2 bit positions.
If E1 has an unsigned type or if E1 has a signed type and a nonnegative value,
the value of the result is the integral part of the quotient of E1 / 2E2.
If E1 has a signed type and a negative value,
the resulting value is implementation-defined.

To rephrase: in case of right shifting and when the original number is negative,
the standard does not define whether the shift is arithmetic or logical
(i.e. will it preserve the sign or not).

In our example, in the case when
(int64_t)vaddr < 0
the result of next shift is 'implementation-defined' (and not
'undefined behavior'):
(int64_t)vaddr >> 63

Mean that result of "(int64_t)vaddr >> 63" can be 0 or 1.
So the next code may not work properly in case of another 'implementations'.
With another compiler (i.e. clang, etc) this code may introduce bugs
which are hard to find.

((int64_t)vaddr >> 47) == ((int64_t)vaddr >> 63)

For this reason it is better to avoid implementation-defined code.

Do you agree?

Thanks

On Fri, May 17, 2019 at 3:32 PM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 17.05.19 at 13:25, <viktor.mitin.19@xxxxxxxxx> wrote:
> > Hi All,
> >
> > While looking at code in tools/libxc/xc_sr_save_x86_pv.c,
> > we see that there is casting of xen virtual address type xen_vaddr_t
> > to signed int64 type.
> > In commit: 7bf74582b343603cb0826d808cd7da43326452a5
> >
> > +/* Check a 64 bit virtual address for being canonical. */
> > +static inline bool is_canonical_address(xen_vaddr_t vaddr)
> > +{
> > +    return ((int64_t)vaddr >> 47) == ((int64_t)vaddr >> 63);
> > +}
> >
> > It seems there is no need to cast vaddr variable. It looks like
> > shifting vaddr signed 64-bit value by 63 bits introduces undefined
> > behavior.
>
> I don't think so - as per my reading of the standard text, UB
> results only when the shift count is greater or equal than the
> width of the (promoted) shifted value's type. The results of
> both shifts above are implementation defined afaict.
>
> > Could you please describe what is the reason for this casting?
>
> Well, we want to check that the top 17 bits are either all 1s
> or all 0s, preferably with just a single comparison.
>
> Jan
>
>

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