[XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE()

[XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE()

[Federico Serafini]

Introduce macro STATIC_ASSERT_UNREACHABLE(),
use it to replace __{get,put}_user_bad(),
update ECLAIR configuration to allow the use of such macro at the end of
switch-caluses.

Federico Serafini (3):
  xen: introduce STATIC_ASSERT_UNREACHABLE()
  x86/uaccess: replace __{get,put}_user_bad() with
    STATIC_ASSERT_UNREACHABLE()
  automation/eclair: add deviation for MISRA C:2012 Rule 16.3

 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
 docs/misra/deviations.rst                        | 5 +++++
 xen/arch/x86/include/asm/uaccess.h               | 7 ++-----
 xen/include/xen/compiler.h                       | 7 +++++++
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.34.1

[XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE()

[Federico Serafini]

Introduce macro STATIC_ASSERT_UNREACHABLE() to check that a program
point is considered unreachable by the static analysis performed by the
compiler.

The use of such macro will lead to one of the following outcomes:
- the program point identified by the macro is considered unreachable,
  then the compiler removes the macro;
- the program point identified by the macro is not considered
  unreachable, then the compiler does not remove the macro, which will
  lead to a failure in the build process caused by an assembler error.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- removed constraint about optimization level -O0;
- use capital letters for macro name;
- add missing blanks;
- remove stray semicolon;
- cite the assertion failure in the error message.
---
 xen/include/xen/compiler.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 16d554f2a5..062f54449c 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -64,6 +64,13 @@
 # define fallthrough        do {} while (0)  /* fallthrough */
 #endif
 
+/*
+ * Add the following macro to check that a program point is considered
+ * unreachable by the static analysis performed by the compiler.
+ */
+#define STATIC_ASSERT_UNREACHABLE() \
+    asm ( ".error \"static assertion failed: unreachable\"" )
+
 #ifdef __clang__
 /* Clang can replace some vars with new automatic ones that go in .data;
  * mark all explicit-segment vars 'used' to prevent that. */
-- 
2.34.1

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

[Federico Serafini]

Use STATIC_ASSERT_UNREACHABLE() to improve readability and anticipate
the build failure (from a linker error to an assembler error) in case
of wrong size.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/include/asm/uaccess.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h
index 7443519d5b..52faf1d919 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -21,9 +21,6 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
 unsigned int copy_to_unsafe_ll(void *to, const void *from, unsigned int n);
 unsigned int copy_from_unsafe_ll(void *to, const void *from, unsigned int n);
 
-extern long __get_user_bad(void);
-extern void __put_user_bad(void);
-
 #define UA_KEEP(args...) args
 #define UA_DROP(args...)
 
@@ -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 )
-- 
2.34.1

[XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3

[Federico Serafini]

Update ECLAIR configuration to consider safe switch clauses ending
with STATIC_ASSERT_UNREACHABLE().

Update docs/misra/deviations.rst accordingly.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
 docs/misra/deviations.rst                        | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fd32ff8a9c..539efd7b30 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -368,6 +368,10 @@ safe."
 -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"}
 -doc_end
 
+-doc_begin="Switch clauses ending with unreachability assertion \"STATIC_ASSERT_UNREACHABLE()\" are safe."
+-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/STATIC_ASSERT_UNREACHABLE\\(\\);/))))"}
+-doc_end
+
 -doc_begin="Switch clauses not ending with the break statement are safe if an
 explicit comment indicating the fallthrough intention is present."
 -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through.? \\*/.*$,0..1))))"}
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 123c78e20a..c96efdd292 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -307,6 +307,11 @@ Deviations related to MISRA C:2012 Rules:
      - Switch clauses ending with failure method \"BUG()\" are safe.
      - Tagged as `safe` for ECLAIR.
 
+   * - R16.3
+     - Switch clauses ending with unreachability assertion
+       \"STATIC_ASSERT_UNREACHABLE()\" are safe.
+     - Tagged as `safe` for ECLAIR.
+
    * - R16.3
      - Existing switch clauses not ending with the break statement are safe if
        an explicit comment indicating the fallthrough intention is present.
-- 
2.34.1

