date:20230616

Re: [PATCH v2] xen/misra: add rules 1.4 and 2.1

2023-06-16 Thread Luca Fancellu



> On 15 Jun 2023, at 22:27, Stefano Stabellini  wrote:
> 
> From: Stefano Stabellini 
> 
> Also add a comment at the top of the file to say rules.rst could be
> changed.
> 
> Signed-off-by: Stefano Stabellini 

Hi Stefano,

Reviewed-by: Luca Fancellu 


While I was testing the patch with our script that translates the docs to 
cppcheck
Inputs, I noticed we might have a small issue there, seems that Directives and 
Rules
clashes, and from a quick look to cppcheck addon, seems that only the rules are 
needed.

I’ll have a look on that soon.

> 
> ---
> Changes in v2:
> - add link for 1.4
> - expand 1.4 comment to say it could be revisited
> - add comment at the top
> ---
> docs/misra/rules.rst | 15 +++
> 1 file changed, 15 insertions(+)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index a88c284e7d..11b9c42b70 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -32,6 +32,9 @@ violations are meant to be documented as deviations, while 
> some others
> should be fixed. Both compliance and documenting deviations on the
> existing codebase are work-in-progress.
> 
> +The list below might need to be updated over time. Reach out to THE REST
> +maintainers if you want to suggest a change.
> +
> .. list-table::
>:header-rows: 1
> 
> @@ -90,6 +93,18 @@ existing codebase are work-in-progress.
>behaviour
>  -
> 
> +   * - `Rule 1.4 
> `_
> + - Required
> + - Emergent language features shall not be used
> + - Emergent language features, such as C11 features, should not be
> +   confused with similar compiler extensions, which we use. When the
> +   time comes to adopt C11, this rule will be revisited.
> +
> +   * - `Rule 2.1 
> `_
> + - Required
> + - A project shall not contain unreachable code
> + -
> +
>* - `Rule 2.6 
> `_
>  - Advisory
>  - A function should not contain unused label declarations
> -- 
> 2.25.1
> 
>

Re: Refactoring of a possibly unsafe pattern for variable initialization via function calls

2023-06-16 Thread Jan Beulich

