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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Jan,

One question below to make a decision on the way forward.

On Mon, 14 Jan 2019, Jan Beulich wrote:
> >>> On 14.01.19 at 04:45, <Stewart.Hildebrand@xxxxxxxxxxxxxxx> wrote:
> > So let's keep the linker-accessible variable as a type that works for the
> > linker (which really could be anything as long as you use the address, not
> > the value), but name it something else - a name that screams "DON'T USE ME
> > UNLESS YOU KNOW WHAT YOU'RE DOING". And then before the first use, copy
> > that value to "uintptr_t _start;".
> > 
> > The following is a quick proof of concept for aarch64. I changed the type
> > of _start and _end, and added code to copy the linker-assigned value to
> > _start and _end. Upon booting, I see the correct values:
> 
> Global symbols starting with underscores should already be shouting
> enough. But what's worse - the whole idea if using array types is to
> avoid the intermediate variables.
> 
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -726,6 +726,12 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> > size_t dtb_size)
> >  
> >  size_t __read_mostly dcache_line_bytes;
> >  
> > +typedef char TYPE_DOESNT_MATTER;
> > +extern TYPE_DOESNT_MATTER _start_linker_assigned_dont_use_me,
> > +                          _end_linker_assigned_dont_use_me;
> 
> This and ...
> 
> > @@ -770,10 +776,17 @@ void __init start_xen(unsigned long boot_phys_offset,
> >      printk("Command line: %s\n", cmdline);
> >      cmdline_parse(cmdline);
> >  
> > +    _start = (uintptr_t)&_start_linker_assigned_dont_use_me;
> > +    _end = (uintptr_t)&_end_linker_assigned_dont_use_me;
> 
> ... this violates what the symbol names say. And if you want to
> avoid issues, you'd want to keep out of C files uses of those
> symbols altogether anyway, and you easily can: In any
> assembly file, have
> 
> _start:       .long _start_linker_assigned_dont_use_me
> _end: .long _end_linker_assigned_dont_use_me
> 
> In particular, they don't need to be runtime initialized, saving
> you from needing to set them before first use. But as said -
> things are the way they are precisely to avoid such variables.
> 
> >> But, instead of converting _start to unsigned long via SYMBOL_HIDE, we
> >> could convert it to uintptr_t instead, it would be a trivial change on
> >> top of the existing unsigned long series. Not sure if it is beneficial.
> > 
> > The difference would be whether we want to rely on implementation-defined
> > behavior or not.
> 
> Why not? Simply specify that compilers with implementation defined
> behavior not matching our expectations are unsuitable. And btw, I
> suppose this is just the tiny tip of the iceberg of our reliance on
> implementation defined behavior.

The reason is that relying on undefined behavior is not reliable, it is
not C compliant, it is not allowed by MISRA-C, and not guaranteed to
work with any compiler. Yes, this instance is only the tip of the
iceberg, we have a long road ahead, but we shouldn't really give up
because it is going to be difficult :-) Stewart's approach would
actually be compliant and help toward reducing reliance on undefined
behavior.

Would you be OK if I rework the series to follow his approach using
intermediate variables? See the attached patch as a reference, it only
"converts" _start and _end as an example. Fortunately, it will be
textually similar to the previous SYMBOL returning unsigned long version
of the series.

If you are OK with it, do you have any suggestions on how would you like
the intermediate variables to be called? I went with _start/start_ and
_end/end_ but I am open to suggestions. Also to which assembly file you
would like the new variables being added -- I created a new one for the
purpose named var.S in the attached example.

Attachment: change-type-poc
Description: Text document

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