Re: [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE()

[Federico Serafini]

On 26/01/24 11:05, Federico Serafini wrote:
> Introduce macro STATIC_ASSERT_UNREACHABLE(),
> use it to replace __{get,put}_user_bad(),
> update ECLAIR configuration to allow the use of such macro at the end of
> switch-caluses.
> 
> Federico Serafini (3):
>    xen: introduce STATIC_ASSERT_UNREACHABLE()
>    x86/uaccess: replace __{get,put}_user_bad() with
>      STATIC_ASSERT_UNREACHABLE()
>    automation/eclair: add deviation for MISRA C:2012 Rule 16.3
> 
>   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>   docs/misra/deviations.rst                        | 5 +++++
>   xen/arch/x86/include/asm/uaccess.h               | 7 ++-----
>   xen/include/xen/compiler.h                       | 7 +++++++
>   4 files changed, 18 insertions(+), 5 deletions(-)
> 

Hello everyone,

do you have any comments on this series?

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE()

[Jan Beulich]

On 31.01.2024 09:19, Federico Serafini wrote:
> On 26/01/24 11:05, Federico Serafini wrote:
>> Introduce macro STATIC_ASSERT_UNREACHABLE(),
>> use it to replace __{get,put}_user_bad(),
>> update ECLAIR configuration to allow the use of such macro at the end of
>> switch-caluses.
>>
>> Federico Serafini (3):
>>    xen: introduce STATIC_ASSERT_UNREACHABLE()
>>    x86/uaccess: replace __{get,put}_user_bad() with
>>      STATIC_ASSERT_UNREACHABLE()
>>    automation/eclair: add deviation for MISRA C:2012 Rule 16.3
>>
>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>>   docs/misra/deviations.rst                        | 5 +++++
>>   xen/arch/x86/include/asm/uaccess.h               | 7 ++-----
>>   xen/include/xen/compiler.h                       | 7 +++++++
>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>
> 
> Hello everyone,
> 
> do you have any comments on this series?

Yes, but in due course.

Jan

Re: [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE()

[Jan Beulich]

On 26.01.2024 11:05, Federico Serafini wrote:> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -64,6 +64,13 @@
>  # define fallthrough        do {} while (0)  /* fallthrough */
>  #endif
>  
> +/*
> + * Add the following macro to check that a program point is considered
> + * unreachable by the static analysis performed by the compiler.
> + */
> +#define STATIC_ASSERT_UNREACHABLE() \
> +    asm ( ".error \"static assertion failed: unreachable\"" )

In the comment s/Add/Use/? The macro is there after all when this gets
committed. Overall maybe

"Use this macro at program points considered unreachable, to be checked
 by the compiler's static analysis."

?

Also while asm()s without operands are implicitly volatile, I think it
would be a good idea to make that explicit nevertheless.

I'd be happy to adjust while committing, so long as you agree. If you
agree, and provided diagnostics resulting from this are useful (an
example would have been nice in the description):
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Jan Beulich]

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?

Jan

Re: [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE()

[Jan Beulich]

On 06.02.2024 14:22, Jan Beulich wrote:
> On 26.01.2024 11:05, Federico Serafini wrote:> --- a/xen/include/xen/compiler.h
>> +++ b/xen/include/xen/compiler.h
>> @@ -64,6 +64,13 @@
>>  # define fallthrough        do {} while (0)  /* fallthrough */
>>  #endif
>>  
>> +/*
>> + * Add the following macro to check that a program point is considered
>> + * unreachable by the static analysis performed by the compiler.
>> + */
>> +#define STATIC_ASSERT_UNREACHABLE() \
>> +    asm ( ".error \"static assertion failed: unreachable\"" )
> 
> In the comment s/Add/Use/? The macro is there after all when this gets
> committed. Overall maybe
> 
> "Use this macro at program points considered unreachable, to be checked
>  by the compiler's static analysis."
> 
> ?
> 
> Also while asm()s without operands are implicitly volatile, I think it
> would be a good idea to make that explicit nevertheless.
> 
> I'd be happy to adjust while committing, so long as you agree. If you
> agree, and provided diagnostics resulting from this are useful (an
> example would have been nice in the description):
> Acked-by: Jan Beulich <jbeulich@suse.com>

Actually, having seen patch 2, I need to withdraw this right away.

Jan

Re: [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3

[Stefano Stabellini]

On Fri, 26 Jan 2024, Federico Serafini wrote:
> Update ECLAIR configuration to consider safe switch clauses ending
> with STATIC_ASSERT_UNREACHABLE().
> 
> Update docs/misra/deviations.rst accordingly.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Stefano Stabellini]

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

Re: [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE()

[Stefano Stabellini]

On Tue, 6 Feb 2024, Jan Beulich wrote:
> On 06.02.2024 14:22, Jan Beulich wrote:
> > On 26.01.2024 11:05, Federico Serafini wrote:> --- a/xen/include/xen/compiler.h
> >> +++ b/xen/include/xen/compiler.h
> >> @@ -64,6 +64,13 @@
> >>  # define fallthrough        do {} while (0)  /* fallthrough */
> >>  #endif
> >>  
> >> +/*
> >> + * Add the following macro to check that a program point is considered
> >> + * unreachable by the static analysis performed by the compiler.
> >> + */
> >> +#define STATIC_ASSERT_UNREACHABLE() \
> >> +    asm ( ".error \"static assertion failed: unreachable\"" )
> > 
> > In the comment s/Add/Use/? The macro is there after all when this gets
> > committed. Overall maybe
> > 
> > "Use this macro at program points considered unreachable, to be checked
> >  by the compiler's static analysis."
> > 
> > ?
> > 
> > Also while asm()s without operands are implicitly volatile, I think it
> > would be a good idea to make that explicit nevertheless.
> > 
> > I'd be happy to adjust while committing, so long as you agree. If you
> > agree, and provided diagnostics resulting from this are useful (an
> > example would have been nice in the description):
> > Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Actually, having seen patch 2, I need to withdraw this right away.

To me it looks good enough but let's continue the discussion on patch
#2

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

[Jan Beulich]

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 ...

Jan

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

[Federico Serafini]

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
make[2]: *** Waiting for unfinished jobs....
...

If I understood your requests correctly,
the only thing missing is the function name but I didn't find a way
to make __FUNCTION__ or __func__ work with the .error directive.
Do you know any tricks to make it work?

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Jan Beulich]

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.

> If I understood your requests correctly,
> the only thing missing is the function name but I didn't find a way
> to make __FUNCTION__ or __func__ work with the .error directive.
> Do you know any tricks to make it work?

I didn't think any tricks would be required:

asm ( ".error " __FILE__ ":" __LINE__ ": in function " __FUNCTION__ );

Yet it looks like I was under the wrong impression that __FUNCTION__
differed from __func__ and would be like __FILE__ / __LINE__. I have
to admit I have no good idea then how to achieve helpful diagnostics.

Jan

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

[Federico Serafini]

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?

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Jan Beulich]

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

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

[Federico Serafini]

On 07/02/24 16:24, Jan Beulich wrote:
> 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

It points to the header file uaccess.h because at line 262 there is
an intentional wrong use of put_guest_size(), within the body of
__copy_to_guest_pv() function.
This example can be misleading because {get,put}_unsafe_size() are
defined in the same file but the diagnostics is doing the
right thing.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Jan Beulich]

