common/efi: deviate Rule 2.1 violation in read_file()

[PATCH] common/efi: deviate Rule 2.1 violation in read_file()

Posted by Dmytro Prokopchuk1 2 months, 1 week ago

MISRA C Rule 2.1 states: "A project shall not contain unreachable code."

The return statements in the 'read_file()' function is unreachable due
to function 'PrintErrMesg()' which is noreturn:
    PrintErrMesg(name, ret);
    /* not reached */
    return false;

This is deviated using a SAF-xx-safe comment (specified in the file
'docs/misra/safe.json').
No functional change.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Previous thread:
https://patchew.org/Xen/5944d87aae330246b7dab6eebd04d5d71a7d7e8f.1755608417.git.dmytro._5Fprokopchuk1@epam.com/

Test CI pipeline:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1994619131
---
 docs/misra/safe.json  | 8 ++++++++
 xen/common/efi/boot.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 3584cb90c6..2b3178de2d 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -124,6 +124,14 @@
         },
         {
             "id": "SAF-15-safe",
+            "analyser": {
+                "eclair": "MC3A2.R2.1"
+            },
+            "name": "Rule 2.1: unreachable code",
+            "text": "This is a deliberate use of unreachable code. The return statement is retained to improve code clarity and readability by explicitly specifying the intended behavior for a case if PrintErrMesg() was to return."
+        },
+        {
+            "id": "SAF-16-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 50ff1d1bd2..860c41c8e7 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -852,7 +852,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     PrintErr(L" failed for ");
     PrintErrMesg(name, ret);
 
-    /* not reached */
+    /* SAF-15-safe deliberately unreachable code */
     return false;
 }
 
-- 
2.43.0

Re: [PATCH] common/efi: deviate Rule 2.1 violation in read_file()

Posted by Jan Beulich 2 months, 1 week ago

On 20.08.2025 20:05, Dmytro Prokopchuk1 wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -852,7 +852,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      PrintErr(L" failed for ");
>      PrintErrMesg(name, ret);
>  
> -    /* not reached */
> +    /* SAF-15-safe deliberately unreachable code */
>      return false;
>  }

Much better (even if not tagged as v2). Yet then, did you consider
alternatives? For example, with PrintErrMesg() properly annotated "noreturn",
I'd kind of expect compilers to not object to the omission of the "return"
statement here. This would then let us get away without a new SAF comment.
While you explain in the SAF text why you retain the statement, I'm not
convinced of code clarity suffering if it was deleted, as long as a suitable
comment is still there. If PrintErrMesg() lost its "noreturn", surely
compilers would then diagnose the lack of "return".

Jan

Re: [PATCH] common/efi: deviate Rule 2.1 violation in read_file()

Posted by Dmytro Prokopchuk1 2 months, 1 week ago


On 8/21/25 13:33, Jan Beulich wrote:
> On 20.08.2025 20:05, Dmytro Prokopchuk1 wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -852,7 +852,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>       PrintErr(L" failed for ");
>>       PrintErrMesg(name, ret);
>>   
>> -    /* not reached */
>> +    /* SAF-15-safe deliberately unreachable code */
>>       return false;
>>   }
> 
> Much better (even if not tagged as v2). Yet then, did you consider
> alternatives? For example, with PrintErrMesg() properly annotated "noreturn",
> I'd kind of expect compilers to not object to the omission of the "return"
> statement here. This would then let us get away without a new SAF comment.
> While you explain in the SAF text why you retain the statement, I'm not
> convinced of code clarity suffering if it was deleted, as long as a suitable
> comment is still there. If PrintErrMesg() lost its "noreturn", surely
> compilers would then diagnose the lack of "return".
> 
> Jan

Sure, the next version will be v3.
Actually, the PrintErrMesg() already has property 'noreturn'.
And it really gives an alternative way: remove 'return false;' from the 
function read_file() (leaving comment there).

With that change Misra is "happy".

In case of removing 'noreturn' attribute from PrintErrMesg() function 
compiler will detect that:
arch/arm/efi/boot.c: In function ‘read_file’:
arch/arm/efi/boot.c:854:1: error: control reaches end of non-void 
function [-Werror=return-type]
  }
  ^

Is it OK to prepare such ^ patch?

Dmytro.

Re: [PATCH] common/efi: deviate Rule 2.1 violation in read_file()

Posted by Marek Marczykowski-Górecki 2 months, 1 week ago

On Thu, Aug 21, 2025 at 11:28:01AM +0000, Dmytro Prokopchuk1 wrote:
> 
> 
> On 8/21/25 13:33, Jan Beulich wrote:
> > On 20.08.2025 20:05, Dmytro Prokopchuk1 wrote:
> >> --- a/xen/common/efi/boot.c
> >> +++ b/xen/common/efi/boot.c
> >> @@ -852,7 +852,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> >>       PrintErr(L" failed for ");
> >>       PrintErrMesg(name, ret);
> >>   
> >> -    /* not reached */
> >> +    /* SAF-15-safe deliberately unreachable code */
> >>       return false;
> >>   }
> > 
> > Much better (even if not tagged as v2). Yet then, did you consider
> > alternatives? For example, with PrintErrMesg() properly annotated "noreturn",
> > I'd kind of expect compilers to not object to the omission of the "return"
> > statement here. This would then let us get away without a new SAF comment.
> > While you explain in the SAF text why you retain the statement, I'm not
> > convinced of code clarity suffering if it was deleted, as long as a suitable
> > comment is still there. If PrintErrMesg() lost its "noreturn", surely
> > compilers would then diagnose the lack of "return".
> > 
> > Jan
> 
> Sure, the next version will be v3.
> Actually, the PrintErrMesg() already has property 'noreturn'.
> And it really gives an alternative way: remove 'return false;' from the 
> function read_file() (leaving comment there).
> 
> With that change Misra is "happy".
> 
> In case of removing 'noreturn' attribute from PrintErrMesg() function 
> compiler will detect that:
> arch/arm/efi/boot.c: In function ‘read_file’:
> arch/arm/efi/boot.c:854:1: error: control reaches end of non-void 
> function [-Werror=return-type]
>   }
>   ^
> 
> Is it OK to prepare such ^ patch?

IMO sounds like the best solution for this issue.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab