Re: [RFC PATCH 1/4] xen/arm: justify or initialize conditionally uninitialized variables

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 20/07/23 17:39, Julien Grall wrote:
> Hi,
>
> The e-mail is getting quite long. Can you trim the unnecessary bits when 
> replying?
Ok.

> On 20/07/2023 15:23, Nicola Vetrini wrote:
> > > > > The problem is that _t may be uninitialized, hence assigning its 
> > > > > address to t could be problematic.
> > > > But the value is set right after. IOW, there is no read between. So 
> > > > how is this prob
> > > > > Another way to address this is to initialize _t to a bad value and 
> > > > > use this variable in the body, then assign to t based on the value 
> > > > > just before returning.
> > > > IHMO, neither solution are ideal. I think we should investigate 
> > > > whether Eclair can be improved.
> > > > [...]
> > > I'll see what can be done about it, I'll reply when I have an answer.
> > What about this:
> >
> > -    p2m_type_t _t;
> > +    p2m_type_t _t = p2m_invalid;
> > [...]
> >       t = t ?: &_t;
> > -    *t = p2m_invalid;
> > +    *t = _t;
> The resulting code is still quite confusing. I am still not quite sure 
> why ECLAIR can't understand the construct. Apologies if this was already 
> said, but this thread is getting quite long with many different issues. 
> So it is a bit difficult to navigate (which is why I suggested to split 
> and have a commit message explaining the rationale for each).
> Anyway, if we can't improve Eclair, then my preference would be the 
> following:
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index de32a2d638ba..05d65db01b0c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -496,16 +496,13 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t 
> gfn,
>       lpae_t entry, *table;
>       int rc;
>       mfn_t mfn = INVALID_MFN;
> -    p2m_type_t _t;
>       DECLARE_OFFSETS(offsets, addr);
>
>       ASSERT(p2m_is_locked(p2m));
>       BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
>
> -    /* Allow t to be NULL */
> -    t = t ?: &_t;
> -
> -    *t = p2m_invalid;
> +    if ( t )
> +        *t = p2m_invalid;
>
>       if ( valid )
>           *valid = false;
> @@ -549,7 +546,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>
>       if ( p2m_is_valid(entry) )
>       {
> -        *t = entry.p2m.type;
> +        if ( t )
> +            *t = entry.p2m.type;
>
>           if ( a )
>               *a = p2m_mem_access_radix_get(p2m, gfn);
Ok, I'll make a separate patch.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)