On Tue, 22 Aug 2023, Jan Beulich wrote: > On 22.08.2023 03:40, Stefano Stabellini wrote: > > On Mon, 21 Aug 2023, Jan Beulich wrote: > >> On 19.08.2023 03:24, Stefano Stabellini wrote: > >>> @@ -106,7 +107,23 @@ maintainers if you want to suggest a change. > >>> * - `Rule 2.1 > >>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ > >>> - Required > >>> - A project shall not contain unreachable code > >>> - - > >>> + - The following are allowed: > >>> + - Invariantly constant conditions (e.g. while(0) { S; }) > >> > >> When (and why) was this decided? The example given looks exactly like what > >> Misra wants us to not have. > > > > This covers things like: > > > > if (IS_ENABLED(CONFIG_HVM)) > > > > which could resolve in if (0) in certain configurations. I think we gave > > feedback to Roberto that we wanted to keep this type of things as is. > > Ah, I see. But then perhaps mention that rather than plain 0 here?
Yes, I'll do > See below as to whether a complete list of excepted constructs is > wanted. [...] > >>> + - Unreachability caused by the following macros/functions is > >>> + deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM, > >>> + PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap, > >>> + machine_halt, machine_restart, machine_reboot, > >>> + ASSERT_UNREACHABLE > >> > >> Base infrastructure items like BUG() are imo fine to mention here. But > >> specific items shouldn't be; the more we mention here, the more we invite > >> the list to be grown. Note also how you mention items which no longer > >> exist (ERROR_EXIT{,_DOM}, PIN_FAIL). > > > > The question is whether we want this list to be exhaustive (so we want > > to mention everything for which we make an exception) or only an example > > (in which case just BUG is fine.) > > > > Let's say we only mention BUG. Where should we keep the exhaustive list? > > Is it OK if it only lives as an ECLAIR config file under > > automation/eclair_analysis? There is another very similar question > > below. > > First and foremost we need to have a single place where everything is > recorded, or where at least a pointer exists to where further details > are. As to this being the Eclair config file: Shouldn't any such > constructs rather be listed in the deviations file, such that e.g. > cppcheck can also benefit? Yes, you are right. Basically the choice is whether we want a project-wide deviation, something like a change in how we interpret the rules, or a regular deviation for one macro or one function. After reading your comments, I also think that all the macros above should be covered by safe.json and in-code comments instead. > > BTW I think both options are OK. > > > > If we only mention BUG, we are basically saying that as a general rule > > only BUG is an exception. Then we have a longer more detailed list for > > ECLAIR because in practice things are always complicated. > > > > On the other hand if we have the full list here, then the documentation > > is more precise, but it looks a bit "strange" to see such a detailed > > list in this document and also we need to make sure to keep the list > > up-to-date. > > Thing is: This list shouldn't grow very long anyway, and also better > would grow / change much over time. +1 > >>> @@ -167,7 +184,7 @@ maintainers if you want to suggest a change. > >>> * - `Rule 5.6 > >>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ > >>> - Required > >>> - A typedef name shall be a unique identifier > >>> - - > >>> + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed > >> > >> I think this permission needs to be limited as much as possible. > > > > Maybe we should take this out completely given that they should be > > limited to efi and acpi code that is excepted anyway > > I would favor that, yes. > > >>> @@ -183,7 +200,10 @@ maintainers if you want to suggest a change. > >>> * - `Rule 7.1 > >>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_01.c>`_ > >>> - Required > >>> - Octal constants shall not be used > >>> - - > >>> + - Usage of the following constants is safe, since they are given > >>> + as-is in the inflate algorithm specification and there is > >>> + therefore no risk of them being interpreted as decimal constants: > >>> + ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$ > >> > >> This is a very odd set of exceptions, which by stating them here you then > >> grant to be exercised everywhere. Once again I don't think special cases > >> dealing with a single source or sub-component should be globally named. > > > > Actually I agree with you there. The problem is where to put them. > > safe.json? Yes you are right > > Right now we have docs/misra/rules.rst with the list of accepted rules > > and their special interpretations by Xen Project. We also have the > > ECLAIR configuration under automation/eclair_analysis with a bunch of > > ECLAIR specific config files. > > > > Is it OK if the constants above only live under > > automation/eclair_analysis and nowhere else? Or should we have another > > rst document under docs/misra for special cases dealing with a single > > source? > > As per above, I think putting anything in Eclair's config file should > only ever be a last resort. Wherever possible we should try to put stuff > in files which aren't tool specific. Where necessary special tool > settings can then be (machine-)derived from there. I agree. I'll send out another patch to update rules.rst. And later as a follow-up I'll see what I can do to update safe.json.