On 07.02.2024 16:58, Federico Serafini wrote:
> On 07/02/24 16:24, Jan Beulich wrote:
>> 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
> 
> It points to the header file uaccess.h because at line 262 there is
> an intentional wrong use of put_guest_size(), within the body of
> __copy_to_guest_pv() function.

Yet that's again only a helper function being inlined into the ultimate
caller. That ultimate caller is what wants identifying in the diag. Not
the least because of ...

> This example can be misleading because {get,put}_unsafe_size() are
> defined in the same file but the diagnostics is doing the
> right thing.

... this. And really __copy_to_guest_pv() is the wrong place to put a
wrong put_guest_size() in, to try out how diagnostics would look like
in reality: That function falls back to copy_to_guest_ll() for all
cases it can't handle directly. You want to place a bogus put_guest()
somewhere in a .c file to see what results.

Jan

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

[Federico Serafini]

On 07/02/24 17:19, Jan Beulich wrote:
> On 07.02.2024 16:58, Federico Serafini wrote:
>> On 07/02/24 16:24, Jan Beulich wrote:
>>> 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
>>
>> It points to the header file uaccess.h because at line 262 there is
>> an intentional wrong use of put_guest_size(), within the body of
>> __copy_to_guest_pv() function.
> 
> Yet that's again only a helper function being inlined into the ultimate
> caller. That ultimate caller is what wants identifying in the diag. Not
> the least because of ...
> 
>> This example can be misleading because {get,put}_unsafe_size() are
>> defined in the same file but the diagnostics is doing the
>> right thing.
> 
> ... this. And really __copy_to_guest_pv() is the wrong place to put a
> wrong put_guest_size() in, to try out how diagnostics would look like
> in reality: That function falls back to copy_to_guest_ll() for all
> cases it can't handle directly. You want to place a bogus put_guest()
> somewhere in a .c file to see what results.

I added a bogus call to put_guest() at line 387 of
file xen/arch/x86/mm.c, inside function page_is_ram_type().
Assuming I did not choose another wrong place,
the diagnostic seems appropriate:

arch/x86/mm.c: Assembler messages:
arch/x86/mm.c:387: Error: static assertion failed: unreachable

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Jan Beulich]

