Re: [Xen-devel] [PATCH v6 1/4] xen: introduce SYMBOL

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 6 Feb 2019, Ian Jackson wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH v6 1/4] xen: introduce SYMBOL"):
> > On 06.02.19 at 17:37, <ian.jackson@xxxxxxxxxx> wrote:
> > > Jan Beulich writes ("Re: [Xen-devel] [PATCH v6 1/4] xen: introduce 
> > > SYMBOL"):
> > >> - it allows the end-of-whatever symbols to also be handed to
> > >>   functions in a type-safe manner
> > > 
> > > Yes.  However...

I am OK with this approach. Maybe not the best IMO, but good enough. It
should also satisfy the MISRAC guys, as they wrote "ideally cast to
uintptr_t only once": here we wouldn't be casting only once, but at
least we would do it inside a single well-defined macro.

I did manage to convert v4 of the series to this approach before writing
this answer -- everything looks plausible and compiles OK. Also, see
below.


> > >>   (we could even have per-instance structure flavors, such that
> > >>   unrelated "end" symbols can't be mixed up; for example the type
> > >>   could simply be a structure wrapping a field of the original base
> > >>   type).
> > > 
> > > I think this would be difficult to achieve without writing a forbidden
> > > pointer comparison in the macro.  It could perhaps be achieved with
> > > typeof() if that is available in hypervisor code.
> > 
> > I'm afraid I don't understand - you want to cast to an integer
> > type anyway for doing the comparison.
> 
> The usual approach to haking a macro perform an explicit typecheck
> (ie, to have the macro check that the types of its arguments are
> right) is to have the macro expansion contain a `spurious' comparison
> whose result is ignored but which will yield a compile-type type
> mismatch if the argument types were wrong.  But this is only legal if
> the provenance of the compared pointers is legal for a pointer
> comparison.  The bad effects of evaluating an UB expression are not
> limited by within-program causality.
> 
> > As to typeof() - this being a compiler construct, it is available
> > whenever the compiler supports it. We certainly use it
> > elsewhere in hypervisor code.
> 
> I think then that
>    (typeof(X))0 == (typeof(Y))0
> is the correct formulation of the type check - because it is legal no
> matter the provenance of X and Y.

Thank you, Ian. I think I understand what you are saying. I'll probably
leave this out for the next iteration though, but I am happy to add it
afterwards.

I was thinking of going with two MACROs:

+/*
+ * Performs x - y, returns the original pointer type. To be used when
+ * either x or y or both are linker symbols.
+ */
+#define SYMBOLS_SUBTRACT(x, y) ({                                             \
+    __typeof__(*(y)) *ptr_;                                                   \
+    ptr_ = (typeof(ptr_)) (((uintptr_t)(x) - (uintptr_t)(y)) / sizeof(*(y))); \
+    ptr_;                                                                     \
+})
+
+/*
+ * Performs x - y, returns uintptr_t. To be used when either x or y or
+ * both are linker symbols.
+ */
+#define SYMBOLS_COMPARE(x, y) ({                                              \
+    uintptr_t ptr_;                                                           \
+    ptr_ = ((uintptr_t)(x) - (uintptr_t)(y)) / sizeof(*(y));                  \
+    ptr_;                                                                     \
+})

Examples:

+    new_ptr = SYMBOLS_SUBTRACT(func->old_addr, _start) + vmap_of_xen_text;

and:

+    for ( alt = region->begin;
+          SYMBOLS_COMPARE(alt, region->end) < 0;
+          alt++ )

We could also define a third macro such as:

  #define SYMBOLS_SUBTRACT_INT(x, y)  SYMBOLS_COMPARE((x), (y))

because we have many places where we need the result of SYMBOLS_SUBTRACT
converted to an integer type. For instance:

  paddr_t xen_size = (uintptr_t)SYMBOLS_SUBTRAC(_end, _start);

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