| Message ID | 20220824102225.11431-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: let ASSERT_UNREACHABLE() WARN() in non-debug builds | expand |
Hi Juergen, Thanks for sending the patch quickly :). On 24/08/2022 11:22, Juergen Gross wrote: > Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production > builds a warning seems to be appropriate when hitting one. > > In order not to flood the console in reproducible cases, introduce > WARN_ONCE() to be used in this case. > > Signed-off-by: Juergen Gross <jgross@suse.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
On 24.08.2022 12:22, Juergen Gross wrote: > Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production > builds a warning seems to be appropriate when hitting one. I disagree, for two reasons: This violates the implication of NDEBUG meaning ASSERT() and friends expand to no actual code. Plus if doing so for ASSERT_UNREACHABLE(), why would we not do the same for ASSERT()? There's a reason we have ASSERT() and friends and, independently, WARN_ON() / BUG_ON() et al. Jan
Hi, > On 24 Aug 2022, at 11:35, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.08.2022 12:22, Juergen Gross wrote: >> Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production >> builds a warning seems to be appropriate when hitting one. > > I disagree, for two reasons: This violates the implication of NDEBUG > meaning ASSERT() and friends expand to no actual code. Plus if doing so > for ASSERT_UNREACHABLE(), why would we not do the same for ASSERT()? > There's a reason we have ASSERT() and friends and, independently, > WARN_ON() / BUG_ON() et al. I agree with Jan here, this is introducing code in ASSERT which is not the intention and will end up with dead code in production mode. In NDEBUG those should appear. If something is needed or we think there could be a situation where this is reachable, then the code should be modified to use something else then ASSERT[_UNREACHABLE](). Bertrand > > Jan >
On 24.08.22 12:35, Jan Beulich wrote: > On 24.08.2022 12:22, Juergen Gross wrote: >> Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production >> builds a warning seems to be appropriate when hitting one. > > I disagree, for two reasons: This violates the implication of NDEBUG > meaning ASSERT() and friends expand to no actual code. Plus if doing so This is something we can change IMHO. > for ASSERT_UNREACHABLE(), why would we not do the same for ASSERT()? There are multiple reasons to have ASSERT()s. Some serve as a kind of documentation (e.g. to document that the programmer thought of a special case not being possible), or they are meant to catch hard to diagnose bugs rather early instead of letting them hit later in a situation where it wouldn't be clear what caused them. Adding a WARN() for all of these cases isn't really appropriate, especially as this might impact performance due to added tests, which isn't the case for theoretically unreachable code. > There's a reason we have ASSERT() and friends and, independently, > WARN_ON() / BUG_ON() et al. We might want to introduce something like ASSERT_OR_WARN(). I'm sure this could be useful in some cases. Juergen
On 24.08.2022 12:45, Juergen Gross wrote: > On 24.08.22 12:35, Jan Beulich wrote: >> On 24.08.2022 12:22, Juergen Gross wrote: >>> Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production >>> builds a warning seems to be appropriate when hitting one. >> >> I disagree, for two reasons: This violates the implication of NDEBUG >> meaning ASSERT() and friends expand to no actual code. Plus if doing so > > This is something we can change IMHO. > >> for ASSERT_UNREACHABLE(), why would we not do the same for ASSERT()? > > There are multiple reasons to have ASSERT()s. Some serve as a kind of > documentation (e.g. to document that the programmer thought of a special > case not being possible), or they are meant to catch hard to diagnose > bugs rather early instead of letting them hit later in a situation where > it wouldn't be clear what caused them. Adding a WARN() for all of these > cases isn't really appropriate, especially as this might impact > performance due to added tests, which isn't the case for theoretically > unreachable code. > >> There's a reason we have ASSERT() and friends and, independently, >> WARN_ON() / BUG_ON() et al. > > We might want to introduce something like ASSERT_OR_WARN(). I'm sure > this could be useful in some cases. I'm curious why in such cases it can't just be WARN_ON(). Jan
On 24/08/2022 11:35, Jan Beulich wrote: > On 24.08.2022 12:22, Juergen Gross wrote: >> Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production >> builds a warning seems to be appropriate when hitting one. > I disagree, for two reasons: This violates the implication of NDEBUG > meaning ASSERT() and friends expand to no actual code. I agree. ASSERT() and friends should no code in !DEBUG builds. Furthermore, if an ASSERT_UNREACHALBE() is proving to be problematic even at runtime, then it's not the correct construct in the first place. ~Andrew
On 24.08.22 13:12, Jan Beulich wrote: > On 24.08.2022 12:45, Juergen Gross wrote: >> On 24.08.22 12:35, Jan Beulich wrote: >>> On 24.08.2022 12:22, Juergen Gross wrote: >>>> Hitting an ASSERT_UNREACHABLE() is always wrong, so even in production >>>> builds a warning seems to be appropriate when hitting one. >>> >>> I disagree, for two reasons: This violates the implication of NDEBUG >>> meaning ASSERT() and friends expand to no actual code. Plus if doing so >> >> This is something we can change IMHO. >> >>> for ASSERT_UNREACHABLE(), why would we not do the same for ASSERT()? >> >> There are multiple reasons to have ASSERT()s. Some serve as a kind of >> documentation (e.g. to document that the programmer thought of a special >> case not being possible), or they are meant to catch hard to diagnose >> bugs rather early instead of letting them hit later in a situation where >> it wouldn't be clear what caused them. Adding a WARN() for all of these >> cases isn't really appropriate, especially as this might impact >> performance due to added tests, which isn't the case for theoretically >> unreachable code. >> >>> There's a reason we have ASSERT() and friends and, independently, >>> WARN_ON() / BUG_ON() et al. >> >> We might want to introduce something like ASSERT_OR_WARN(). I'm sure >> this could be useful in some cases. > > I'm curious why in such cases it can't just be WARN_ON(). It won't result in test failure of debug builds. In the end I'm not feeling really strong here, so I'm fine to drop this patch. Juergen
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 05ee1e18af..b8472d753c 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -40,6 +40,16 @@ unlikely(ret_warn_on_); \ }) +#define WARN_ONCE() do { \ + static bool warned = false; \ + \ + if ( !warned ) \ + { \ + warned = true; \ + WARN(); \ + } \ +} while (0) + /* All clang versions supported by Xen have _Static_assert. */ #if defined(__clang__) || \ (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) @@ -63,7 +73,7 @@ #define ASSERT_UNREACHABLE() assert_failed("unreachable") #else #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0) -#define ASSERT_UNREACHABLE() do { } while (0) +#define ASSERT_UNREACHABLE() WARN_ONCE() #endif #define ABS(_x) ({ \