On 05.01.2024 17:19, Federico Serafini wrote:
> Hello everyone,
>
> On 21/12/23 13:41, Jan Beulich wrote:
>> On 21.12.2023 13:01, Nicola Vetrini wrote:
>>> Hi Andrew,
>>>
>>> On 2023-12-21 12:03, Andrew Cooper wrote:
>>>> On 21/12/2023 10:58 am, Jan Beulich wrote:
>>>>> On 21.12.2023 11:53, Federico Serafini wrote:
>>>>>> Remove declarations of __put_user_bad() and __get_user_bad()
>>>>>> since they have no definition.
>>>>>> Replace their uses with a break statement to address violations of
>>>>>> MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
>>>>>> terminate every switch-clause").
>>>>>> No functional change.
>>>>>>
>>>>>> Signed-off-by: Federico Serafini <federico.seraf...@bugseng.com>
>>>>>> ---
>>>>>> Several violations of Rule 16.3 come from uses of macros
>>>>>> get_unsafe_size() and put_unsafe_size().
>>>>>> Looking at the macro definitions I found __get_user_bad() and
>>>>>> __put_user_bad().
>>>>>> I was wondering if instead of just adding the break statement I can
>>>>>> also remove
>>>>>> such functions which seem to not have a definition.
>>>>> No, you can't. Try introducing a caller which "accidentally" uses the
>>>>> wrong size. Without your change you'll observe the build failing (in
>>>>> a somewhat obscure way, but still), while with your change bad code
>>>>> will silently be generated.
>>>>
>>>> The construct here is deliberate. It's a build time assertion that bad
>>>> sizes aren't used.
>>>>
>>>> __bitop_bad_size() and __xsm_action_mismatch_detected() are the same
>>>> pattern in other areas of code too, with the latter being more explicit
>>>> because of how it's wrapped by LINKER_BUG_ON().
>>>>
>>>>
>>>> It is slightly horrible, and not the most obvious construct for
>>>> newcomers. If there's an alternative way to get a build assertion, we
>>>> could consider switching to a new pattern.
>>>
>>> would you be in favour of a solution with a BUILD_BUG_ON in the default
>>> branch followed by a break?
>>>
>>> default:
>>> BUILD_BUG_ON(!size || size >=8 || (size & (size - 1)));
>>> break;
>>
>> I don't think this would compile - BUILD_BUG_ON() wants a compile-time
>> constant passed.
>
> What do you think about adding the following macro to compiler.h:
>
> #define static_assert_unreachable(identifier) \
> asm("unreachable " #identifier " reached")
>
> It expands to an invalid assembly instruction that will lead to a
> customizable error message generated by the assembler instead of the
> linker (anticipating the error detection).
>
> The use of this macro will indicate a program point considered
> unreachable (and as such removed) by the static analysis performed by
> the compiler, even at an optimization level -O0.
>
> An example of use is in the default case of put_unsafe_size():
>
> default: static_assert_unreachable(default);
>
> In case a wrong size will be used, the following message will be
> generated:
>
> ./arch/x86/include/asm/uaccess.h: Assembler messages:
> ./arch/x86/include/asm/uaccess.h:257: Error: no such instruction:
> `unreachable default reached'
Nice idea. To take it one step further, why not simply use the .error
assembler directive then?
> Note that adopting the macro and discussing its definition are two
> separate things:
> I think we can all agree on the fact that the use of such macro improves
> readability, so I would suggest its adoption.
> Whereas for its definition, if you don't like the invalid asm
> instruction, we could discuss for a different solution, for example,
> the following is something similar to what you are doing now:
>
> #define static_assert_unreachable(identifier) \
> extern void identifier(void); \
> identifier()
>
>
> Note also that the problem of the missing break statement (that violates
> Rule 16.3) is still present, it could be addressed by adding the break
> or deviating for such special cases, do you have any preferences?
Amend the new macro's expansion by unreachable()?
Jan