On 08.02.2024 11:45, Federico Serafini wrote:
> On 07/02/24 17:19, Jan Beulich wrote:
>> On 07.02.2024 16:58, Federico Serafini wrote:
>>> On 07/02/24 16:24, Jan Beulich wrote:
>>>> 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
>>>
>>> It points to the header file uaccess.h because at line 262 there is
>>> an intentional wrong use of put_guest_size(), within the body of
>>> __copy_to_guest_pv() function.
>>
>> Yet that's again only a helper function being inlined into the ultimate
>> caller. That ultimate caller is what wants identifying in the diag. Not
>> the least because of ...
>>
>>> This example can be misleading because {get,put}_unsafe_size() are
>>> defined in the same file but the diagnostics is doing the
>>> right thing.
>>
>> ... this. And really __copy_to_guest_pv() is the wrong place to put a
>> wrong put_guest_size() in, to try out how diagnostics would look like
>> in reality: That function falls back to copy_to_guest_ll() for all
>> cases it can't handle directly. You want to place a bogus put_guest()
>> somewhere in a .c file to see what results.
> 
> I added a bogus call to put_guest() at line 387 of
> file xen/arch/x86/mm.c, inside function page_is_ram_type().
> Assuming I did not choose another wrong place,
> the diagnostic seems appropriate:
> 
> arch/x86/mm.c: Assembler messages:
> arch/x86/mm.c:387: Error: static assertion failed: unreachable

Oh, okay, this looks appropriate then as to identifying where the
source construct is. However, we then still don't know where the
assertion in question is (there could be multiple in what the
original construct expands to). So I'm still inclined to ask that
__FILE__ / __LINE__ and/or the name of the invoking construct
(macro or function) be made visible in the diagnostic.

Jan

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

[Federico Serafini]

On 08/02/24 12:14, Jan Beulich wrote:
> On 08.02.2024 11:45, Federico Serafini wrote:
>> On 07/02/24 17:19, Jan Beulich wrote:
>>> On 07.02.2024 16:58, Federico Serafini wrote:
>>>> On 07/02/24 16:24, Jan Beulich wrote:
>>>>> 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
>>>>
>>>> It points to the header file uaccess.h because at line 262 there is
>>>> an intentional wrong use of put_guest_size(), within the body of
>>>> __copy_to_guest_pv() function.
>>>
>>> Yet that's again only a helper function being inlined into the ultimate
>>> caller. That ultimate caller is what wants identifying in the diag. Not
>>> the least because of ...
>>>
>>>> This example can be misleading because {get,put}_unsafe_size() are
>>>> defined in the same file but the diagnostics is doing the
>>>> right thing.
>>>
>>> ... this. And really __copy_to_guest_pv() is the wrong place to put a
>>> wrong put_guest_size() in, to try out how diagnostics would look like
>>> in reality: That function falls back to copy_to_guest_ll() for all
>>> cases it can't handle directly. You want to place a bogus put_guest()
>>> somewhere in a .c file to see what results.
>>
>> I added a bogus call to put_guest() at line 387 of
>> file xen/arch/x86/mm.c, inside function page_is_ram_type().
>> Assuming I did not choose another wrong place,
>> the diagnostic seems appropriate:
>>
>> arch/x86/mm.c: Assembler messages:
>> arch/x86/mm.c:387: Error: static assertion failed: unreachable
> 
> Oh, okay, this looks appropriate then as to identifying where the
> source construct is. However, we then still don't know where the
> assertion in question is (there could be multiple in what the
> original construct expands to). So I'm still inclined to ask that
> __FILE__ / __LINE__ and/or the name of the invoking construct
> (macro or function) be made visible in the diagnostic.

Any use of __FILE__ and __LINE__ results in obtaining
the same information already reported by the assembler error message.

We could add an argument to the new macro to manually add some context
at every use of the macro, but I think this would be annoying.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Jan Beulich]

