| Message ID | d06a312944bee7457fa2ac75e0cfef20f0ec430f.1689329728.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix some issues related to MISRA C:2012 Rule 9.1 | expand |
Hi, On 14/07/2023 12:49, Nicola Vetrini wrote: > The macro 'testop' expands to a function that declares the local > variable 'oldbit', which is written before being set, but is such a > way that is not amenable to automatic checking. The code is pretty straightforward. So I am not entirely sure why Eclair is not happy. Is it because the value is set by assembly code? Cheers,
> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: > > The macro 'testop' expands to a function that declares the local > variable 'oldbit', which is written before being set, but is such a > way that is not amenable to automatic checking. > > Therefore, a deviation comment, is introduced to document this situation. > > A similar reasoning applies to macro 'guest_testop'. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > docs/misra/safe.json | 16 ++++++++++++++++ > xen/arch/arm/arm64/lib/bitops.c | 3 +++ > xen/arch/arm/include/asm/guest_atomics.h | 3 +++ > 3 files changed, 22 insertions(+) > > diff --git a/docs/misra/safe.json b/docs/misra/safe.json > index 244001f5be..4cf7cbf57b 100644 > --- a/docs/misra/safe.json > +++ b/docs/misra/safe.json > @@ -20,6 +20,22 @@ > }, > { > "id": "SAF-2-safe", > + "analyser": { > + "eclair": "MC3R1.R9.1" > + }, > + "name": "Rule 9.1: initializer not needed", > + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." > + }, > + { > + "id": "SAF-3-safe", > + "analyser": { > + "eclair": "MC3R1.R9.1" > + }, > + "name": "Rule 9.1: initializer not needed", > + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." > + }, Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who > + { > + "id": "SAF-4-safe", > "analyser": {}, > "name": "Sentinel", > "text": "Next ID to be used" > diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c > index 20e3f3d6ce..e0728bb29d 100644 > --- a/xen/arch/arm/arm64/lib/bitops.c > +++ b/xen/arch/arm/arm64/lib/bitops.c > @@ -114,8 +114,11 @@ bitop(change_bit, eor) > bitop(clear_bit, bic) > bitop(set_bit, orr) > > +/* SAF-2-safe MC3R1.R9.1 */ > testop(test_and_change_bit, eor) > +/* SAF-2-safe MC3R1.R9.1 */ > testop(test_and_clear_bit, bic) > +/* SAF-2-safe MC3R1.R9.1 */ > testop(test_and_set_bit, orr) > > static always_inline bool int_clear_mask16(uint16_t mask, volatile uint16_t *p, > diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h > index a1745f8613..9d8f8ec3a3 100644 > --- a/xen/arch/arm/include/asm/guest_atomics.h > +++ b/xen/arch/arm/include/asm/guest_atomics.h > @@ -67,8 +67,11 @@ guest_bitop(change_bit) > /* test_bit does not use load-store atomic operations */ > #define guest_test_bit(d, nr, p) ((void)(d), test_bit(nr, p)) > > +/* SAF-3-safe MC3R1.R9.1 */ > guest_testop(test_and_set_bit) > +/* SAF-3-safe MC3R1.R9.1 */ > guest_testop(test_and_clear_bit) > +/* SAF-3-safe MC3R1.R9.1 */ > guest_testop(test_and_change_bit) > > #undef guest_testop > -- > 2.34.1 > >
> On 14 Jul 2023, at 15:20, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > > >> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >> >> The macro 'testop' expands to a function that declares the local >> variable 'oldbit', which is written before being set, but is such a >> way that is not amenable to automatic checking. >> >> Therefore, a deviation comment, is introduced to document this situation. >> >> A similar reasoning applies to macro 'guest_testop'. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> docs/misra/safe.json | 16 ++++++++++++++++ >> xen/arch/arm/arm64/lib/bitops.c | 3 +++ >> xen/arch/arm/include/asm/guest_atomics.h | 3 +++ >> 3 files changed, 22 insertions(+) >> >> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >> index 244001f5be..4cf7cbf57b 100644 >> --- a/docs/misra/safe.json >> +++ b/docs/misra/safe.json >> @@ -20,6 +20,22 @@ >> }, >> { >> "id": "SAF-2-safe", >> + "analyser": { >> + "eclair": "MC3R1.R9.1" >> + }, >> + "name": "Rule 9.1: initializer not needed", >> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >> + }, >> + { >> + "id": "SAF-3-safe", >> + "analyser": { >> + "eclair": "MC3R1.R9.1" >> + }, >> + "name": "Rule 9.1: initializer not needed", >> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >> + }, > > Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, > also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who Sorry, I see there was in a patch before a SAF-1-safe with the same justification, so I suggest you use SAF-3-safe as tag and drop the new justifications introduced here > >> + { >> + "id": "SAF-4-safe", >> "analyser": {}, >> "name": "Sentinel", >> "text": "Next ID to be used" >> diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c >> index 20e3f3d6ce..e0728bb29d 100644 >> --- a/xen/arch/arm/arm64/lib/bitops.c >> +++ b/xen/arch/arm/arm64/lib/bitops.c >> @@ -114,8 +114,11 @@ bitop(change_bit, eor) >> bitop(clear_bit, bic) >> bitop(set_bit, orr) >> >> +/* SAF-2-safe MC3R1.R9.1 */ >> testop(test_and_change_bit, eor) >> +/* SAF-2-safe MC3R1.R9.1 */ >> testop(test_and_clear_bit, bic) >> +/* SAF-2-safe MC3R1.R9.1 */ >> testop(test_and_set_bit, orr) >> >> static always_inline bool int_clear_mask16(uint16_t mask, volatile uint16_t *p, >> diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h >> index a1745f8613..9d8f8ec3a3 100644 >> --- a/xen/arch/arm/include/asm/guest_atomics.h >> +++ b/xen/arch/arm/include/asm/guest_atomics.h >> @@ -67,8 +67,11 @@ guest_bitop(change_bit) >> /* test_bit does not use load-store atomic operations */ >> #define guest_test_bit(d, nr, p) ((void)(d), test_bit(nr, p)) >> >> +/* SAF-3-safe MC3R1.R9.1 */ >> guest_testop(test_and_set_bit) >> +/* SAF-3-safe MC3R1.R9.1 */ >> guest_testop(test_and_clear_bit) >> +/* SAF-3-safe MC3R1.R9.1 */ >> guest_testop(test_and_change_bit) >> >> #undef guest_testop >> -- >> 2.34.1 >> >> >
>>> "id": "SAF-2-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>> + }, >>> + { >>> + "id": "SAF-3-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>> + }, >> >> Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, >> also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who > > Sorry, I see there was in a patch before a SAF-1-safe with the same justification, so I suggest you use SAF-3-safe as tag and drop the new justifications introduced here I meant “use SAF-1-safe”, sorry, I should do less multitasking
On 14/07/23 16:32, Luca Fancellu wrote: > > >> On 14 Jul 2023, at 15:20, Luca Fancellu <Luca.Fancellu@arm.com> wrote: >> >> >> >>> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>> >>> The macro 'testop' expands to a function that declares the local >>> variable 'oldbit', which is written before being set, but is such a >>> way that is not amenable to automatic checking. >>> >>> Therefore, a deviation comment, is introduced to document this situation. >>> >>> A similar reasoning applies to macro 'guest_testop'. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> docs/misra/safe.json | 16 ++++++++++++++++ >>> xen/arch/arm/arm64/lib/bitops.c | 3 +++ >>> xen/arch/arm/include/asm/guest_atomics.h | 3 +++ >>> 3 files changed, 22 insertions(+) >>> >>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>> index 244001f5be..4cf7cbf57b 100644 >>> --- a/docs/misra/safe.json >>> +++ b/docs/misra/safe.json >>> @@ -20,6 +20,22 @@ >>> }, >>> { >>> "id": "SAF-2-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>> + }, >>> + { >>> + "id": "SAF-3-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>> + }, >> >> Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, >> also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who > > Sorry, I see there was in a patch before a SAF-1-safe with the same justification, so I suggest you use SAF-3-safe as tag and drop the new justifications introduced here I was a bit uncertain in choosing whether to use different IDs for bitops.c and guest_atomics.h, as they are very similar. Maybe a catch-all justification is too broad, but I have no strong opinion here. As for the rule identifier after the tag, that's not a big problem (I added it mostly to emphasize why the deviation is there at a glance). > >> >>> + { >>> + "id": "SAF-4-safe", >>> "analyser": {}, >>> "name": "Sentinel", >>> "text": "Next ID to be used" >>> diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c >>> index 20e3f3d6ce..e0728bb29d 100644 >>> --- a/xen/arch/arm/arm64/lib/bitops.c >>> +++ b/xen/arch/arm/arm64/lib/bitops.c >>> @@ -114,8 +114,11 @@ bitop(change_bit, eor) >>> bitop(clear_bit, bic) >>> bitop(set_bit, orr) >>> >>> +/* SAF-2-safe MC3R1.R9.1 */ >>> testop(test_and_change_bit, eor) >>> +/* SAF-2-safe MC3R1.R9.1 */ >>> testop(test_and_clear_bit, bic) >>> +/* SAF-2-safe MC3R1.R9.1 */ >>> testop(test_and_set_bit, orr) >>> >>> static always_inline bool int_clear_mask16(uint16_t mask, volatile uint16_t *p, >>> diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h >>> index a1745f8613..9d8f8ec3a3 100644 >>> --- a/xen/arch/arm/include/asm/guest_atomics.h >>> +++ b/xen/arch/arm/include/asm/guest_atomics.h >>> @@ -67,8 +67,11 @@ guest_bitop(change_bit) >>> /* test_bit does not use load-store atomic operations */ >>> #define guest_test_bit(d, nr, p) ((void)(d), test_bit(nr, p)) >>> >>> +/* SAF-3-safe MC3R1.R9.1 */ >>> guest_testop(test_and_set_bit) >>> +/* SAF-3-safe MC3R1.R9.1 */ >>> guest_testop(test_and_clear_bit) >>> +/* SAF-3-safe MC3R1.R9.1 */ >>> guest_testop(test_and_change_bit) >>> >>> #undef guest_testop >>> -- >>> 2.34.1 >>> >>> >> >
> On 14 Jul 2023, at 14:05, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 14/07/2023 12:49, Nicola Vetrini wrote: >> The macro 'testop' expands to a function that declares the local >> variable 'oldbit', which is written before being set, but is such a >> way that is not amenable to automatic checking. > > The code is pretty straightforward. So I am not entirely sure why Eclair is not happy. Is it because the value is set by assembly code? Hi Julien, Yes I agree that it’s strange, do you think that that if at line 97 we initialize oldbit to zero, it would solve the issue? @Nicola Cheers, Luca > > Cheers, > > -- > Julien Grall >
Hi, On 16/07/2023 10:20, Luca Fancellu wrote: >> On 14 Jul 2023, at 14:05, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 14/07/2023 12:49, Nicola Vetrini wrote: >>> The macro 'testop' expands to a function that declares the local >>> variable 'oldbit', which is written before being set, but is such a >>> way that is not amenable to automatic checking. >> >> The code is pretty straightforward. So I am not entirely sure why Eclair is not happy. Is it because the value is set by assembly code? > > Hi Julien, > > Yes I agree that it’s strange, do you think that that if at line 97 we initialize oldbit to zero, it would solve the issue? At the moment, I am not entirely happy to zero oldbit because we have other places in Xen where we need assembly instructions to read a value (e.g. READ_SYSREG()). So I would first like to understand why Eclair can't cope with it. Cheers,
On 16/07/23 18:50, Julien Grall wrote: > Hi, > > On 16/07/2023 10:20, Luca Fancellu wrote: >>> On 14 Jul 2023, at 14:05, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 14/07/2023 12:49, Nicola Vetrini wrote: >>>> The macro 'testop' expands to a function that declares the local >>>> variable 'oldbit', which is written before being set, but is such a >>>> way that is not amenable to automatic checking. >>> >>> The code is pretty straightforward. So I am not entirely sure why >>> Eclair is not happy. Is it because the value is set by assembly code? Exactly. The reason why I didn't just state that oldbit is always written or never read before being written in that function is that I was unsure about the meaning of the assembly. >> >> Hi Julien, >> >> Yes I agree that it’s strange, do you think that that if at line 97 we >> initialize oldbit to zero, it would solve the issue? > > At the moment, I am not entirely happy to zero oldbit because we have > other places in Xen where we need assembly instructions to read a value > (e.g. READ_SYSREG()). > > So I would first like to understand why Eclair can't cope with it. > > Cheers, >
Hi, On 17/07/2023 13:16, Nicola Vetrini wrote: > > > On 16/07/23 18:50, Julien Grall wrote: >> Hi, >> >> On 16/07/2023 10:20, Luca Fancellu wrote: >>>> On 14 Jul 2023, at 14:05, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi, >>>> >>>> On 14/07/2023 12:49, Nicola Vetrini wrote: >>>>> The macro 'testop' expands to a function that declares the local >>>>> variable 'oldbit', which is written before being set, but is such a >>>>> way that is not amenable to automatic checking. >>>> >>>> The code is pretty straightforward. So I am not entirely sure why >>>> Eclair is not happy. Is it because the value is set by assembly code? > > Exactly. I am a bit puzzled. There are other values read from assembly (e.g. READ_SYSREG(), read_atomic()) within Xen. This is including 'res' in the same function. So can you elaborate, why 'oldbit' is a problem but not the others? Cheers,
On 14.07.2023 16:20, Luca Fancellu wrote: > > >> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >> >> The macro 'testop' expands to a function that declares the local >> variable 'oldbit', which is written before being set, but is such a >> way that is not amenable to automatic checking. >> >> Therefore, a deviation comment, is introduced to document this situation. >> >> A similar reasoning applies to macro 'guest_testop'. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> docs/misra/safe.json | 16 ++++++++++++++++ >> xen/arch/arm/arm64/lib/bitops.c | 3 +++ >> xen/arch/arm/include/asm/guest_atomics.h | 3 +++ >> 3 files changed, 22 insertions(+) >> >> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >> index 244001f5be..4cf7cbf57b 100644 >> --- a/docs/misra/safe.json >> +++ b/docs/misra/safe.json >> @@ -20,6 +20,22 @@ >> }, >> { >> "id": "SAF-2-safe", >> + "analyser": { >> + "eclair": "MC3R1.R9.1" >> + }, >> + "name": "Rule 9.1: initializer not needed", >> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >> + }, >> + { >> + "id": "SAF-3-safe", >> + "analyser": { >> + "eclair": "MC3R1.R9.1" >> + }, >> + "name": "Rule 9.1: initializer not needed", >> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >> + }, > > Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, +1 I'm puzzled by the wording vs comment placement though: The comments are inserted ahead of the macro invocations, so there are no "following local variables". Plus does this imply the comment would suppress the checking on _all_ of them, rather than just the one that was confirmed to be safe? What if another new one was added, that actually introduces a problem? > also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who Me, at least. The annotations should be tool-agnostic imo, or else the more tools we use, the longer these comments might get. Jan
On 17.07.2023 14:16, Nicola Vetrini wrote: > On 16/07/23 18:50, Julien Grall wrote: >> On 16/07/2023 10:20, Luca Fancellu wrote: >>>> On 14 Jul 2023, at 14:05, Julien Grall <julien@xen.org> wrote: >>>> On 14/07/2023 12:49, Nicola Vetrini wrote: >>>>> The macro 'testop' expands to a function that declares the local >>>>> variable 'oldbit', which is written before being set, but is such a >>>>> way that is not amenable to automatic checking. >>>> >>>> The code is pretty straightforward. So I am not entirely sure why >>>> Eclair is not happy. Is it because the value is set by assembly code? > > Exactly. The reason why I didn't just state that oldbit is always > written or never read before being written in that function is that I > was unsure about the meaning of the assembly. So I'm pretty sure the tool wouldn't take apart the string literal passed first to the asm(). Instead I expect it goes from the operands specified, which for oldbit is "=&r". There's nothing conditional here, so if the tool didn't trust that outputs are written, it would need to flag such an issue on about any asm() having outputs. I hope the issue isn't that the tool doesn't properly deal with the do/while. In any event: I may misremember earlier discussions, but isn't this a pretty obvious false positive? In which case didn't we mean to flag those as such (because really an improved checking tool could avoid them)? Jan
On 17/07/23 16:06, Jan Beulich wrote: > On 17.07.2023 14:16, Nicola Vetrini wrote: >> On 16/07/23 18:50, Julien Grall wrote: >>> On 16/07/2023 10:20, Luca Fancellu wrote: >>>>> On 14 Jul 2023, at 14:05, Julien Grall <julien@xen.org> wrote: >>>>> On 14/07/2023 12:49, Nicola Vetrini wrote: >>>>>> The macro 'testop' expands to a function that declares the local >>>>>> variable 'oldbit', which is written before being set, but is such a >>>>>> way that is not amenable to automatic checking. >>>>> >>>>> The code is pretty straightforward. So I am not entirely sure why >>>>> Eclair is not happy. Is it because the value is set by assembly code? >> >> Exactly. The reason why I didn't just state that oldbit is always >> written or never read before being written in that function is that I >> was unsure about the meaning of the assembly. > > So I'm pretty sure the tool wouldn't take apart the string literal passed > first to the asm(). Instead I expect it goes from the operands specified, > which for oldbit is "=&r". There's nothing conditional here, so if the > tool didn't trust that outputs are written, it would need to flag such an > issue on about any asm() having outputs. > > I hope the issue isn't that the tool doesn't properly deal with the > do/while. > > In any event: I may misremember earlier discussions, but isn't this a > pretty obvious false positive? In which case didn't we mean to flag > those as such (because really an improved checking tool could avoid them)? > > Jan > Actually, given that the function indeed always writes oldbit in the asm instruction, I can state that this post-condition holds for the function.
On 17/07/23 15:59, Jan Beulich wrote: > On 14.07.2023 16:20, Luca Fancellu wrote: >> >> >>> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>> >>> The macro 'testop' expands to a function that declares the local >>> variable 'oldbit', which is written before being set, but is such a >>> way that is not amenable to automatic checking. >>> >>> Therefore, a deviation comment, is introduced to document this situation. >>> >>> A similar reasoning applies to macro 'guest_testop'. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> docs/misra/safe.json | 16 ++++++++++++++++ >>> xen/arch/arm/arm64/lib/bitops.c | 3 +++ >>> xen/arch/arm/include/asm/guest_atomics.h | 3 +++ >>> 3 files changed, 22 insertions(+) >>> >>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>> index 244001f5be..4cf7cbf57b 100644 >>> --- a/docs/misra/safe.json >>> +++ b/docs/misra/safe.json >>> @@ -20,6 +20,22 @@ >>> }, >>> { >>> "id": "SAF-2-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>> + }, >>> + { >>> + "id": "SAF-3-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>> + }, >> >> Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, > > +1 > > I'm puzzled by the wording vs comment placement though: The comments > are inserted ahead of the macro invocations, so there are no "following > local variables". Plus does this imply the comment would suppress the > checking on _all_ of them, rather than just the one that was confirmed > to be safe? What if another new one was added, that actually introduces > a problem? > >> also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who > > Me, at least. The annotations should be tool-agnostic imo, or else the > more tools we use, the longer these comments might get. > > Jan No problem for both: Given the earlier remarks by Julien on patch 1/4, I think we can devise something along the lines of /* SAF-x-safe MISRA:C 2012 Rule 9.1 <Justification> */ int var; and then write a generic justification in docs/misra/safe.json, while <Justification> contains a specific one (e.g., this loop does so and so to ensure that no access to unset variables happens). Thus any modification to that variable would give who submits the patch and the reviewer a chance to spot any inconsistency when the code is modified. However, given my other response to this same patch, this does not apply here specifically.
On 17.07.2023 17:28, Nicola Vetrini wrote: > > > On 17/07/23 15:59, Jan Beulich wrote: >> On 14.07.2023 16:20, Luca Fancellu wrote: >>> >>> >>>> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>>> >>>> The macro 'testop' expands to a function that declares the local >>>> variable 'oldbit', which is written before being set, but is such a >>>> way that is not amenable to automatic checking. >>>> >>>> Therefore, a deviation comment, is introduced to document this situation. >>>> >>>> A similar reasoning applies to macro 'guest_testop'. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> docs/misra/safe.json | 16 ++++++++++++++++ >>>> xen/arch/arm/arm64/lib/bitops.c | 3 +++ >>>> xen/arch/arm/include/asm/guest_atomics.h | 3 +++ >>>> 3 files changed, 22 insertions(+) >>>> >>>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>>> index 244001f5be..4cf7cbf57b 100644 >>>> --- a/docs/misra/safe.json >>>> +++ b/docs/misra/safe.json >>>> @@ -20,6 +20,22 @@ >>>> }, >>>> { >>>> "id": "SAF-2-safe", >>>> + "analyser": { >>>> + "eclair": "MC3R1.R9.1" >>>> + }, >>>> + "name": "Rule 9.1: initializer not needed", >>>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>>> + }, >>>> + { >>>> + "id": "SAF-3-safe", >>>> + "analyser": { >>>> + "eclair": "MC3R1.R9.1" >>>> + }, >>>> + "name": "Rule 9.1: initializer not needed", >>>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>>> + }, >>> >>> Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, >> >> +1 >> >> I'm puzzled by the wording vs comment placement though: The comments >> are inserted ahead of the macro invocations, so there are no "following >> local variables". Plus does this imply the comment would suppress the >> checking on _all_ of them, rather than just the one that was confirmed >> to be safe? What if another new one was added, that actually introduces >> a problem? >> >>> also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who >> >> Me, at least. The annotations should be tool-agnostic imo, or else the >> more tools we use, the longer these comments might get. > > No problem for both: Given the earlier remarks by Julien on patch 1/4, I > think we can devise something along the lines of > > /* SAF-x-safe MISRA:C 2012 Rule 9.1 <Justification> */ > int var; > > and then write a generic justification in docs/misra/safe.json, while > <Justification> contains a specific one (e.g., this loop does so and so > to ensure that no access to unset variables happens). But that still mentions the rule. What if a new MISRA:C 2025 appears, with different rule numbers? I don't mind the comment mentioning, in a sufficiently terse way, what problem is circumvented. But I don't think tools or specifications should be referenced here. That's the purpose of the referenced "database" entry. Jan
Hi, On 14/07/2023 12:49, Nicola Vetrini wrote: > The macro 'testop' expands to a function that declares the local > variable 'oldbit', which is written before being set, but is such a > way that is not amenable to automatic checking. > > Therefore, a deviation comment, is introduced to document this situation. > > A similar reasoning applies to macro 'guest_testop'. Would you be able to check if the code below (only compile tested so far) would silence Eclair? diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c index 20e3f3d6ceaf..f7a10b790f2a 100644 --- a/xen/arch/arm/arm64/lib/bitops.c +++ b/xen/arch/arm/arm64/lib/bitops.c @@ -64,13 +64,15 @@ bool name##_timeout(int nr, volatile void *p, unsigned int max_try) \ } #define testop(name, instr) \ -static always_inline bool int_##name(int nr, volatile void *p, int *oldbit, \ - bool timeout, unsigned int max_try) \ +static always_inline int int_##name(int nr, volatile void *p, \ + bool allow_timeout, bool *timeout, \ + unsigned int max_try) \ { \ volatile uint32_t *ptr = (uint32_t *)p + BITOP_WORD((unsigned int)nr); \ unsigned int bit = (unsigned int)nr % BITOP_BITS_PER_WORD; \ const uint32_t mask = BITOP_MASK(bit); \ unsigned long res, tmp; \ + int oldbit; \ \ do \ { \ @@ -79,27 +81,30 @@ static always_inline bool int_##name(int nr, volatile void *p, int *oldbit, \ " lsr %w1, %w3, %w5 // Save old value of bit\n" \ " " __stringify(instr) " %w3, %w3, %w4 // Toggle bit\n" \ " stlxr %w0, %w3, %2\n" \ - : "=&r" (res), "=&r" (*oldbit), "+Q" (*ptr), "=&r" (tmp) \ + : "=&r" (res), "=&r" (oldbit), "+Q" (*ptr), "=&r" (tmp) \ : "r" (mask), "r" (bit) \ : "memory"); \ \ if ( !res ) \ break; \ - } while ( !timeout || ((--max_try) > 0) ); \ + } while ( !allow_timeout || ((--max_try) > 0) ); \ \ dmb(ish); \ \ - *oldbit &= 1; \ + ASSERT(!allow_timeout || timeout); \ + if ( timeout ) \ + *timeout = !res; \ + else if ( !res ) \ + ASSERT_UNREACHABLE(); \ \ - return !res; \ + return (oldbit & 1); \ } \ \ int name(int nr, volatile void *p) \ { \ int oldbit; \ \ - if ( !int_##name(nr, p, &oldbit, false, 0) ) \ - ASSERT_UNREACHABLE(); \ + oldbit = int_##name(nr, p, false, NULL, 0); \ \ Cheers,
On 17/07/2023 21:40, Julien Grall wrote: > On 14/07/2023 12:49, Nicola Vetrini wrote: >> The macro 'testop' expands to a function that declares the local >> variable 'oldbit', which is written before being set, but is such a >> way that is not amenable to automatic checking. >> >> Therefore, a deviation comment, is introduced to document this situation. >> >> A similar reasoning applies to macro 'guest_testop'. > > Would you be able to check if the code below (only compile tested so > far) would silence Eclair? Hmmm.. I think my e-mail client mangled the diff. Here an unmangled version if needed: http://paste.debian.net/1286154/ Cheers,
On 17/07/23 17:46, Jan Beulich wrote: > On 17.07.2023 17:28, Nicola Vetrini wrote: >> >> >> On 17/07/23 15:59, Jan Beulich wrote: >>> On 14.07.2023 16:20, Luca Fancellu wrote: >>>> >>>> >>>>> On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>>>> >>>>> The macro 'testop' expands to a function that declares the local >>>>> variable 'oldbit', which is written before being set, but is such a >>>>> way that is not amenable to automatic checking. >>>>> >>>>> Therefore, a deviation comment, is introduced to document this situation. >>>>> >>>>> A similar reasoning applies to macro 'guest_testop'. >>>>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> --- >>>>> docs/misra/safe.json | 16 ++++++++++++++++ >>>>> xen/arch/arm/arm64/lib/bitops.c | 3 +++ >>>>> xen/arch/arm/include/asm/guest_atomics.h | 3 +++ >>>>> 3 files changed, 22 insertions(+) >>>>> >>>>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>>>> index 244001f5be..4cf7cbf57b 100644 >>>>> --- a/docs/misra/safe.json >>>>> +++ b/docs/misra/safe.json >>>>> @@ -20,6 +20,22 @@ >>>>> }, >>>>> { >>>>> "id": "SAF-2-safe", >>>>> + "analyser": { >>>>> + "eclair": "MC3R1.R9.1" >>>>> + }, >>>>> + "name": "Rule 9.1: initializer not needed", >>>>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>>>> + }, >>>>> + { >>>>> + "id": "SAF-3-safe", >>>>> + "analyser": { >>>>> + "eclair": "MC3R1.R9.1" >>>>> + }, >>>>> + "name": "Rule 9.1: initializer not needed", >>>>> + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." >>>>> + }, >>>> >>>> Since the rule and the justification are the same, you can declare only once and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 */, >>> >>> +1 >>> >>> I'm puzzled by the wording vs comment placement though: The comments >>> are inserted ahead of the macro invocations, so there are no "following >>> local variables". Plus does this imply the comment would suppress the >>> checking on _all_ of them, rather than just the one that was confirmed >>> to be safe? What if another new one was added, that actually introduces >>> a problem? >>> >>>> also, I remember some maintainers not happy about the misra rule being put after the tag, now I don’t recall who >>> >>> Me, at least. The annotations should be tool-agnostic imo, or else the >>> more tools we use, the longer these comments might get. >> >> No problem for both: Given the earlier remarks by Julien on patch 1/4, I >> think we can devise something along the lines of >> >> /* SAF-x-safe MISRA:C 2012 Rule 9.1 <Justification> */ >> int var; >> >> and then write a generic justification in docs/misra/safe.json, while >> <Justification> contains a specific one (e.g., this loop does so and so >> to ensure that no access to unset variables happens). > > But that still mentions the rule. What if a new MISRA:C 2025 appears, > with different rule numbers? I don't mind the comment mentioning, in > a sufficiently terse way, what problem is circumvented. But I don't > think tools or specifications should be referenced here. That's the > purpose of the referenced "database" entry. > > Jan Ok
On 17/07/23 22:45, Julien Grall wrote: > > > On 17/07/2023 21:40, Julien Grall wrote: >> On 14/07/2023 12:49, Nicola Vetrini wrote: >>> The macro 'testop' expands to a function that declares the local >>> variable 'oldbit', which is written before being set, but is such a >>> way that is not amenable to automatic checking. >>> >>> Therefore, a deviation comment, is introduced to document this >>> situation. >>> >>> A similar reasoning applies to macro 'guest_testop'. >> >> Would you be able to check if the code below (only compile tested so >> far) would silence Eclair? > > Hmmm.. I think my e-mail client mangled the diff. Here an unmangled > version if needed: > > http://paste.debian.net/1286154/ > > Cheers, > I have a question: wouldn't this patch also imply an update of xen/arch/arm/include/asm/guest_atomics.h ? This in particular is the bit of code that needs to be reworked to use the newer *_timeout definition. bool succeed; succeed = name##_timeout(nr, p, &oldbit, this_cpu(guest_safe_atomic_max)); if ( succeed ) return oldbit; Probably something similar to int succeed; succeed = name##_timeout(nr, p, false, NULL, [...]); if ( succeed != 0 ) return succeed; I'm not really sure whether the third param should be true or false, but I assumed false because it's similar to the use in bitops.c
Hi, On 19/07/2023 09:54, Nicola Vetrini wrote: > > > On 17/07/23 22:45, Julien Grall wrote: >> >> >> On 17/07/2023 21:40, Julien Grall wrote: >>> On 14/07/2023 12:49, Nicola Vetrini wrote: >>>> The macro 'testop' expands to a function that declares the local >>>> variable 'oldbit', which is written before being set, but is such a >>>> way that is not amenable to automatic checking. >>>> >>>> Therefore, a deviation comment, is introduced to document this >>>> situation. >>>> >>>> A similar reasoning applies to macro 'guest_testop'. >>> >>> Would you be able to check if the code below (only compile tested so >>> far) would silence Eclair? >> >> Hmmm.. I think my e-mail client mangled the diff. Here an unmangled >> version if needed: >> >> http://paste.debian.net/1286154/ >> >> Cheers, >> > > I have a question: wouldn't this patch also imply an update of > xen/arch/arm/include/asm/guest_atomics.h ? > > This in particular is the bit of code that needs to be reworked to use > the newer *_timeout definition. My patch is not changing the exported *_timeout. It is only changing the definition of int_##name which is not exported. But I have realized that one chunk were missing. Here the new version: https://paste.debian.net/1286311/ Cheers,
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index 244001f5be..4cf7cbf57b 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -20,6 +20,22 @@ }, { "id": "SAF-2-safe", + "analyser": { + "eclair": "MC3R1.R9.1" + }, + "name": "Rule 9.1: initializer not needed", + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." + }, + { + "id": "SAF-3-safe", + "analyser": { + "eclair": "MC3R1.R9.1" + }, + "name": "Rule 9.1: initializer not needed", + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." + }, + { + "id": "SAF-4-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c index 20e3f3d6ce..e0728bb29d 100644 --- a/xen/arch/arm/arm64/lib/bitops.c +++ b/xen/arch/arm/arm64/lib/bitops.c @@ -114,8 +114,11 @@ bitop(change_bit, eor) bitop(clear_bit, bic) bitop(set_bit, orr) +/* SAF-2-safe MC3R1.R9.1 */ testop(test_and_change_bit, eor) +/* SAF-2-safe MC3R1.R9.1 */ testop(test_and_clear_bit, bic) +/* SAF-2-safe MC3R1.R9.1 */ testop(test_and_set_bit, orr) static always_inline bool int_clear_mask16(uint16_t mask, volatile uint16_t *p, diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h index a1745f8613..9d8f8ec3a3 100644 --- a/xen/arch/arm/include/asm/guest_atomics.h +++ b/xen/arch/arm/include/asm/guest_atomics.h @@ -67,8 +67,11 @@ guest_bitop(change_bit) /* test_bit does not use load-store atomic operations */ #define guest_test_bit(d, nr, p) ((void)(d), test_bit(nr, p)) +/* SAF-3-safe MC3R1.R9.1 */ guest_testop(test_and_set_bit) +/* SAF-3-safe MC3R1.R9.1 */ guest_testop(test_and_clear_bit) +/* SAF-3-safe MC3R1.R9.1 */ guest_testop(test_and_change_bit) #undef guest_testop
The macro 'testop' expands to a function that declares the local variable 'oldbit', which is written before being set, but is such a way that is not amenable to automatic checking. Therefore, a deviation comment, is introduced to document this situation. A similar reasoning applies to macro 'guest_testop'. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- docs/misra/safe.json | 16 ++++++++++++++++ xen/arch/arm/arm64/lib/bitops.c | 3 +++ xen/arch/arm/include/asm/guest_atomics.h | 3 +++ 3 files changed, 22 insertions(+)