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

[Stewart Hildebrand <Stewart.Hildebrand@xxxxxxxxxxxxxxx>]

On Thursday, January 10, 2019 12:30 PM, Stefano Stabellini wrote:
> On Thu, 10 Jan 2019, Jan Beulich wrote:
> > >>> On 10.01.19 at 03:40, <julien.grall@xxxxxxxxx> wrote:
> > > On Wed, 9 Jan 2019, 18:43 Stefano Stabellini, <sstabellini@xxxxxxxxxx>
> > > wrote:
> > >
> > >> Introduce a macro, SYMBOL, which is similar to RELOC_HIDE, but it is
> > >> meant to be used everywhere symbols such as _stext and _etext are used
> > >> in the code. It can take an array type as a parameter, and it returns
> > >> the same type.
> > >>
> > >> SYMBOL is needed when accessing symbols such as _stext and _etext
> > >> because the C standard forbids for both comparisons and substraction
> > >> (see C Standard, 6.5.6 [ISO/IEC 9899:2011] and [1]) between pointers
> > >> pointing to different objects. _stext, _etext, etc. are all pointers to
> > >> different objects from ANCI C point of view.
> > >>
> > >
> > > This does not make sense because you still return a pointer and therefore
> > > the undefined behavior is still present.
> > >
> > > I really don't believe this patch is going to make the MISRA tool happy.
> >
> > Well, till now I've been assuming that no version of this series was
> > posted without being certain the changes achieve the goal (of
> > making that tool happy).
> 
> No, Jan: unfortunately we cannot re-run the scanning tool against any
> version of Xen we wish to :-(
> 
> We cannot know in advance if a set of changes will make the tool happy
> or not.

Playing devil's advocate: even with all sorts of casting and inline assembly
to suppress static analysis tool warnings, we're still not addressing the
underlying rule violation. A pointer value casted or fed through some inline
assembly at the end of the day is still a value that represents an address
in an object. And as soon as we subtract or compare that value to one
that represents another object we start violating the MISRA rules (this is
my own rather strict interpretation for the purpose of making a point - 
please feel free to disagree). 

If all we really care about is making PRQA happy, I believe it does support
some sort of comment-based suppression. I've seen comments like
/* PRQA S 0487 */ or /* PRQA S 0488 */ in various codebases, I'm guessing
comments like this have something to do with suppressing these types of
warnings.

> 
> If I knew that SYMBOL returning the native point type as in v6 is
> sufficient to make the tool happy, I wouldn't be here arguing. We cannot
> know for sure until we commit the changes, then we ask PRQA to re-scan
> against a more recent version of Xen. It is an heavy process and for
> this reason I preferred the safer of the two approaches.
> 
> Anyway, I would rather get something in, even if insufficient, than
> nothing. So I'll address all your comments based on returning the
> pointer type, and submit a new version. The bothersome changes are the
> ones to the call sites, and I would like to get those in no matter the
> implementation of SYMBOL.

I agree, it would be nice to highlight everywhere we think we're in violation
of the "pointers to different objects" rules. Perhaps it would make it clearer
if we added a comment in the codebase to spell out the intent, which I'm
interpreting as roughly "This violates MISRA Rule 18.1, 18.2, or 18.3. We 
need to revisit this in the future to evaluate if we can avoid violating these
rules."

Or perhaps it would make it clearer to forget about SYMBOL altogether and
instead just add suppression comments.

Further, if we decide in an instance that we have no choice but to
subtract/compare pointers to different objects, then the MISRA rules I
mentioned are only *required* rules (not *mandatory*), which means we
are OK to violate them as long as we write up a deviation with the appropriate
justification and explanation of any undefined behavior and why it's OK to rely
on said undefined behavior.

-Stew

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