On 09.02.2024 10:50, Federico Serafini wrote:
> On 08/02/24 12:14, Jan Beulich wrote:
>> On 08.02.2024 11:45, Federico Serafini wrote:
>>> On 07/02/24 17:19, Jan Beulich wrote:
>>>> On 07.02.2024 16:58, Federico Serafini wrote:
>>>>> On 07/02/24 16:24, Jan Beulich wrote:
>>>>>> 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
>>>>>
>>>>> It points to the header file uaccess.h because at line 262 there is
>>>>> an intentional wrong use of put_guest_size(), within the body of
>>>>> __copy_to_guest_pv() function.
>>>>
>>>> Yet that's again only a helper function being inlined into the ultimate
>>>> caller. That ultimate caller is what wants identifying in the diag. Not
>>>> the least because of ...
>>>>
>>>>> This example can be misleading because {get,put}_unsafe_size() are
>>>>> defined in the same file but the diagnostics is doing the
>>>>> right thing.
>>>>
>>>> ... this. And really __copy_to_guest_pv() is the wrong place to put a
>>>> wrong put_guest_size() in, to try out how diagnostics would look like
>>>> in reality: That function falls back to copy_to_guest_ll() for all
>>>> cases it can't handle directly. You want to place a bogus put_guest()
>>>> somewhere in a .c file to see what results.
>>>
>>> I added a bogus call to put_guest() at line 387 of
>>> file xen/arch/x86/mm.c, inside function page_is_ram_type().
>>> Assuming I did not choose another wrong place,
>>> the diagnostic seems appropriate:
>>>
>>> arch/x86/mm.c: Assembler messages:
>>> arch/x86/mm.c:387: Error: static assertion failed: unreachable
>>
>> Oh, okay, this looks appropriate then as to identifying where the
>> source construct is. However, we then still don't know where the
>> assertion in question is (there could be multiple in what the
>> original construct expands to). So I'm still inclined to ask that
>> __FILE__ / __LINE__ and/or the name of the invoking construct
>> (macro or function) be made visible in the diagnostic.
> 
> Any use of __FILE__ and __LINE__ results in obtaining
> the same information already reported by the assembler error message.

Hmm, yes, since put_guest() is itself a macro.

> We could add an argument to the new macro to manually add some context
> at every use of the macro, but I think this would be annoying.

That's a last resort. An alternative would be to see about converting
from macros to inline functions, where this would make a difference
here.

Jan

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

[Federico Serafini]

On 12/02/24 09:43, Jan Beulich wrote:
> On 09.02.2024 10:50, Federico Serafini wrote:
>> On 08/02/24 12:14, Jan Beulich wrote:
>>> On 08.02.2024 11:45, Federico Serafini wrote:
>>>> On 07/02/24 17:19, Jan Beulich wrote:
>>>>> On 07.02.2024 16:58, Federico Serafini wrote:
>>>>>> On 07/02/24 16:24, Jan Beulich wrote:
>>>>>>> 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
>>>>>>
>>>>>> It points to the header file uaccess.h because at line 262 there is
>>>>>> an intentional wrong use of put_guest_size(), within the body of
>>>>>> __copy_to_guest_pv() function.
>>>>>
>>>>> Yet that's again only a helper function being inlined into the ultimate
>>>>> caller. That ultimate caller is what wants identifying in the diag. Not
>>>>> the least because of ...
>>>>>
>>>>>> This example can be misleading because {get,put}_unsafe_size() are
>>>>>> defined in the same file but the diagnostics is doing the
>>>>>> right thing.
>>>>>
>>>>> ... this. And really __copy_to_guest_pv() is the wrong place to put a
>>>>> wrong put_guest_size() in, to try out how diagnostics would look like
>>>>> in reality: That function falls back to copy_to_guest_ll() for all
>>>>> cases it can't handle directly. You want to place a bogus put_guest()
>>>>> somewhere in a .c file to see what results.
>>>>
>>>> I added a bogus call to put_guest() at line 387 of
>>>> file xen/arch/x86/mm.c, inside function page_is_ram_type().
>>>> Assuming I did not choose another wrong place,
>>>> the diagnostic seems appropriate:
>>>>
>>>> arch/x86/mm.c: Assembler messages:
>>>> arch/x86/mm.c:387: Error: static assertion failed: unreachable
>>>
>>> Oh, okay, this looks appropriate then as to identifying where the
>>> source construct is. However, we then still don't know where the
>>> assertion in question is (there could be multiple in what the
>>> original construct expands to). So I'm still inclined to ask that
>>> __FILE__ / __LINE__ and/or the name of the invoking construct
>>> (macro or function) be made visible in the diagnostic.
>>
>> Any use of __FILE__ and __LINE__ results in obtaining
>> the same information already reported by the assembler error message.
> 
> Hmm, yes, since put_guest() is itself a macro.
> 
>> We could add an argument to the new macro to manually add some context
>> at every use of the macro, but I think this would be annoying.
> 
> That's a last resort. An alternative would be to see about converting
> from macros to inline functions, where this would make a difference
> here.

I did some tries with example programs
and the assembler error always points to file and line
of the most enclosing function that caused the failure.
If I am not missing something, using __FILE__ and __LINE__ does not add
any information.

