| Message ID | 7f8cbd8c8ad64cd3a0d099f31cb4d3fad48aa63b.1690985045.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address MISRA C:2012 Rule 2.1 | expand |
On 02.08.2023 16:38, Nicola Vetrini wrote: > Rule 2.1 states: "A project shall not contain unreachable code". > > The functions > - machine_halt > - maybe_reboot > - machine_restart > are not supposed to return, hence the following break statement > is marked as intentionally unreachable with the ASSERT_UNREACHABLE() > macro to justify the violation of the rule. During the discussion it was mentioned that this won't help with release builds, where right now ASSERT_UNREACHABLE() expands to effectively nothing. You want to clarify here how release builds are to be taken care of, as those are what eventual certification will be run against. Jan
On Thu, 3 Aug 2023, Jan Beulich wrote: > On 02.08.2023 16:38, Nicola Vetrini wrote: > > Rule 2.1 states: "A project shall not contain unreachable code". > > > > The functions > > - machine_halt > > - maybe_reboot > > - machine_restart > > are not supposed to return, hence the following break statement > > is marked as intentionally unreachable with the ASSERT_UNREACHABLE() > > macro to justify the violation of the rule. > > During the discussion it was mentioned that this won't help with > release builds, where right now ASSERT_UNREACHABLE() expands to > effectively nothing. You want to clarify here how release builds > are to be taken care of, as those are what eventual certification > will be run against. Something along these lines: ASSERT_UNREACHABLE(), not only is used in non-release builds to actually assert and detect errors, but it is also used as a marker to tag unreachable code. In release builds ASSERT_UNREACHABLE() doesn't resolve into an assert, but retains its role of a code marker. Does it work?
On 04.08.2023 01:50, Stefano Stabellini wrote: > On Thu, 3 Aug 2023, Jan Beulich wrote: >> On 02.08.2023 16:38, Nicola Vetrini wrote: >>> Rule 2.1 states: "A project shall not contain unreachable code". >>> >>> The functions >>> - machine_halt >>> - maybe_reboot >>> - machine_restart >>> are not supposed to return, hence the following break statement >>> is marked as intentionally unreachable with the ASSERT_UNREACHABLE() >>> macro to justify the violation of the rule. >> >> During the discussion it was mentioned that this won't help with >> release builds, where right now ASSERT_UNREACHABLE() expands to >> effectively nothing. You want to clarify here how release builds >> are to be taken care of, as those are what eventual certification >> will be run against. > > Something along these lines: > > ASSERT_UNREACHABLE(), not only is used in non-release builds to actually > assert and detect errors, but it is also used as a marker to tag > unreachable code. In release builds ASSERT_UNREACHABLE() doesn't resolve > into an assert, but retains its role of a code marker. > > Does it work? Well, it states what is happening, but I'm not convinced it satisfies rule 2.1. There's then still code there which isn't reachable, and which a scanner will spot and report. Jan
On 04/08/2023 08:42, Jan Beulich wrote: > On 04.08.2023 01:50, Stefano Stabellini wrote: >> On Thu, 3 Aug 2023, Jan Beulich wrote: >>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>> Rule 2.1 states: "A project shall not contain unreachable code". >>>> >>>> The functions >>>> - machine_halt >>>> - maybe_reboot >>>> - machine_restart >>>> are not supposed to return, hence the following break statement >>>> is marked as intentionally unreachable with the ASSERT_UNREACHABLE() >>>> macro to justify the violation of the rule. >>> >>> During the discussion it was mentioned that this won't help with >>> release builds, where right now ASSERT_UNREACHABLE() expands to >>> effectively nothing. You want to clarify here how release builds >>> are to be taken care of, as those are what eventual certification >>> will be run against. >> >> Something along these lines: >> >> ASSERT_UNREACHABLE(), not only is used in non-release builds to >> actually >> assert and detect errors, but it is also used as a marker to tag >> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >> resolve >> into an assert, but retains its role of a code marker. >> >> Does it work? > > Well, it states what is happening, but I'm not convinced it satisfies > rule 2.1. There's then still code there which isn't reachable, and > which a scanner will spot and report. > > Jan It's not clear to me whether you dislike the patch itself or the commit message. If it's the latter, how about: "ASSERT_UNREACHABLE() is used as a marker for intentionally unreachable code, which constitutes a motivated deviation from Rule 2.1. Additionally, in non-release builds, this macro performs a failing assertion to detect errors."
Hi, On 08/08/2023 11:03, Nicola Vetrini wrote: > On 04/08/2023 08:42, Jan Beulich wrote: >> On 04.08.2023 01:50, Stefano Stabellini wrote: >>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>> Rule 2.1 states: "A project shall not contain unreachable code". >>>>> >>>>> The functions >>>>> - machine_halt >>>>> - maybe_reboot >>>>> - machine_restart >>>>> are not supposed to return, hence the following break statement >>>>> is marked as intentionally unreachable with the >>>>> ASSERT_UNREACHABLE() >>>>> macro to justify the violation of the rule. >>>> >>>> During the discussion it was mentioned that this won't help with >>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>> effectively nothing. You want to clarify here how release builds >>>> are to be taken care of, as those are what eventual certification >>>> will be run against. >>> >>> Something along these lines: >>> >>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>> actually >>> assert and detect errors, but it is also used as a marker to tag >>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>> resolve >>> into an assert, but retains its role of a code marker. >>> >>> Does it work? >> >> Well, it states what is happening, but I'm not convinced it satisfies >> rule 2.1. There's then still code there which isn't reachable, and >> which a scanner will spot and report. >> >> Jan > > It's not clear to me whether you dislike the patch itself or the commit > message. If it's the latter, how about: > "ASSERT_UNREACHABLE() is used as a marker for intentionally > unreachable code, which > constitutes a motivated deviation from Rule 2.1. Additionally, in > non-release > builds, this macro performs a failing assertion to detect errors." Any feedback on this (with one edit: s/a failing assertion/an assertion/)
On 16.08.2023 12:01, Nicola Vetrini wrote: > Hi, > > On 08/08/2023 11:03, Nicola Vetrini wrote: >> On 04/08/2023 08:42, Jan Beulich wrote: >>> On 04.08.2023 01:50, Stefano Stabellini wrote: >>>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>>> Rule 2.1 states: "A project shall not contain unreachable code". >>>>>> >>>>>> The functions >>>>>> - machine_halt >>>>>> - maybe_reboot >>>>>> - machine_restart >>>>>> are not supposed to return, hence the following break statement >>>>>> is marked as intentionally unreachable with the >>>>>> ASSERT_UNREACHABLE() >>>>>> macro to justify the violation of the rule. >>>>> >>>>> During the discussion it was mentioned that this won't help with >>>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>>> effectively nothing. You want to clarify here how release builds >>>>> are to be taken care of, as those are what eventual certification >>>>> will be run against. >>>> >>>> Something along these lines: >>>> >>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>>> actually >>>> assert and detect errors, but it is also used as a marker to tag >>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>>> resolve >>>> into an assert, but retains its role of a code marker. >>>> >>>> Does it work? >>> >>> Well, it states what is happening, but I'm not convinced it satisfies >>> rule 2.1. There's then still code there which isn't reachable, and >>> which a scanner will spot and report. >> >> It's not clear to me whether you dislike the patch itself or the commit >> message. If it's the latter, how about: >> "ASSERT_UNREACHABLE() is used as a marker for intentionally >> unreachable code, which >> constitutes a motivated deviation from Rule 2.1. Additionally, in >> non-release >> builds, this macro performs a failing assertion to detect errors." > > Any feedback on this (with one edit: s/a failing assertion/an > assertion/) The patch here is kind of okay, but I'm afraid I view my earlier question as not addressed: How will the proposed change prevent the scanner from spotting issues here in release builds? Just stating in the description that there's a deviation is not going to help that. Jan
On 16/08/2023 12:31, Jan Beulich wrote: > On 16.08.2023 12:01, Nicola Vetrini wrote: >> Hi, >> >> On 08/08/2023 11:03, Nicola Vetrini wrote: >>> On 04/08/2023 08:42, Jan Beulich wrote: >>>> On 04.08.2023 01:50, Stefano Stabellini wrote: >>>>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>>>> Rule 2.1 states: "A project shall not contain unreachable code". >>>>>>> >>>>>>> The functions >>>>>>> - machine_halt >>>>>>> - maybe_reboot >>>>>>> - machine_restart >>>>>>> are not supposed to return, hence the following break statement >>>>>>> is marked as intentionally unreachable with the >>>>>>> ASSERT_UNREACHABLE() >>>>>>> macro to justify the violation of the rule. >>>>>> >>>>>> During the discussion it was mentioned that this won't help with >>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>>>> effectively nothing. You want to clarify here how release builds >>>>>> are to be taken care of, as those are what eventual certification >>>>>> will be run against. >>>>> >>>>> Something along these lines: >>>>> >>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>>>> actually >>>>> assert and detect errors, but it is also used as a marker to tag >>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>>>> resolve >>>>> into an assert, but retains its role of a code marker. >>>>> >>>>> Does it work? >>>> >>>> Well, it states what is happening, but I'm not convinced it >>>> satisfies >>>> rule 2.1. There's then still code there which isn't reachable, and >>>> which a scanner will spot and report. >>> >>> It's not clear to me whether you dislike the patch itself or the >>> commit >>> message. If it's the latter, how about: >>> "ASSERT_UNREACHABLE() is used as a marker for intentionally >>> unreachable code, which >>> constitutes a motivated deviation from Rule 2.1. Additionally, in >>> non-release >>> builds, this macro performs a failing assertion to detect errors." >> >> Any feedback on this (with one edit: s/a failing assertion/an >> assertion/) > > The patch here is kind of okay, but I'm afraid I view my earlier > question > as not addressed: How will the proposed change prevent the scanner from > spotting issues here in release builds? Just stating in the description > that there's a deviation is not going to help that. > > Jan There is a deviation already in place. At the moment it just ignores anything below an unreachable ASSERT_UNREACHABLE(), no matter what that macro will expand to.
On 16.08.2023 12:47, Nicola Vetrini wrote: > On 16/08/2023 12:31, Jan Beulich wrote: >> On 16.08.2023 12:01, Nicola Vetrini wrote: >>> On 08/08/2023 11:03, Nicola Vetrini wrote: >>>> On 04/08/2023 08:42, Jan Beulich wrote: >>>>> On 04.08.2023 01:50, Stefano Stabellini wrote: >>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>>>>> Rule 2.1 states: "A project shall not contain unreachable code". >>>>>>>> >>>>>>>> The functions >>>>>>>> - machine_halt >>>>>>>> - maybe_reboot >>>>>>>> - machine_restart >>>>>>>> are not supposed to return, hence the following break statement >>>>>>>> is marked as intentionally unreachable with the >>>>>>>> ASSERT_UNREACHABLE() >>>>>>>> macro to justify the violation of the rule. >>>>>>> >>>>>>> During the discussion it was mentioned that this won't help with >>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>>>>> effectively nothing. You want to clarify here how release builds >>>>>>> are to be taken care of, as those are what eventual certification >>>>>>> will be run against. >>>>>> >>>>>> Something along these lines: >>>>>> >>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>>>>> actually >>>>>> assert and detect errors, but it is also used as a marker to tag >>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>>>>> resolve >>>>>> into an assert, but retains its role of a code marker. >>>>>> >>>>>> Does it work? >>>>> >>>>> Well, it states what is happening, but I'm not convinced it >>>>> satisfies >>>>> rule 2.1. There's then still code there which isn't reachable, and >>>>> which a scanner will spot and report. >>>> >>>> It's not clear to me whether you dislike the patch itself or the >>>> commit >>>> message. If it's the latter, how about: >>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally >>>> unreachable code, which >>>> constitutes a motivated deviation from Rule 2.1. Additionally, in >>>> non-release >>>> builds, this macro performs a failing assertion to detect errors." >>> >>> Any feedback on this (with one edit: s/a failing assertion/an >>> assertion/) >> >> The patch here is kind of okay, but I'm afraid I view my earlier >> question >> as not addressed: How will the proposed change prevent the scanner from >> spotting issues here in release builds? Just stating in the description >> that there's a deviation is not going to help that. > > There is a deviation already in place. At the moment it just ignores > anything below an unreachable ASSERT_UNREACHABLE(), no matter what that > macro will expand to. What do you mean by "in place"? docs/misra/ has nothing, afaics. And automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering things out of reports, aiui. (Plus of course there should be a single central place where all deviations are recorded, imo.) Jan
On 16/08/2023 13:23, Jan Beulich wrote: > On 16.08.2023 12:47, Nicola Vetrini wrote: >> On 16/08/2023 12:31, Jan Beulich wrote: >>> On 16.08.2023 12:01, Nicola Vetrini wrote: >>>> On 08/08/2023 11:03, Nicola Vetrini wrote: >>>>> On 04/08/2023 08:42, Jan Beulich wrote: >>>>>> On 04.08.2023 01:50, Stefano Stabellini wrote: >>>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>>>>>> Rule 2.1 states: "A project shall not contain unreachable >>>>>>>>> code". >>>>>>>>> >>>>>>>>> The functions >>>>>>>>> - machine_halt >>>>>>>>> - maybe_reboot >>>>>>>>> - machine_restart >>>>>>>>> are not supposed to return, hence the following break statement >>>>>>>>> is marked as intentionally unreachable with the >>>>>>>>> ASSERT_UNREACHABLE() >>>>>>>>> macro to justify the violation of the rule. >>>>>>>> >>>>>>>> During the discussion it was mentioned that this won't help with >>>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>>>>>> effectively nothing. You want to clarify here how release builds >>>>>>>> are to be taken care of, as those are what eventual >>>>>>>> certification >>>>>>>> will be run against. >>>>>>> >>>>>>> Something along these lines: >>>>>>> >>>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>>>>>> actually >>>>>>> assert and detect errors, but it is also used as a marker to tag >>>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>>>>>> resolve >>>>>>> into an assert, but retains its role of a code marker. >>>>>>> >>>>>>> Does it work? >>>>>> >>>>>> Well, it states what is happening, but I'm not convinced it >>>>>> satisfies >>>>>> rule 2.1. There's then still code there which isn't reachable, and >>>>>> which a scanner will spot and report. >>>>> >>>>> It's not clear to me whether you dislike the patch itself or the >>>>> commit >>>>> message. If it's the latter, how about: >>>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally >>>>> unreachable code, which >>>>> constitutes a motivated deviation from Rule 2.1. Additionally, in >>>>> non-release >>>>> builds, this macro performs a failing assertion to detect errors." >>>> >>>> Any feedback on this (with one edit: s/a failing assertion/an >>>> assertion/) >>> >>> The patch here is kind of okay, but I'm afraid I view my earlier >>> question >>> as not addressed: How will the proposed change prevent the scanner >>> from >>> spotting issues here in release builds? Just stating in the >>> description >>> that there's a deviation is not going to help that. >> >> There is a deviation already in place. At the moment it just ignores >> anything below an unreachable ASSERT_UNREACHABLE(), no matter what >> that >> macro will expand to. > > What do you mean by "in place"? docs/misra/ has nothing, afaics. And > automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering > things out of reports, aiui. (Plus of course there should be a single > central place where all deviations are recorded, imo.) The second statement is not quite correct, as some of the configurations instruct the checker how to behave.
On 16.08.2023 15:43, Nicola Vetrini wrote: > On 16/08/2023 13:23, Jan Beulich wrote: >> On 16.08.2023 12:47, Nicola Vetrini wrote: >>> On 16/08/2023 12:31, Jan Beulich wrote: >>>> On 16.08.2023 12:01, Nicola Vetrini wrote: >>>>> On 08/08/2023 11:03, Nicola Vetrini wrote: >>>>>> On 04/08/2023 08:42, Jan Beulich wrote: >>>>>>> On 04.08.2023 01:50, Stefano Stabellini wrote: >>>>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>>>>>>> Rule 2.1 states: "A project shall not contain unreachable >>>>>>>>>> code". >>>>>>>>>> >>>>>>>>>> The functions >>>>>>>>>> - machine_halt >>>>>>>>>> - maybe_reboot >>>>>>>>>> - machine_restart >>>>>>>>>> are not supposed to return, hence the following break statement >>>>>>>>>> is marked as intentionally unreachable with the >>>>>>>>>> ASSERT_UNREACHABLE() >>>>>>>>>> macro to justify the violation of the rule. >>>>>>>>> >>>>>>>>> During the discussion it was mentioned that this won't help with >>>>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>>>>>>> effectively nothing. You want to clarify here how release builds >>>>>>>>> are to be taken care of, as those are what eventual >>>>>>>>> certification >>>>>>>>> will be run against. >>>>>>>> >>>>>>>> Something along these lines: >>>>>>>> >>>>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>>>>>>> actually >>>>>>>> assert and detect errors, but it is also used as a marker to tag >>>>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>>>>>>> resolve >>>>>>>> into an assert, but retains its role of a code marker. >>>>>>>> >>>>>>>> Does it work? >>>>>>> >>>>>>> Well, it states what is happening, but I'm not convinced it >>>>>>> satisfies >>>>>>> rule 2.1. There's then still code there which isn't reachable, and >>>>>>> which a scanner will spot and report. >>>>>> >>>>>> It's not clear to me whether you dislike the patch itself or the >>>>>> commit >>>>>> message. If it's the latter, how about: >>>>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally >>>>>> unreachable code, which >>>>>> constitutes a motivated deviation from Rule 2.1. Additionally, in >>>>>> non-release >>>>>> builds, this macro performs a failing assertion to detect errors." >>>>> >>>>> Any feedback on this (with one edit: s/a failing assertion/an >>>>> assertion/) >>>> >>>> The patch here is kind of okay, but I'm afraid I view my earlier >>>> question >>>> as not addressed: How will the proposed change prevent the scanner >>>> from >>>> spotting issues here in release builds? Just stating in the >>>> description >>>> that there's a deviation is not going to help that. >>> >>> There is a deviation already in place. At the moment it just ignores >>> anything below an unreachable ASSERT_UNREACHABLE(), no matter what >>> that >>> macro will expand to. >> >> What do you mean by "in place"? docs/misra/ has nothing, afaics. And >> automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering >> things out of reports, aiui. (Plus of course there should be a single >> central place where all deviations are recorded, imo.) > > The second statement is not quite correct, as some of the configurations > instruct the > checker how to behave. Well, I was referring to the one setting relevant here, and I added "aiui" to indicate that I may be misreading what that file specifies. Jan
On Wed, 16 Aug 2023, Jan Beulich wrote: > On 16.08.2023 12:47, Nicola Vetrini wrote: > > On 16/08/2023 12:31, Jan Beulich wrote: > >> On 16.08.2023 12:01, Nicola Vetrini wrote: > >>> On 08/08/2023 11:03, Nicola Vetrini wrote: > >>>> On 04/08/2023 08:42, Jan Beulich wrote: > >>>>> On 04.08.2023 01:50, Stefano Stabellini wrote: > >>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote: > >>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: > >>>>>>>> Rule 2.1 states: "A project shall not contain unreachable code". > >>>>>>>> > >>>>>>>> The functions > >>>>>>>> - machine_halt > >>>>>>>> - maybe_reboot > >>>>>>>> - machine_restart > >>>>>>>> are not supposed to return, hence the following break statement > >>>>>>>> is marked as intentionally unreachable with the > >>>>>>>> ASSERT_UNREACHABLE() > >>>>>>>> macro to justify the violation of the rule. > >>>>>>> > >>>>>>> During the discussion it was mentioned that this won't help with > >>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to > >>>>>>> effectively nothing. You want to clarify here how release builds > >>>>>>> are to be taken care of, as those are what eventual certification > >>>>>>> will be run against. > >>>>>> > >>>>>> Something along these lines: > >>>>>> > >>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to > >>>>>> actually > >>>>>> assert and detect errors, but it is also used as a marker to tag > >>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't > >>>>>> resolve > >>>>>> into an assert, but retains its role of a code marker. > >>>>>> > >>>>>> Does it work? > >>>>> > >>>>> Well, it states what is happening, but I'm not convinced it > >>>>> satisfies > >>>>> rule 2.1. There's then still code there which isn't reachable, and > >>>>> which a scanner will spot and report. > >>>> > >>>> It's not clear to me whether you dislike the patch itself or the > >>>> commit > >>>> message. If it's the latter, how about: > >>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally > >>>> unreachable code, which > >>>> constitutes a motivated deviation from Rule 2.1. Additionally, in > >>>> non-release > >>>> builds, this macro performs a failing assertion to detect errors." > >>> > >>> Any feedback on this (with one edit: s/a failing assertion/an > >>> assertion/) > >> > >> The patch here is kind of okay, but I'm afraid I view my earlier > >> question > >> as not addressed: How will the proposed change prevent the scanner from > >> spotting issues here in release builds? Just stating in the description > >> that there's a deviation is not going to help that. > > > > There is a deviation already in place. At the moment it just ignores > > anything below an unreachable ASSERT_UNREACHABLE(), no matter what that > > macro will expand to. > > What do you mean by "in place"? docs/misra/ has nothing, afaics. And > automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering > things out of reports, aiui. (Plus of course there should be a single > central place where all deviations are recorded, imo.) Jan has a point: I think we should record all our deviations and unique ways to interpret the rules under docs/misra. And the Eclair configuration should reflect that. It is not a good idea to only keep the information in the Eclair config because, even if it is now upstream in xen.git, it is still a tool-specific configuration file -- it is not a proper document. MISRA C and its interpretation is important enough that we should have a plain English document to cover it (which now is docs/misra/rules.rst). Nicola, I volunteer to send patches to make any necessary updates to docs/misra/rules.rst and other docs. Please send out to me a list of deviations/configurations and I'll make sure to reflect them under docs/misra so that they are in sync. Cheers, Stefano
> Jan has a point: I think we should record all our deviations and unique > ways to interpret the rules under docs/misra. And the Eclair > configuration should reflect that. It is not a good idea to only keep > the information in the Eclair config because, even if it is now > upstream > in xen.git, it is still a tool-specific configuration file -- it is not > a proper document. MISRA C and its interpretation is important enough > that we should have a plain English document to cover it (which now is > docs/misra/rules.rst). > > Nicola, I volunteer to send patches to make any necessary updates to > docs/misra/rules.rst and other docs. Please send out to me a list of > deviations/configurations and I'll make sure to reflect them under > docs/misra so that they are in sync. > > Cheers, > > Stefano Sure, I'll let you know when I have it. Thanks,
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c index a933ee001e..88f735a30e 100644 --- a/xen/common/shutdown.c +++ b/xen/common/shutdown.c @@ -38,39 +38,45 @@ void hwdom_shutdown(u8 reason) printk("Hardware Dom%u halted: halting machine\n", hardware_domain->domain_id); machine_halt(); - break; /* not reached */ + ASSERT_UNREACHABLE(); + break; case SHUTDOWN_crash: debugger_trap_immediate(); printk("Hardware Dom%u crashed: ", hardware_domain->domain_id); kexec_crash(CRASHREASON_HWDOM); maybe_reboot(); - break; /* not reached */ + ASSERT_UNREACHABLE(); + break; case SHUTDOWN_reboot: printk("Hardware Dom%u shutdown: rebooting machine\n", hardware_domain->domain_id); machine_restart(0); - break; /* not reached */ + ASSERT_UNREACHABLE(); + break; case SHUTDOWN_watchdog: printk("Hardware Dom%u shutdown: watchdog rebooting machine\n", hardware_domain->domain_id); kexec_crash(CRASHREASON_WATCHDOG); machine_restart(0); - break; /* not reached */ + ASSERT_UNREACHABLE(); + break; case SHUTDOWN_soft_reset: printk("Hardware domain %d did unsupported soft reset, rebooting.\n", hardware_domain->domain_id); machine_restart(0); - break; /* not reached */ + ASSERT_UNREACHABLE(); + break; default: printk("Hardware Dom%u shutdown (unknown reason %u): ", hardware_domain->domain_id, reason); maybe_reboot(); - break; /* not reached */ + ASSERT_UNREACHABLE(); + break; } }
Rule 2.1 states: "A project shall not contain unreachable code". The functions - machine_halt - maybe_reboot - machine_restart are not supposed to return, hence the following break statement is marked as intentionally unreachable with the ASSERT_UNREACHABLE() macro to justify the violation of the rule. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/common/shutdown.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)