Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2024 16:08, Federico Serafini wrote:
> On 07/02/24 15:16, Jan Beulich wrote:
>> On 07.02.2024 14:51, Federico Serafini wrote:
>>> On 07/02/24 08:38, Jan Beulich wrote:
>>>> On 07.02.2024 02:08, Stefano Stabellini wrote:
>>>>> On Tue, 6 Feb 2024, Jan Beulich wrote:
>>>>>> On 26.01.2024 11:05, Federico Serafini wrote:
>>>>>>> @@ -208,7 +205,7 @@ do {                                                
>>>>>>>                        \
>>>>>>>        case 8:                                                          
>>>>>>>       \
>>>>>>>            put_unsafe_asm(x, ptr, grd, retval, "q",  "", "ir", errret); 
>>>>>>>       \
>>>>>>>            break;                                                       
>>>>>>>       \
>>>>>>> -    default: __put_user_bad();                                         
>>>>>>>     \
>>>>>>> +    default: STATIC_ASSERT_UNREACHABLE();                              
>>>>>>>     \
>>>>>>>        }                                                                
>>>>>>>       \
>>>>>>>        clac();                                                          
>>>>>>>       \
>>>>>>>    } while ( false )
>>>>>>> @@ -227,7 +224,7 @@ do {                                                
>>>>>>>                        \
>>>>>>>        case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", errret); 
>>>>>>> break; \
>>>>>>>        case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", errret); 
>>>>>>> break; \
>>>>>>>        case 8: get_unsafe_asm(x, ptr, grd, retval,  "", "=r", errret); 
>>>>>>> break; \
>>>>>>> -    default: __get_user_bad();                                         
>>>>>>>     \
>>>>>>> +    default: STATIC_ASSERT_UNREACHABLE();                              
>>>>>>>     \
>>>>>>>        }                                                                
>>>>>>>       \
>>>>>>>        clac();                                                          
>>>>>>>       \
>>>>>>>    } while ( false )
>>>>>>
>>>>>> Related to my remark on patch 1 - how is one to know the macro this was
>>>>>> invoked from, when seeing the resulting diagnostic?
>>>>>
>>>>> I am not sure what do you mean here... we do get an error like the
>>>>> following (I added a STATIC_ASSERT_UNREACHABLE for case 4):
>>>>>
>>>>> ./arch/x86/include/asm/uaccess.h:262: Error: static assertion failed: 
>>>>> unreachable
>>>>
>>>> Right - and how do I know what _user_ of the macro actually triggered
>>>> it? ISTR suggesting to use one or more of __FILE__ / __LINE__ /
>>>> __FUNCTION__ here, for that specific purpose ...
>>>
>>> To test the macro and its diagnostics,
>>> I modified the first "git grep" occurrence of ASSERT_UNREACHABLE()
>>> on the x86 code with STATIC_ASSERT_UNREACHABLE(),
>>> that is in file arch/x86/alternative.c, line 312,
>>> function _apply_alternatives().
>>>
>>> What I got is the following build error:
>>>
>>> ...
>>> arch/x86/alternative.c: Assembler messages:
>>> arch/x86/alternative.c:312: Error: static assertion failed: unreachable
>>>     CC      arch/x86/copy_page.o
>>> make[2]: *** [Rules.mk:247: arch/x86/alternative.o] Error 1
>>
>> But that's not what my request was about. Here sufficient context is
>> given, even if it would be nice if the function was also visible right
>> away. But that's not the same as the case above, where the new macro
>> is used inside another macro.
> 
> An example of that is the get_unsafe_size() macro,
> whose body uses STATIC_ASSERT_UNREACHABLE().
> A wrong use of get_unsafe_size() at line n
> leads to a build error pointing to the line n,
> isn't this the desired behavior?

Aiui this would point to the line in the header file, when what you need
to spot the bad use of the macro is the line in the source file actually
using the macro. Quoting from an earlier mail of yours:

./arch/x86/include/asm/uaccess.h:262: Error: static assertion failed: 
unreachable

Compare this with what we have today, where the linker will point out
the function it found the bad use in. Of course this could also be
solved by better assembler diagnostics, but I'm not sure compiler output
would actually lend itself to that. Specifically we'd then rely on the
.type directive always preceding the actual function. Plus while it may
be reasonably possible to change gas, I'm not as sure about Clang's
integrated assembler. Plus changing gas and then getting it into use by
people will take quite a bit of time.

Jan