Therefore, if the new macro is used within the body of other macros,
then the resulting assembler error will point to the source of
the problem (e.g., the site of a bogus call to put_guest()).

In my opinion, converting put_guest() &Co. to inline functions is not
convenient: the assembler error will point to the most enclosing
function that would be put_unsafe_size(), instead of pointing to the
source of the problem.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Stefano Stabellini]

On Wed, 14 Feb 2024, Federico Serafini wrote:
> On 12/02/24 09:43, Jan Beulich wrote:
> > On 09.02.2024 10:50, Federico Serafini wrote:
> > > On 08/02/24 12:14, Jan Beulich wrote:
> > > > On 08.02.2024 11:45, Federico Serafini wrote:
> > > > > On 07/02/24 17:19, Jan Beulich wrote:
> > > > > > On 07.02.2024 16:58, Federico Serafini wrote:
> > > > > > > On 07/02/24 16:24, Jan Beulich wrote:
> > > > > > > > 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
> > > > > > > 
> > > > > > > It points to the header file uaccess.h because at line 262 there
> > > > > > > is
> > > > > > > an intentional wrong use of put_guest_size(), within the body of
> > > > > > > __copy_to_guest_pv() function.
> > > > > > 
> > > > > > Yet that's again only a helper function being inlined into the
> > > > > > ultimate
> > > > > > caller. That ultimate caller is what wants identifying in the diag.
> > > > > > Not
> > > > > > the least because of ...
> > > > > > 
> > > > > > > This example can be misleading because {get,put}_unsafe_size() are
> > > > > > > defined in the same file but the diagnostics is doing the
> > > > > > > right thing.
> > > > > > 
> > > > > > ... this. And really __copy_to_guest_pv() is the wrong place to put
> > > > > > a
> > > > > > wrong put_guest_size() in, to try out how diagnostics would look
> > > > > > like
> > > > > > in reality: That function falls back to copy_to_guest_ll() for all
> > > > > > cases it can't handle directly. You want to place a bogus
> > > > > > put_guest()
> > > > > > somewhere in a .c file to see what results.
> > > > > 
> > > > > I added a bogus call to put_guest() at line 387 of
> > > > > file xen/arch/x86/mm.c, inside function page_is_ram_type().
> > > > > Assuming I did not choose another wrong place,
> > > > > the diagnostic seems appropriate:
> > > > > 
> > > > > arch/x86/mm.c: Assembler messages:
> > > > > arch/x86/mm.c:387: Error: static assertion failed: unreachable
> > > > 
> > > > Oh, okay, this looks appropriate then as to identifying where the
> > > > source construct is. However, we then still don't know where the
> > > > assertion in question is (there could be multiple in what the
> > > > original construct expands to). So I'm still inclined to ask that
> > > > __FILE__ / __LINE__ and/or the name of the invoking construct
> > > > (macro or function) be made visible in the diagnostic.
> > > 
> > > Any use of __FILE__ and __LINE__ results in obtaining
> > > the same information already reported by the assembler error message.
> > 
> > Hmm, yes, since put_guest() is itself a macro.
> > 
> > > We could add an argument to the new macro to manually add some context
> > > at every use of the macro, but I think this would be annoying.
> > 
> > That's a last resort. An alternative would be to see about converting
> > from macros to inline functions, where this would make a difference
> > here.
> 
> I did some tries with example programs
> and the assembler error always points to file and line
> of the most enclosing function that caused the failure.
> If I am not missing something, using __FILE__ and __LINE__ does not add
> any information.
> 
> Therefore, if the new macro is used within the body of other macros,
> then the resulting assembler error will point to the source of
> the problem (e.g., the site of a bogus call to put_guest()).
> 
> In my opinion, converting put_guest() &Co. to inline functions is not
> convenient: the assembler error will point to the most enclosing
> function that would be put_unsafe_size(), instead of pointing to the
> source of the problem.

I don't think is a good idea to add further changes to this patch. I
think we should go ahead with it as-is.

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

[Jan Beulich]

On 14.02.2024 17:11, Federico Serafini wrote:
> I did some tries with example programs
> and the assembler error always points to file and line
> of the most enclosing function that caused the failure.
> If I am not missing something, using __FILE__ and __LINE__ does not add
> any information.
> 
> Therefore, if the new macro is used within the body of other macros,
> then the resulting assembler error will point to the source of
> the problem (e.g., the site of a bogus call to put_guest()).
> 
> In my opinion, converting put_guest() &Co. to inline functions is not
> convenient: the assembler error will point to the most enclosing
> function that would be put_unsafe_size(), instead of pointing to the
> source of the problem.

