* [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE()
@ 2024-01-26 10:05 Federico Serafini
2024-01-26 10:05 ` [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() Federico Serafini
` (3 more replies)
0 siblings, 4 replies; 32+ messages in thread
From: Federico Serafini @ 2024-01-26 10:05 UTC (permalink / raw)
To: xen-devel
Cc: consulting, Federico Serafini, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Roger Pau Monné, Simone Ballarin, Doug Goldstein
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() 2024-01-26 10:05 [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini @ 2024-01-26 10:05 ` Federico Serafini 2024-02-06 13:22 ` Jan Beulich 2024-01-26 10:05 ` [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() Federico Serafini ` (2 subsequent siblings) 3 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-01-26 10:05 UTC (permalink / raw) To: xen-devel Cc: consulting, Federico Serafini, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() 2024-01-26 10:05 ` [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() Federico Serafini @ 2024-02-06 13:22 ` Jan Beulich 2024-02-06 13:26 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-06 13:22 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() 2024-02-06 13:22 ` Jan Beulich @ 2024-02-06 13:26 ` Jan Beulich 2024-02-07 1:09 ` Stefano Stabellini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-06 13:26 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() 2024-02-06 13:26 ` Jan Beulich @ 2024-02-07 1:09 ` Stefano Stabellini 0 siblings, 0 replies; 32+ messages in thread From: Stefano Stabellini @ 2024-02-07 1:09 UTC (permalink / raw) To: Jan Beulich Cc: Federico Serafini, consulting, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-01-26 10:05 [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini 2024-01-26 10:05 ` [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() Federico Serafini @ 2024-01-26 10:05 ` Federico Serafini 2024-02-06 13:25 ` Jan Beulich 2024-01-26 10:05 ` [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 Federico Serafini 2024-01-31 8:19 ` [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini 3 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-01-26 10:05 UTC (permalink / raw) To: xen-devel Cc: consulting, Federico Serafini, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-01-26 10:05 ` [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() Federico Serafini @ 2024-02-06 13:25 ` Jan Beulich 2024-02-07 1:08 ` Stefano Stabellini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-06 13:25 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-06 13:25 ` Jan Beulich @ 2024-02-07 1:08 ` Stefano Stabellini 2024-02-07 7:38 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Stefano Stabellini @ 2024-02-07 1:08 UTC (permalink / raw) To: Jan Beulich Cc: Federico Serafini, consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 1:08 ` Stefano Stabellini @ 2024-02-07 7:38 ` Jan Beulich 2024-02-07 13:51 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-07 7:38 UTC (permalink / raw) To: Stefano Stabellini Cc: Federico Serafini, consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 7:38 ` Jan Beulich @ 2024-02-07 13:51 ` Federico Serafini 2024-02-07 14:16 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-07 13:51 UTC (permalink / raw) To: Jan Beulich, Stefano Stabellini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 13:51 ` Federico Serafini @ 2024-02-07 14:16 ` Jan Beulich 2024-02-07 15:08 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-07 14:16 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 14:16 ` Jan Beulich @ 2024-02-07 15:08 ` Federico Serafini 2024-02-07 15:24 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-07 15:08 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 15:08 ` Federico Serafini @ 2024-02-07 15:24 ` Jan Beulich 2024-02-07 15:58 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-07 15:24 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 15:24 ` Jan Beulich @ 2024-02-07 15:58 ` Federico Serafini 2024-02-07 16:19 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-07 15:58 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 15:58 ` Federico Serafini @ 2024-02-07 16:19 ` Jan Beulich 2024-02-08 10:45 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-07 16:19 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-07 16:19 ` Jan Beulich @ 2024-02-08 10:45 ` Federico Serafini 2024-02-08 11:14 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-08 10:45 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-08 10:45 ` Federico Serafini @ 2024-02-08 11:14 ` Jan Beulich 2024-02-09 9:50 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-08 11:14 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-08 11:14 ` Jan Beulich @ 2024-02-09 9:50 ` Federico Serafini 2024-02-12 8:43 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-09 9:50 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-09 9:50 ` Federico Serafini @ 2024-02-12 8:43 ` Jan Beulich 2024-02-14 16:11 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-12 8:43 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-12 8:43 ` Jan Beulich @ 2024-02-14 16:11 ` Federico Serafini 2024-02-15 0:05 ` Stefano Stabellini 2024-02-15 8:10 ` Jan Beulich 0 siblings, 2 replies; 32+ messages in thread From: Federico Serafini @ 2024-02-14 16:11 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-14 16:11 ` Federico Serafini @ 2024-02-15 0:05 ` Stefano Stabellini 2024-02-15 9:26 ` Jan Beulich 2024-02-15 8:10 ` Jan Beulich 1 sibling, 1 reply; 32+ messages in thread From: Stefano Stabellini @ 2024-02-15 0:05 UTC (permalink / raw) To: Federico Serafini Cc: Jan Beulich, consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-15 0:05 ` Stefano Stabellini @ 2024-02-15 9:26 ` Jan Beulich 0 siblings, 0 replies; 32+ messages in thread From: Jan Beulich @ 2024-02-15 9:26 UTC (permalink / raw) To: Stefano Stabellini, Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-14 16:11 ` Federico Serafini 2024-02-15 0:05 ` Stefano Stabellini @ 2024-02-15 8:10 ` Jan Beulich 2024-02-15 10:07 ` Federico Serafini 1 sibling, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-15 8:10 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-15 8:10 ` Jan Beulich @ 2024-02-15 10:07 ` Federico Serafini 2024-02-15 10:32 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-15 10:07 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-15 10:07 ` Federico Serafini @ 2024-02-15 10:32 ` Jan Beulich 2024-02-19 8:35 ` Federico Serafini 0 siblings, 1 reply; 32+ messages in thread From: Jan Beulich @ 2024-02-15 10:32 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-15 10:32 ` Jan Beulich @ 2024-02-19 8:35 ` Federico Serafini 2024-02-19 20:43 ` Stefano Stabellini 0 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-02-19 8:35 UTC (permalink / raw) To: Jan Beulich Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, Stefano Stabellini 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-19 8:35 ` Federico Serafini @ 2024-02-19 20:43 ` Stefano Stabellini 2024-02-20 7:18 ` Jan Beulich 0 siblings, 1 reply; 32+ messages in thread From: Stefano Stabellini @ 2024-02-19 20:43 UTC (permalink / raw) To: Federico Serafini Cc: Jan Beulich, consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel, 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() 2024-02-19 20:43 ` Stefano Stabellini @ 2024-02-20 7:18 ` Jan Beulich 0 siblings, 0 replies; 32+ messages in thread From: Jan Beulich @ 2024-02-20 7:18 UTC (permalink / raw) To: Stefano Stabellini, Federico Serafini Cc: consulting, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 2024-01-26 10:05 [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini 2024-01-26 10:05 ` [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() Federico Serafini 2024-01-26 10:05 ` [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() Federico Serafini @ 2024-01-26 10:05 ` Federico Serafini 2024-02-07 1:07 ` Stefano Stabellini 2024-01-31 8:19 ` [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini 3 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-01-26 10:05 UTC (permalink / raw) To: xen-devel Cc: consulting, Federico Serafini, Simone Ballarin, Doug Goldstein, Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Wei Liu 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 2024-01-26 10:05 ` [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 Federico Serafini @ 2024-02-07 1:07 ` Stefano Stabellini 0 siblings, 0 replies; 32+ messages in thread From: Stefano Stabellini @ 2024-02-07 1:07 UTC (permalink / raw) To: Federico Serafini Cc: xen-devel, consulting, Simone Ballarin, Doug Goldstein, Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Wei Liu 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> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() 2024-01-26 10:05 [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini ` (2 preceding siblings ...) 2024-01-26 10:05 ` [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 Federico Serafini @ 2024-01-31 8:19 ` Federico Serafini 2024-01-31 9:04 ` Jan Beulich 3 siblings, 1 reply; 32+ messages in thread From: Federico Serafini @ 2024-01-31 8:19 UTC (permalink / raw) To: xen-devel Cc: consulting, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu, Roger Pau Monné, Simone Ballarin, Doug Goldstein 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() 2024-01-31 8:19 ` [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini @ 2024-01-31 9:04 ` Jan Beulich 0 siblings, 0 replies; 32+ messages in thread From: Jan Beulich @ 2024-01-31 9:04 UTC (permalink / raw) To: Federico Serafini Cc: consulting, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu, Roger Pau Monné, Simone Ballarin, Doug Goldstein, xen-devel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2024-02-20 7:19 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-26 10:05 [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini
2024-01-26 10:05 ` [XEN PATCH v2 1/3] xen: introduce STATIC_ASSERT_UNREACHABLE() Federico Serafini
2024-02-06 13:22 ` Jan Beulich
2024-02-06 13:26 ` Jan Beulich
2024-02-07 1:09 ` Stefano Stabellini
2024-01-26 10:05 ` [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE() Federico Serafini
2024-02-06 13:25 ` Jan Beulich
2024-02-07 1:08 ` Stefano Stabellini
2024-02-07 7:38 ` Jan Beulich
2024-02-07 13:51 ` Federico Serafini
2024-02-07 14:16 ` Jan Beulich
2024-02-07 15:08 ` Federico Serafini
2024-02-07 15:24 ` Jan Beulich
2024-02-07 15:58 ` Federico Serafini
2024-02-07 16:19 ` Jan Beulich
2024-02-08 10:45 ` Federico Serafini
2024-02-08 11:14 ` Jan Beulich
2024-02-09 9:50 ` Federico Serafini
2024-02-12 8:43 ` Jan Beulich
2024-02-14 16:11 ` Federico Serafini
2024-02-15 0:05 ` Stefano Stabellini
2024-02-15 9:26 ` Jan Beulich
2024-02-15 8:10 ` Jan Beulich
2024-02-15 10:07 ` Federico Serafini
2024-02-15 10:32 ` Jan Beulich
2024-02-19 8:35 ` Federico Serafini
2024-02-19 20:43 ` Stefano Stabellini
2024-02-20 7:18 ` Jan Beulich
2024-01-26 10:05 ` [XEN PATCH v2 3/3] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 Federico Serafini
2024-02-07 1:07 ` Stefano Stabellini
2024-01-31 8:19 ` [XEN PATCH v2 0/3] Introduce and use STATIC_ASSERT_UNREACHABLE() Federico Serafini
2024-01-31 9:04 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.