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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 10 Jan 2019, Stewart Hildebrand wrote:
> 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). 

Yes, this seems to be Jan's point of view too, but I disagree: _start is
not a pontier to an object -- it is a linker-set memory address. Also,
if there any doubts, certainly it is not a pointer to an object after
being converted to unsigned long in assembly. I don't think C compliance
could/should make any assumptions or guesses on asm returned values. But
I am less convinced of this argument if we convert it back to another
C pointer, which is what the current implementation does.

BTW, in case Jan's changes his mind, this is an alternative version of
v7 with SYMBOL_HIDE returning unsigned long:

http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git 
certification-7-unsigned_long


> 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.

Interesting ... something to investigate.


> > 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."

Yeah, that basically why I would like the series to go in. Even if it
doesn't make the code compliant, at least SYMBOL_HIDE clearly mark the
non-compliant sites.


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

We still need SYMBOL because of potential weird compiler issues, see the
comment on top of RELOC_HIDE in the Linux kernel.


> 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.


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