[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
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)
- References:
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
- Re: [XEN PATCH v2 2/3] x86/uaccess: replace __{get,put}_user_bad() with STATIC_ASSERT_UNREACHABLE()
|
