| Message ID | 394b69b769f2dc2461d2ddb0c7e037f4794eb244.1702283415.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 2.1 | expand |
Hi, On 11/12/2023 10:30, Nicola Vetrini wrote: > The branches of the switch after a call to 'do_unexpected_trap' > cannot return, but there is one path that may return, hence > only some clauses are marked with ASSERT_UNREACHABLE(). I don't understand why this is necessary. The code should never be reachable because do_unexpected_trap() is a noreturn(). Cheers,
On 2023-12-11 13:32, Julien Grall wrote: > Hi, > > On 11/12/2023 10:30, Nicola Vetrini wrote: >> The branches of the switch after a call to 'do_unexpected_trap' >> cannot return, but there is one path that may return, hence >> only some clauses are marked with ASSERT_UNREACHABLE(). > I don't understand why this is necessary. The code should never be > reachable because do_unexpected_trap() is a noreturn(). > > Cheers, It was meant as a safeguard against mistakes. There are MISRA rules that deal with this aspect (e.g., a noreturn function should not return), but they are not in Amendment 2, which is what Xen is following.
Hi Nicola, On 11/12/2023 14:54, Nicola Vetrini wrote: > On 2023-12-11 13:32, Julien Grall wrote: >> Hi, >> >> On 11/12/2023 10:30, Nicola Vetrini wrote: >>> The branches of the switch after a call to 'do_unexpected_trap' >>> cannot return, but there is one path that may return, hence >>> only some clauses are marked with ASSERT_UNREACHABLE(). >> I don't understand why this is necessary. The code should never be >> reachable because do_unexpected_trap() is a noreturn(). >> >> Cheers, > > It was meant as a safeguard against mistakes. I am confused about which mistakes you are trying to prevent. Are you saying you are not trusting the noreturn attribute? And if so, are you intending to add ASSERT_UNREACHABLE() after every single call to noreturn functions? > There are MISRA rules that > deal with this aspect (e.g., a noreturn function should not return), but > they are not in Amendment 2, which is what Xen is following. Do you mind providing a bit more details about the rules you are talking about? At least the numbers would be helpful. Cheers,
On 11/12/2023 15:59, Julien Grall wrote: > Hi Nicola, > > On 11/12/2023 14:54, Nicola Vetrini wrote: >> On 2023-12-11 13:32, Julien Grall wrote: >>> Hi, >>> >>> On 11/12/2023 10:30, Nicola Vetrini wrote: >>>> The branches of the switch after a call to 'do_unexpected_trap' >>>> cannot return, but there is one path that may return, hence >>>> only some clauses are marked with ASSERT_UNREACHABLE(). >>> I don't understand why this is necessary. The code should never be >>> reachable because do_unexpected_trap() is a noreturn(). >>> >>> Cheers, >> >> It was meant as a safeguard against mistakes. > > I am confused about which mistakes you are trying to prevent. Are you > saying you are not trusting the noreturn attribute? > > And if so, are you intending to add ASSERT_UNREACHABLE() after every > single call to noreturn functions? Replying to myself. What's confusing the most is that in [1], you decided to not add the ASSERT_UNREACHABLE(). Yet the problem is similar. I'd also like to point out that by removing the "break", then if the 'noreturn' function turns out to return, then in prod build you would fallthrough to the next case. And who knows what's going to happen... All of this really adds some confusion... > >> There are MISRA rules that deal with this aspect (e.g., a noreturn >> function should not return), but they are not in Amendment 2, which is >> what Xen is following. > Do you mind providing a bit more details about the rules you are talking > about? At least the numbers would be helpful. [1] https://lore.kernel.org/xen-devel/c0a8a12e39d688e101936d221af0f8eeefabe352.1702283415.git.nicola.vetrini@bugseng.com/ Cheers,
On 2023-12-11 17:05, Julien Grall wrote: > On 11/12/2023 15:59, Julien Grall wrote: >> Hi Nicola, >> >> On 11/12/2023 14:54, Nicola Vetrini wrote: >>> On 2023-12-11 13:32, Julien Grall wrote: >>>> Hi, >>>> >>>> On 11/12/2023 10:30, Nicola Vetrini wrote: >>>>> The branches of the switch after a call to 'do_unexpected_trap' >>>>> cannot return, but there is one path that may return, hence >>>>> only some clauses are marked with ASSERT_UNREACHABLE(). >>>> I don't understand why this is necessary. The code should never be >>>> reachable because do_unexpected_trap() is a noreturn(). >>>> >>>> Cheers, >>> >>> It was meant as a safeguard against mistakes. >> >> I am confused about which mistakes you are trying to prevent. Are you >> saying you are not trusting the noreturn attribute? >> >> And if so, are you intending to add ASSERT_UNREACHABLE() after every >> single call to noreturn functions? > > Replying to myself. What's confusing the most is that in [1], you > decided to not add the ASSERT_UNREACHABLE(). Yet the problem is > similar. > > I'd also like to point out that by removing the "break", then if the > 'noreturn' function turns out to return, then in prod build you would > fallthrough to the next case. And who knows what's going to happen... > > All of this really adds some confusion... > I should have checked before responding: do_trap_hyp_sync is not afaik noreturn. Specifically, do_trap_brk may return. If I worked under the wrong assumption, then certainly the ASSERT_UNREACHABLE-s should be dropped. >> >>> There are MISRA rules that deal with this aspect (e.g., a noreturn >>> function should not return), but they are not in Amendment 2, which >>> is what Xen is following. >> Do you mind providing a bit more details about the rules you are >> talking about? At least the numbers would be helpful. > > [1] > https://lore.kernel.org/xen-devel/c0a8a12e39d688e101936d221af0f8eeefabe352.1702283415.git.nicola.vetrini@bugseng.com/ > > Cheers, Sure. Here are the rules concerned with _Noreturn: R17.9: A function declared with a _Noreturn function specifier shall not return to its caller R17.10: A function declared with a _Noreturn function specifier shall have void return type R17.11: A function that never returns should be declared with a _Noreturn function specifier
On Mon, 11 Dec 2023, Nicola Vetrini wrote: > On 2023-12-11 17:05, Julien Grall wrote: > > On 11/12/2023 15:59, Julien Grall wrote: > > > Hi Nicola, > > > > > > On 11/12/2023 14:54, Nicola Vetrini wrote: > > > > On 2023-12-11 13:32, Julien Grall wrote: > > > > > Hi, > > > > > > > > > > On 11/12/2023 10:30, Nicola Vetrini wrote: > > > > > > The branches of the switch after a call to 'do_unexpected_trap' > > > > > > cannot return, but there is one path that may return, hence > > > > > > only some clauses are marked with ASSERT_UNREACHABLE(). > > > > > I don't understand why this is necessary. The code should never be > > > > > reachable because do_unexpected_trap() is a noreturn(). > > > > > > > > > > Cheers, > > > > > > > > It was meant as a safeguard against mistakes. > > > > > > I am confused about which mistakes you are trying to prevent. Are you > > > saying you are not trusting the noreturn attribute? > > > > > > And if so, are you intending to add ASSERT_UNREACHABLE() after every > > > single call to noreturn functions? > > > > Replying to myself. What's confusing the most is that in [1], you decided to > > not add the ASSERT_UNREACHABLE(). Yet the problem is similar. > > > > I'd also like to point out that by removing the "break", then if the > > 'noreturn' function turns out to return, then in prod build you would > > fallthrough to the next case. And who knows what's going to happen... > > > > All of this really adds some confusion... > > > > I should have checked before responding: do_trap_hyp_sync is not afaik > noreturn. Specifically, do_trap_brk may return. If I worked under the wrong > assumption, then certainly the ASSERT_UNREACHABLE-s should be dropped. It looks like we could add noreturn to do_trap_brk. Julien what do you think?
Hi Stefano, On 12/12/2023 01:36, Stefano Stabellini wrote: > On Mon, 11 Dec 2023, Nicola Vetrini wrote: >> On 2023-12-11 17:05, Julien Grall wrote: >>> On 11/12/2023 15:59, Julien Grall wrote: >>>> Hi Nicola, >>>> >>>> On 11/12/2023 14:54, Nicola Vetrini wrote: >>>>> On 2023-12-11 13:32, Julien Grall wrote: >>>>>> Hi, >>>>>> >>>>>> On 11/12/2023 10:30, Nicola Vetrini wrote: >>>>>>> The branches of the switch after a call to 'do_unexpected_trap' >>>>>>> cannot return, but there is one path that may return, hence >>>>>>> only some clauses are marked with ASSERT_UNREACHABLE(). >>>>>> I don't understand why this is necessary. The code should never be >>>>>> reachable because do_unexpected_trap() is a noreturn(). >>>>>> >>>>>> Cheers, >>>>> >>>>> It was meant as a safeguard against mistakes. >>>> >>>> I am confused about which mistakes you are trying to prevent. Are you >>>> saying you are not trusting the noreturn attribute? >>>> >>>> And if so, are you intending to add ASSERT_UNREACHABLE() after every >>>> single call to noreturn functions? >>> >>> Replying to myself. What's confusing the most is that in [1], you decided to >>> not add the ASSERT_UNREACHABLE(). Yet the problem is similar. >>> >>> I'd also like to point out that by removing the "break", then if the >>> 'noreturn' function turns out to return, then in prod build you would >>> fallthrough to the next case. And who knows what's going to happen... >>> >>> All of this really adds some confusion... >>> >> >> I should have checked before responding: do_trap_hyp_sync is not afaik >> noreturn. Specifically, do_trap_brk may return. If I worked under the wrong >> assumption, then certainly the ASSERT_UNREACHABLE-s should be dropped. > > It looks like we could add noreturn to do_trap_brk. Julien what do you > think? You can't add noreturn to do_trap_brk(). It is used to handle WARN_ON() that *will* return. Cheers,
Hi, On 11/12/2023 12:32, Julien Grall wrote: > Hi, > > On 11/12/2023 10:30, Nicola Vetrini wrote: >> The branches of the switch after a call to 'do_unexpected_trap' >> cannot return, but there is one path that may return, hence >> only some clauses are marked with ASSERT_UNREACHABLE(). > I don't understand why this is necessary. The code should never be > reachable because do_unexpected_trap() is a noreturn(). From the matrix discussion, it wasn't clear what was my position on this patch. I would much prefer if the breaks are kept. I could accept: ASSERT_UNREACHABLE(); break; But this solution is a Nack because if you are concerned about functions like do_unexpected_trap() to return by mistaken, then it needs to also be safe in production. The current proposal is not safe. Cheers,
On 2023-12-12 16:49, Julien Grall wrote: > Hi, > > On 11/12/2023 12:32, Julien Grall wrote: >> Hi, >> >> On 11/12/2023 10:30, Nicola Vetrini wrote: >>> The branches of the switch after a call to 'do_unexpected_trap' >>> cannot return, but there is one path that may return, hence >>> only some clauses are marked with ASSERT_UNREACHABLE(). >> I don't understand why this is necessary. The code should never be >> reachable because do_unexpected_trap() is a noreturn(). > > From the matrix discussion, it wasn't clear what was my position on > this patch. > > I would much prefer if the breaks are kept. I could accept: > > ASSERT_UNREACHABLE(); > break; > > But this solution is a Nack because if you are concerned about > functions like do_unexpected_trap() to return by mistaken, then it > needs to also be safe in production. > > The current proposal is not safe. > > Cheers, Ok. I wonder whether the should be applied here in vcpreg.c: diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c index 39aeda9dab62..089d2f03eb5e 100644 --- a/xen/arch/arm/vcpreg.c +++ b/xen/arch/arm/vcpreg.c @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr hsr) inject_undef_exception(regs, hsr); return; } - + + ASSERT_UNREACHABLE(); advance_pc(regs, hsr); } the rationale being that, should the switch somehow fail to return, the advance_pc would be called, rather than doing nothing.
Hi, On 13/12/2023 14:02, Nicola Vetrini wrote: > On 2023-12-12 16:49, Julien Grall wrote: >> Hi, >> >> On 11/12/2023 12:32, Julien Grall wrote: >>> Hi, >>> >>> On 11/12/2023 10:30, Nicola Vetrini wrote: >>>> The branches of the switch after a call to 'do_unexpected_trap' >>>> cannot return, but there is one path that may return, hence >>>> only some clauses are marked with ASSERT_UNREACHABLE(). >>> I don't understand why this is necessary. The code should never be >>> reachable because do_unexpected_trap() is a noreturn(). >> >> From the matrix discussion, it wasn't clear what was my position on >> this patch. >> >> I would much prefer if the breaks are kept. I could accept: >> >> ASSERT_UNREACHABLE(); >> break; >> >> But this solution is a Nack because if you are concerned about >> functions like do_unexpected_trap() to return by mistaken, then it >> needs to also be safe in production. >> >> The current proposal is not safe. >> >> Cheers, > > Ok. I wonder whether the should be applied here in vcpreg.c: > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > index 39aeda9dab62..089d2f03eb5e 100644 > --- a/xen/arch/arm/vcpreg.c > +++ b/xen/arch/arm/vcpreg.c > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union > hsr hsr) > inject_undef_exception(regs, hsr); > return; > } > - > + > + ASSERT_UNREACHABLE(); > advance_pc(regs, hsr); > } > > the rationale being that, should the switch somehow fail to return, the > advance_pc would be called, rather than doing nothing. To clarify, advance_pc(regs, hsr) would still be called in production build. So if you are concerned about advance_pc() been called, then adding an ASSERT_UNREACHABLE() is not going to help. It took me a little while to confirm that none of the path effectively returns due to the macros (in hindsight, it wasn't a good idea of mine to introduce them). Depending on what we are trying to solve there are 3 possible approach: 1. Leave advance_pc(). This means we could potentially a. Advance the PC twice (if it was already called) and therefore skipping an instruction b. Advance the PC once without an emulation 2. Remove advance_pc(). If we already called the function, then there is no problem. Otherwise, we would trap in a loop effectively rendering the guest vCPU unusable. 3. Replace with domain_crash() Here it feels, that 3 is more suitable as this gives a clear indication why/where the emulation gone wrong. This may still need to be accompanied with a ASSERT_UNREACHABLE() to please MISRA. Bertrand, Michal, Stefano, what do you think? Cheers,
On Thu, 14 Dec 2023, Julien Grall wrote: > Hi, > > On 13/12/2023 14:02, Nicola Vetrini wrote: > > On 2023-12-12 16:49, Julien Grall wrote: > > > Hi, > > > > > > On 11/12/2023 12:32, Julien Grall wrote: > > > > Hi, > > > > > > > > On 11/12/2023 10:30, Nicola Vetrini wrote: > > > > > The branches of the switch after a call to 'do_unexpected_trap' > > > > > cannot return, but there is one path that may return, hence > > > > > only some clauses are marked with ASSERT_UNREACHABLE(). > > > > I don't understand why this is necessary. The code should never be > > > > reachable because do_unexpected_trap() is a noreturn(). > > > > > > From the matrix discussion, it wasn't clear what was my position on this > > > patch. > > > > > > I would much prefer if the breaks are kept. I could accept: > > > > > > ASSERT_UNREACHABLE(); > > > break; > > > > > > But this solution is a Nack because if you are concerned about functions > > > like do_unexpected_trap() to return by mistaken, then it needs to also be > > > safe in production. > > > > > > The current proposal is not safe. I re-read the email thread. I also do not think that this is useful: do_unexpected_trap("SVE trap at EL2", regs); - break; + ASSERT_UNREACHABLE(); I also do not think that we should be concerned about functions like do_unexpected_trap() to return by mistaken. That said, what is the problem from MISRA point of view that we are trying to fix? Is the only problem the presence of break; after the call to a noreturn function? If that's not allowed, I would suggest to do this: do_unexpected_trap("SVE trap at EL2", regs); - break; + /* break; */ Or deviate "break" globally as it doesn't seem to be a safety risk in my opinion. If nothing else, it should make the code a bit safer because in case of mistakes in do_unexpected_trap, at least we would continue to follow a more reasonable code path rather than blindly falling through the next switch case by accident. > > Ok. I wonder whether the should be applied here in vcpreg.c: > > > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > > index 39aeda9dab62..089d2f03eb5e 100644 > > --- a/xen/arch/arm/vcpreg.c > > +++ b/xen/arch/arm/vcpreg.c > > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr > > hsr) > > inject_undef_exception(regs, hsr); > > return; > > } > > - > > + > > + ASSERT_UNREACHABLE(); > > advance_pc(regs, hsr); > > } > > > > the rationale being that, should the switch somehow fail to return, the > > advance_pc would be called, rather than doing nothing. > > To clarify, advance_pc(regs, hsr) would still be called in production build. > So if you are concerned about advance_pc() been called, then adding an > ASSERT_UNREACHABLE() is not going to help. > > It took me a little while to confirm that none of the path effectively returns > due to the macros (in hindsight, it wasn't a good idea of mine to introduce > them). > > Depending on what we are trying to solve there are 3 possible approach: > 1. Leave advance_pc(). This means we could potentially > a. Advance the PC twice (if it was already called) and therefore skipping > an instruction > b. Advance the PC once without an emulation > 2. Remove advance_pc(). If we already called the function, then there is no > problem. Otherwise, we would trap in a loop effectively rendering the guest > vCPU unusable. > 3. Replace with domain_crash() > > Here it feels, that 3 is more suitable as this gives a clear indication > why/where the emulation gone wrong. > > This may still need to be accompanied with a ASSERT_UNREACHABLE() to please > MISRA. > > Bertrand, Michal, Stefano, what do you think? Yes, I would go with 3., replace advance_pc with domain_crash. Assuming that it would also solve the violation in ECLAIR.
On 2023-12-14 23:32, Stefano Stabellini wrote: > On Thu, 14 Dec 2023, Julien Grall wrote: >> Hi, >> >> On 13/12/2023 14:02, Nicola Vetrini wrote: >> > On 2023-12-12 16:49, Julien Grall wrote: >> > > Hi, >> > > >> > > On 11/12/2023 12:32, Julien Grall wrote: >> > > > Hi, >> > > > >> > > > On 11/12/2023 10:30, Nicola Vetrini wrote: >> > > > > The branches of the switch after a call to 'do_unexpected_trap' >> > > > > cannot return, but there is one path that may return, hence >> > > > > only some clauses are marked with ASSERT_UNREACHABLE(). >> > > > I don't understand why this is necessary. The code should never be >> > > > reachable because do_unexpected_trap() is a noreturn(). >> > > >> > > From the matrix discussion, it wasn't clear what was my position on this >> > > patch. >> > > >> > > I would much prefer if the breaks are kept. I could accept: >> > > >> > > ASSERT_UNREACHABLE(); >> > > break; >> > > >> > > But this solution is a Nack because if you are concerned about functions >> > > like do_unexpected_trap() to return by mistaken, then it needs to also be >> > > safe in production. >> > > >> > > The current proposal is not safe. > > I re-read the email thread. I also do not think that this is useful: > > do_unexpected_trap("SVE trap at EL2", regs); > - break; > + ASSERT_UNREACHABLE(); > > I also do not think that we should be concerned about functions like > do_unexpected_trap() to return by mistaken. > > That said, what is the problem from MISRA point of view that we are > trying to fix? Is the only problem the presence of break; after the > call > to a noreturn function? > > If that's not allowed, I would suggest to do this: > > > do_unexpected_trap("SVE trap at EL2", regs); > - break; > + /* break; */ > > > Or deviate "break" globally as it doesn't seem to be a safety risk in > my > opinion. If nothing else, it should make the code a bit safer because > in > case of mistakes in do_unexpected_trap, at least we would continue to > follow a more reasonable code path rather than blindly falling through > the next switch case by accident. > > That doesn't seem like a good idea to deviate just "break". However, Julien's earlier proposal ASSERT_UNREACHABLE(); break; is ok, though it could be shrunk in a macro #define unreachable_break ASSERT_UNREACHABLE(); break; or just #define unreachable_break break; so that "unreachable_break" can be deviated. >> > Ok. I wonder whether the should be applied here in vcpreg.c: >> > >> > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >> > index 39aeda9dab62..089d2f03eb5e 100644 >> > --- a/xen/arch/arm/vcpreg.c >> > +++ b/xen/arch/arm/vcpreg.c >> > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr >> > hsr) >> > inject_undef_exception(regs, hsr); >> > return; >> > } >> > - >> > + >> > + ASSERT_UNREACHABLE(); >> > advance_pc(regs, hsr); >> > } >> > >> > the rationale being that, should the switch somehow fail to return, the >> > advance_pc would be called, rather than doing nothing. >> >> To clarify, advance_pc(regs, hsr) would still be called in production >> build. >> So if you are concerned about advance_pc() been called, then adding an >> ASSERT_UNREACHABLE() is not going to help. >> >> It took me a little while to confirm that none of the path effectively >> returns >> due to the macros (in hindsight, it wasn't a good idea of mine to >> introduce >> them). >> >> Depending on what we are trying to solve there are 3 possible >> approach: >> 1. Leave advance_pc(). This means we could potentially >> a. Advance the PC twice (if it was already called) and therefore >> skipping >> an instruction >> b. Advance the PC once without an emulation >> 2. Remove advance_pc(). If we already called the function, then >> there is no >> problem. Otherwise, we would trap in a loop effectively rendering the >> guest >> vCPU unusable. >> 3. Replace with domain_crash() >> >> Here it feels, that 3 is more suitable as this gives a clear >> indication >> why/where the emulation gone wrong. >> >> This may still need to be accompanied with a ASSERT_UNREACHABLE() to >> please >> MISRA. >> >> Bertrand, Michal, Stefano, what do you think? > > Yes, I would go with 3., replace advance_pc with domain_crash. Assuming > that it would also solve the violation in ECLAIR. It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because it's still a violation if there is no execution path leading to domain_crash(), but other than that it seems the safest choice.
Hi Julien, >> Yes, I would go with 3., replace advance_pc with domain_crash. >> Assuming >> that it would also solve the violation in ECLAIR. > > It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because > it's still a violation if there is no execution path leading to > domain_crash(), but other than that it seems the safest choice. Assuming there are no objections to going forward with this proposal, would you mind telling me how can I do the proper domain_crash call. Most of the examples get a "struct domain *" from a parameter or from the macro "current", so I was thinking of domain_crash(current->domain); but I'm not so sure about this, as there are no other uses in vcpreg.c. You can also submit the patch yourself, if you prefer.
Hi, On 15/12/2023 14:08, Nicola Vetrini wrote: >>> Yes, I would go with 3., replace advance_pc with domain_crash. Assuming >>> that it would also solve the violation in ECLAIR. >> >> It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because >> it's still a violation if there is no execution path leading to >> domain_crash(), but other than that it seems the safest choice. > > Assuming there are no objections to going forward with this proposal, > would you mind telling me how can I do the proper domain_crash call. > Most of the examples get a "struct domain *" from a parameter or from > the macro "current", so I was thinking of > > domain_crash(current->domain); > > but I'm not so sure about this, as there are no other uses in vcpreg.c. That would be correct. All the helpers in vcpreg.c are meant to work on the current vCPU. > You can also submit the patch yourself, if you prefer. I am not entirely sure about which justification you want to use for MISRA. So here the diff: diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c index 39aeda9dab62..485b3cb63c86 100644 --- a/xen/arch/arm/vcpreg.c +++ b/xen/arch/arm/vcpreg.c @@ -708,7 +708,13 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr hsr) return; } - advance_pc(regs, hsr); + /* + * All the cases in the switch should return. If this is not the + * case, then something went wrong and it is best to crash the + * domain. + */ + ASSERT_UNREACHABLE(); + domain_crash(current->domain); } void do_cp(struct cpu_user_regs *regs, const union hsr hsr) Cheers,
On Fri, 15 Dec 2023, Nicola Vetrini wrote: > On 2023-12-14 23:32, Stefano Stabellini wrote: > > On Thu, 14 Dec 2023, Julien Grall wrote: > > > Hi, > > > > > > On 13/12/2023 14:02, Nicola Vetrini wrote: > > > > On 2023-12-12 16:49, Julien Grall wrote: > > > > > Hi, > > > > > > > > > > On 11/12/2023 12:32, Julien Grall wrote: > > > > > > Hi, > > > > > > > > > > > > On 11/12/2023 10:30, Nicola Vetrini wrote: > > > > > > > The branches of the switch after a call to 'do_unexpected_trap' > > > > > > > cannot return, but there is one path that may return, hence > > > > > > > only some clauses are marked with ASSERT_UNREACHABLE(). > > > > > > I don't understand why this is necessary. The code should never be > > > > > > reachable because do_unexpected_trap() is a noreturn(). > > > > > > > > > > From the matrix discussion, it wasn't clear what was my position on > > > this > > > > > patch. > > > > > > > > > > I would much prefer if the breaks are kept. I could accept: > > > > > > > > > > ASSERT_UNREACHABLE(); > > > > > break; > > > > > > > > > > But this solution is a Nack because if you are concerned about > > > functions > > > > > like do_unexpected_trap() to return by mistaken, then it needs to also > > > be > > > > > safe in production. > > > > > > > > > > The current proposal is not safe. > > > > I re-read the email thread. I also do not think that this is useful: > > > > do_unexpected_trap("SVE trap at EL2", regs); > > - break; > > + ASSERT_UNREACHABLE(); > > > > I also do not think that we should be concerned about functions like > > do_unexpected_trap() to return by mistaken. > > > > That said, what is the problem from MISRA point of view that we are > > trying to fix? Is the only problem the presence of break; after the call > > to a noreturn function? > > > > If that's not allowed, I would suggest to do this: > > > > > > do_unexpected_trap("SVE trap at EL2", regs); > > - break; > > + /* break; */ > > > > > > Or deviate "break" globally as it doesn't seem to be a safety risk in my > > opinion. If nothing else, it should make the code a bit safer because in > > case of mistakes in do_unexpected_trap, at least we would continue to > > follow a more reasonable code path rather than blindly falling through > > the next switch case by accident. > > > > > > That doesn't seem like a good idea to deviate just "break". However, Julien's > earlier proposal > > ASSERT_UNREACHABLE(); > break; > > is ok, though it could be shrunk in a macro > > #define unreachable_break ASSERT_UNREACHABLE(); break; > > or just > > #define unreachable_break break; > > so that "unreachable_break" can be deviated. Let's just go with: ASSERT_UNREACHABLE(); break; If Julien is OK with it too. Just to make sure we are all aligned on what the problem and solution are: - we are not concerned that do_unexpected_trap could return - it is just forbidden to have any code after something that doesn't return - not even a break; - so we need to use ASSERT_UNREACHABLE() as a marker to tell ECLAIR to ignore the violation
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 3784e8276ef6..e7c920b043d2 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2152,7 +2152,7 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) case HSR_EC_SVE: /* An SVE exception is a bug somewhere in hypervisor code */ do_unexpected_trap("SVE trap at EL2", regs); - break; + ASSERT_UNREACHABLE(); #endif case HSR_EC_DATA_ABORT_CURR_EL: case HSR_EC_INSTR_ABORT_CURR_EL: @@ -2171,13 +2171,13 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) dump_hyp_walk(get_hfar(is_data)); do_unexpected_trap(fault, regs); - - break; + ASSERT_UNREACHABLE(); } default: printk("Hypervisor Trap. HSR=%#"PRIregister" EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n", hsr.bits, hsr.ec, hsr.len, hsr.iss); do_unexpected_trap("Hypervisor", regs); + ASSERT_UNREACHABLE(); } }
The branches of the switch after a call to 'do_unexpected_trap' cannot return, but there is one path that may return, hence only some clauses are marked with ASSERT_UNREACHABLE(). Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/arm/traps.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)