The assembler error will point to where the inline function was expanded,
sure. __FILE__ / __LINE__ ought to point to that inline function (where
the macro was used) then, though?

Jan

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

[Jan Beulich]

On 15.02.2024 01:05, Stefano Stabellini wrote:
> I don't think is a good idea to add further changes to this patch. I
> think we should go ahead with it as-is.

I didn't suggest adding anything right here; there may want/need to be new
prereq-s, though. I'd like to make sure that we don't (significantly)
regress in terms of being able to diagnose programming mistakes.

Jan

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

[Federico Serafini]

On 15/02/24 09:10, Jan Beulich wrote:
> On 14.02.2024 17:11, Federico Serafini wrote:
>> I did some tries with example programs
>> and the assembler error always points to file and line
>> of the most enclosing function that caused the failure.
>> If I am not missing something, using __FILE__ and __LINE__ does not add
>> any information.
>>
>> Therefore, if the new macro is used within the body of other macros,
>> then the resulting assembler error will point to the source of
>> the problem (e.g., the site of a bogus call to put_guest()).
>>
>> In my opinion, converting put_guest() &Co. to inline functions is not
>> convenient: the assembler error will point to the most enclosing
>> function that would be put_unsafe_size(), instead of pointing to the
>> source of the problem.
> 
> The assembler error will point to where the inline function was expanded,
> sure. __FILE__ / __LINE__ ought to point to that inline function (where
> the macro was used) then, though?

This is what I get:

federico@Dell:~$ cat m.c
#define STRINGIFY(arg) #arg
#define STATIC_ASSERT_UNREACHABLE(file, line) \
   asm(".error \"static assertion failed: " file ": " STRINGIFY(line) "\"")

static inline __attribute__((always_inline)) void g(int x) {
   switch(x) {
     case 0:
       STATIC_ASSERT_UNREACHABLE(__FILE__, __LINE__);
   }
}

static inline __attribute__((always_inline)) void f(int x) {
   g(x);
}

int main(void) {
   f(0);
   return 0;
}
federico@Dell:~$ gcc -O3 m.c
m.c: Assembler messages:
m.c:8: Error: static assertion failed: m.c: 8


Note that the linker behaves differently:

federico@Dell:~$ cat m.c
extern void __put_user_bad(void);

static inline __attribute__((always_inline)) void g(int x) {
   switch(x) {
     case 0:
       __put_user_bad();
   }
}

static inline __attribute__((always_inline)) void f(int x) {
   g(x);
}

int main(void) {
   f(0);
   return 0;
}
federico@Dell:~$ gcc -O3 m.c
/usr/bin/ld: /tmp/ccv9KHJD.o: in function `main':
m.c:(.text.startup+0x9): undefined reference to `__put_user_bad'
collect2: error: ld returned 1 exit status

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Jan Beulich]

On 15.02.2024 11:07, Federico Serafini wrote:
> On 15/02/24 09:10, Jan Beulich wrote:
>> On 14.02.2024 17:11, Federico Serafini wrote:
>>> I did some tries with example programs
>>> and the assembler error always points to file and line
>>> of the most enclosing function that caused the failure.
>>> If I am not missing something, using __FILE__ and __LINE__ does not add
>>> any information.
>>>
>>> Therefore, if the new macro is used within the body of other macros,
>>> then the resulting assembler error will point to the source of
>>> the problem (e.g., the site of a bogus call to put_guest()).
>>>
>>> In my opinion, converting put_guest() &Co. to inline functions is not
>>> convenient: the assembler error will point to the most enclosing
>>> function that would be put_unsafe_size(), instead of pointing to the
>>> source of the problem.
>>
>> The assembler error will point to where the inline function was expanded,
>> sure. __FILE__ / __LINE__ ought to point to that inline function (where
>> the macro was used) then, though?
> 
> This is what I get:
> 
> federico@Dell:~$ cat m.c
> #define STRINGIFY(arg) #arg
> #define STATIC_ASSERT_UNREACHABLE(file, line) \
>    asm(".error \"static assertion failed: " file ": " STRINGIFY(line) "\"")

__FILE__ / __LINE__, if to be used, want using here, not at the use
site.

> static inline __attribute__((always_inline)) void g(int x) {
>    switch(x) {
>      case 0:
>        STATIC_ASSERT_UNREACHABLE(__FILE__, __LINE__);
>    }
> }
> 
> static inline __attribute__((always_inline)) void f(int x) {
>    g(x);
> }
> 
> int main(void) {
>    f(0);
>    return 0;
> }
> federico@Dell:~$ gcc -O3 m.c
> m.c: Assembler messages:
> m.c:8: Error: static assertion failed: m.c: 8

