(+ Bertrand)
Hi Jan,
On 27/01/2022 14:34, Jan Beulich wrote:
The increasing amount of constructs along the lines of
if ( !condition )
{
ASSERT_UNREACHABLE();
return;
}
is not only longer than necessary, but also doesn't produce incident
specific console output (except for file name and line number).
So I agree that this construct will always result to a minimum 5 lines.
Which is not nice. But the proposed change is...
Allow
the intended effect to be achieved with ASSERT(), by giving it a second
parameter allowing specification of the action to take in release builds
in case an assertion would have triggered in a debug one. The example
above then becomes
ASSERT(condition, return);
Make sure the condition will continue to not get evaluated when just a
single argument gets passed to ASSERT().
Signed-off-by: Jan Beulich <jbeul...@suse.com>
---
v2: Rename new macro parameter.
---
RFC: The use of a control flow construct as a macro argument may be
controversial.
indeed controversial. I find this quite confusing and not something I
would request a user to switch to if they use the longer version.
That said, this is mainly a matter of taste. So I am interested to hear
others view.
I have also CCed Bertrand to have an opinions from the Fusa Group (I
suspect this will go backward for them).
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -826,11 +826,7 @@ int xenmem_add_to_physmap(struct domain
union add_to_physmap_extra extra = {};
struct page_info *pages[16];
- if ( !paging_mode_translate(d) )
- {
- ASSERT_UNREACHABLE();
- return -EACCES;
- }
+ ASSERT(paging_mode_translate(d), return -EACCES);
if ( xatp->space == XENMAPSPACE_gmfn_foreign )
extra.foreign_domid = DOMID_INVALID;
@@ -920,11 +916,7 @@ static int xenmem_add_to_physmap_batch(s
* call doesn't succumb to dead-code-elimination. Duplicate the
short-circut
* from xatp_permission_check() to try and help the compiler out.
*/
- if ( !paging_mode_translate(d) )
- {
- ASSERT_UNREACHABLE();
- return -EACCES;
- }
+ ASSERT(paging_mode_translate(d), return -EACCES);
if ( unlikely(xatpb->size < extent) )
return -EILSEQ;
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -49,11 +49,13 @@
#endif
#ifndef NDEBUG
-#define ASSERT(p) \
+#define ASSERT(p, ...) \
do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
#define ASSERT_UNREACHABLE() assert_failed("unreachable")
#else
-#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
+#define ASSERT(p, failsafe...) do { \
+ if ( !count_args(failsafe) || unlikely(!(p)) ) { failsafe; } \
+ } while ( 0 )
#define ASSERT_UNREACHABLE() do { } while (0)
#endif
Cheers,
--
Julien Grall