On 15.06.2023 18:39, nicola wrote:
> while investigating possible patches regarding Mandatory Rule 9.1, I
> found the following pattern, that is likely to results in a lot possible
> positives from many (all) static analysis tools for this rule.
> 
> This is the current status (taken from `xen/common/device_tree.c:135')
> 
> 
> const struct dt_property *dt_find_property(const struct dt_device_node *np,
> const char *name, u32 *lenp)
> {
>  const struct dt_property *pp;
> 
>  if ( !np )
>  return NULL;
> 
>  for ( pp = np->properties; pp; pp = pp->next )
>  {
>  if ( dt_prop_cmp(pp->name, name) == 0 )
>  {
>  if ( lenp )
>  *lenp = pp->length;
>  break;
>  }
>  }
> 
>  return pp;
> }
> 
> 
> 
> 
> It's very hard to detect that the pointee is always written whenever a 
> non-NULL pointer for `lenp' is supplied, and it can safely be read in 
> the callee, so a sound analysis will err on the cautious side.

I'm having trouble seeing why this is hard to recognize: The loop can
only be exited two ways: pp == NULL or with *lenp written.

For rule 9.1 I'd rather expect the scanning tool (and often the compiler)
to get into trouble with the NULL return value case, and *lenp not being
written yet apparently consumed in the caller. Then, however, ...

> My proposal, in a future patch, is to refactor these kinds of functions 
> as follows:
> 
> 
> const struct dt_property *dt_find_property(const struct dt_device_node *np,
> const char *name, u32 *lenp)
> {
>  u32 len = 0;
>  const struct dt_property *pp;
> 
>  if ( !np )
>  return NULL;

... this path would be a problem as well.

>  for ( pp = np->properties; pp; pp = pp->next )
>  {
>  if ( dt_prop_cmp(pp->name, name) == 0 )
>  {
>  len = pp->length;
>  break;
>  }
>  }
> 
>  if ( lenp )
>  *lenp = len;
>  return pp;
> }
> 
> 
> The advantage here is that we can easily argue that `*lenp' is always
> initialized by the function (if not NULL) and inform the tool about
> this, which is a safer API and also resolves almost all subsequent
> "don't know"s about further uses of the variables involved (e.g. `lenp').

The disadvantage is that in a more complex case and with the function
e.g. being static, the initializer of "len" may prevent compiler /
tools from spotting cases where the variable would (otherwise) truly
(and wrongly) remain uninitialized (and that fact propagating up the
call chain, through - in this example - whatever variable's address
the caller passed for "lenp"). IOW - I don't think a common pattern
can be agreed upon up front for cases like this one.

Jan

Re: [XEN PATCH] docs/misra: document the C dialect and translation toolchain assumptions.

2023-06-16 Thread Roberto Bagnara

On 16/06/23 08:53, Jan Beulich wrote:

On 16.06.2023 01:26, Stefano Stabellini wrote:

On Thu, 15 Jun 2023, Roberto Bagnara wrote:
I have a few comments below, mostly to clarify the description of some
of the less documented GCC extensions, for the purpose of having all
community members be able to understand what they can and cannot use.

What do you mean by "can and cannot use"? Is this document intended to
forbid the use of any extensions we may not currently use, or we use
but which aren't enumerated here?

One of the reasons that kept me from replying to this submission is
that the full purpose of this new doc isn't stated in the description.

My full purpose was to give the community a starting point for the
discussion on the assumptions the project makes on the programming
language and the translation toolchains that are intended to be used
now or in the future. As far as I know, no documentation is currently
provided on these topics, so I believe the document fills a gap and
I hope it is good enough as a starting point.

Which in turn leaves open whether certain items actually need to be
here (see e.g. the libc related remark below).

Because the analyzed build used to included some of the tools, which in turn
relied on libc for program termination. Once confirmation is given
that the analyzed build is now what is intended, all references to
libc can be removed.

Another is that it's
hard to tell how to convince oneself of this being an exhaustive
enumeration. One extension we use extensively yet iirc is missing here
is omission of the middle operand of the ternary operator.

Not sure I understand: do you mean something different from the following
entry in the document?

* - Binary conditional expression
- ARM64, X86_64
- See Section "6.8 Conditionals with Omitted Operands" of GCC_MANUAL.

+Reference Documentation
+___
+
+The following documents are referred to in the sequel:
+
+GCC_MANUAL:
+ https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc.pdf
+CPP_MANUAL:
+ https://gcc.gnu.org/onlinedocs/gcc-12.1.0/cpp.pdf

Why 12.1 when meanwhile there's 12.3 and 13.1?

For no special reason: as I said, my purpose is only to provide
a starting point for discussion and customization of the
assumptions.

+ARM64_ABI_MANUAL:
+
https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst
+X86_64_ABI_MANUAL:
+
https://gitlab.com/x86-psABIs/x86-64-ABI/-/jobs/artifacts/master/raw/x86-64-ABI/abi.pdf?job=build
+ARM64_LIBC_MANUAL:
+ https://www.gnu.org/software/libc/manual/pdf/libc.pdf
+X86_64_LIBC_MANUAL:
+ https://www.gnu.org/software/libc/manual/pdf/libc.pdf

How is libc relevant to the hypervisor?

See above.

+ * - Empty declaration
+ - ARM64, X86_64
+ - Non-documented GCC extension.

For the non-documented GCC extensions, would it be possible to add a
very brief example or a couple of words in the "References" sections?
Otherwise I think people might not understand what we are talking about.

For instance in this case I would say:

An empty declaration is a semicolon with nothing before it.
Non-documented GCC extension.

Which then could be confused with empty statements. I think in a document
like this language needs to be very precise, to avoid ambiguities and
confusion as much as possible. (Iirc from going over this doc yesterday
this applies elsewhere as well.)

OK.

+ * - Ill-formed source detected by the parser

As we are documenting compiler extensions that we are using, I am a bit
confused by the name of this category of compiler extensions, and the
reason why they are bundled together. After all, they are all separate
compiler extensions? Should each of them have their own row?

OK.

+
+ * - Unspecified escape sequence is encountered in a character constant or a
string literal token
+ - X86_64
+ - \\m:
+ non-documented GCC extension.

Are you saying that we are using \m and \m is not allowed by the C
standard?

This exists in the __ASSEMBLY__ part of a header, and I had previously
commented on Roberto's diagnosis (possibly derived from Eclair's) here.
As per that I don't think the item should be here, but I'm of course
open to be shown that my understanding of translation phases is wrong.

I was not convinced by your explanation but, as I think I have said already,
I am not the one to be convinced. In the specific case, independently
from __ASSEMBLY__ or any other considerations, that thing reaches the C
preprocessor and, to the best of my knowledge, the C preprocessor documentation
does not say how that would be handled. I have spent a lot of time in the
past 10 years on the study of functional-safety standards, and what I
am providing is a honest opinion on what I believe is compliant
and what is not. But I may be wrong of course: if you or anyone else feels
like they would not have any problems in arguing a different position
from mine in front of an assesso

80 matches

Mail list logo