That's as expected. There's no mix of macros and inline functions in
your example.

> Note that the linker behaves differently:
> 
> federico@Dell:~$ cat m.c
> extern void __put_user_bad(void);
> 
> static inline __attribute__((always_inline)) void g(int x) {
>    switch(x) {
>      case 0:
>        __put_user_bad();
>    }
> }
> 
> static inline __attribute__((always_inline)) void f(int x) {
>    g(x);
> }
> 
> int main(void) {
>    f(0);
>    return 0;
> }
> federico@Dell:~$ gcc -O3 m.c
> /usr/bin/ld: /tmp/ccv9KHJD.o: in function `main':
> m.c:(.text.startup+0x9): undefined reference to `__put_user_bad'
> collect2: error: ld returned 1 exit status

The important difference is: Here we're told that there was a use of
__put_user_bad, which is easy to grep for, and thus see how the
supplied function / file / line(?) relate to the ultimate problem.

I'm afraid I'm meanwhile confused enough by the various replies
containing results of experimentation that I can't really tell
anymore what case is best. Hence I can only restate my expectation for
an eventual v3: Diagnosing what the issue is, no matter whether the new
macro is used in another macro or in an inline function, should not
become meaningfully more difficult. In how far this is the case wants
clarifying in the description of the change.

Jan

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

[Federico Serafini]

On 15/02/24 11:32, Jan Beulich wrote:
> The important difference is: Here we're told that there was a use of
> __put_user_bad, which is easy to grep for, and thus see how the
> supplied function / file / line(?) relate to the ultimate problem.
> 
> I'm afraid I'm meanwhile confused enough by the various replies
> containing results of experimentation that I can't really tell
> anymore what case is best. Hence I can only restate my expectation for
> an eventual v3: Diagnosing what the issue is, no matter whether the new
> macro is used in another macro or in an inline function, should not
> become meaningfully more difficult. In how far this is the case wants
> clarifying in the description of the change.

I think the best thing at the moment is to deviate
__{get,put}_user_bad() for Rule 16.3.
I'll let maintainers further explore the possibility of having a
compile-time assertion based on the assembler error.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

[Stefano Stabellini]

On Mon, 19 Feb 2024, Federico Serafini wrote:
> On 15/02/24 11:32, Jan Beulich wrote:
> > The important difference is: Here we're told that there was a use of
> > __put_user_bad, which is easy to grep for, and thus see how the
> > supplied function / file / line(?) relate to the ultimate problem.
> > 
> > I'm afraid I'm meanwhile confused enough by the various replies
> > containing results of experimentation that I can't really tell
> > anymore what case is best. Hence I can only restate my expectation for
> > an eventual v3: Diagnosing what the issue is, no matter whether the new
> > macro is used in another macro or in an inline function, should not
> > become meaningfully more difficult. In how far this is the case wants
> > clarifying in the description of the change.
> 
> I think the best thing at the moment is to deviate
> __{get,put}_user_bad() for Rule 16.3.
> I'll let maintainers further explore the possibility of having a
> compile-time assertion based on the assembler error.

OK. I hope Jan is OK to deviate by in-code comment.

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

[Jan Beulich]

On 19.02.2024 21:43, Stefano Stabellini wrote:
> On Mon, 19 Feb 2024, Federico Serafini wrote:
>> On 15/02/24 11:32, Jan Beulich wrote:
>>> The important difference is: Here we're told that there was a use of
>>> __put_user_bad, which is easy to grep for, and thus see how the
>>> supplied function / file / line(?) relate to the ultimate problem.
>>>
>>> I'm afraid I'm meanwhile confused enough by the various replies
>>> containing results of experimentation that I can't really tell
>>> anymore what case is best. Hence I can only restate my expectation for
>>> an eventual v3: Diagnosing what the issue is, no matter whether the new
>>> macro is used in another macro or in an inline function, should not
>>> become meaningfully more difficult. In how far this is the case wants
>>> clarifying in the description of the change.
>>
>> I think the best thing at the moment is to deviate
>> __{get,put}_user_bad() for Rule 16.3.
>> I'll let maintainers further explore the possibility of having a
>> compile-time assertion based on the assembler error.
> 
> OK. I hope Jan is OK to deviate by in-code comment.

Hmm, the follow-on suggestion was to add break statements? Followed
by me asking whether adding noreturn to the decls wouldn't also help.
(Then again I was under the impression that there was more than just
the "missing" break statements which Misra thought was an issue here.)

Jan