avoid clobbering registers with J_ASSERT macro
Subsystems (2)
cover avoid clobbering registers with J_ASSERT macro
The J_ASSERT() macro in jbd and jbd2 calls printk() prior to BUG(). While this makes it more convenient to read the assertion failure, it also clobbers registers, which can sometimes make debugging harder, which is clearly not the intended purpose. I recently banged my head on this myself.
The following patches to jbd and jbd2 enable the printk only if CONFIG_JBD[2]_DEBUG is set. Otherwise, it will simply BUG if the condition is violated. This way test kernels still get the benefit of the J_ASSERT printk, while production kernels, which come from a more stable source base where it's easier to trace line numbers back to specific lines of code, simply get the BUG, with all registers preserved.
This is, of course, not the only way of fixing this problem, but it seems to be the least invasive way, which is why I'm proposing these patches.
-- Chris
RFC PATCH 1/2 jbd: avoid clobbering registers with J_ASSERT macro
From: Chris Snook <[email protected]>
Don't printk before BUG in J_ASSERT unless CONFIG_JBD_DEBUG is set.
include/linux/jbd.h +4 -1
@@ -246,7 +246,10 @@ typedef struct journal_superblock_s#include <linux/fs.h> #include <linux/sched.h> +#ifdef CONFIG_JBD_DEBUG #define JBD_ASSERTIONS +#endif +#ifdef JBD_ASSERTIONS #define J_ASSERT (assert) \ do { \
@@ -273,7 +276,7 @@ void buffer_assertion_failure(struct buf#endif #else -#define J_ASSERT (assert) do { } while (0) +#define J_ASSERT (assert) BUG_ON(!(assert))#endif /* JBD_ASSERTIONS */ #if defined(JBD_PARANOID_IOFAIL )
RFC PATCH 2/2 jbd2: avoid clobbering registers with J_ASSERT macro
In reply to Chris SnookFrom: Chris Snook <[email protected]>
Don't printk before BUG in J_ASSERT unless CONFIG_JBD2_DEBUG is set.
include/linux/jbd2.h +4 -1
@@ -255,7 +255,10 @@ typedef struct journal_superblock_s#include <linux/fs.h> #include <linux/sched.h> +#ifdef CONFIG_JBD2_DEBUG #define JBD_ASSERTIONS +#endif +#ifdef JBD_ASSERTIONS #define J_ASSERT (assert) \ do { \
@@ -282,7 +285,7 @@ void buffer_assertion_failure(struct buf#endif #else -#define J_ASSERT (assert) do { } while (0) +#define J_ASSERT (assert) BUG_ON(!(assert))#endif /* JBD_ASSERTIONS */ #if defined(JBD_PARANOID_IOFAIL )
Quoted 12 lines · On Fri, 17 Aug 2007 01:54:18 -0400 Chris Snook <[email protected]> wrote:
The
J_ASSERT()macro in jbd and jbd2 callsprintk()prior toBUG(). While this makes it more convenient to read the assertion failure, it also clobbers registers, which can sometimes make debugging harder, which is clearly not the intended purpose. I recently banged my head on this myself.The following patches to jbd and jbd2 enable the printk only if CONFIG_JBD[2]_DEBUG is set. Otherwise, it will simply BUG if the condition is violated. This way test kernels still get the benefit of the
J_ASSERTprintk, while production kernels, which come from a more stable source base where it's easier to trace line numbers back to specific lines of code, simply get the BUG, with all registers preserved.
Quoted 2 lines
This is, of course, not the only way of fixing this problem, but it seems to be the least invasive way, which is why I'm proposing these patches.
How's about we just remove that printk? Do
#define J_ASSERT(e) BUG_ON(e)?
The rest of the kernel seems to be able to cope with that...
Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Fri, 17 Aug 2007 01:54:18 -0400 Chris Snook <[email protected]> wrote:
The
J_ASSERT()macro in jbd and jbd2 callsprintk()prior toBUG(). While this makes it more convenient to read the assertion failure, it also clobbers registers, which can sometimes make debugging harder, which is clearly not the intended purpose. I recently banged my head on this myself.The following patches to jbd and jbd2 enable the printk only if CONFIG_JBD[2]_DEBUG is set. Otherwise, it will simply BUG if the condition is violated. This way test kernels still get the benefit of the
J_ASSERTprintk, while production kernels, which come from a more stable source base where it's easier to trace line numbers back to specific lines of code, simply get the BUG, with all registers preserved.
Quoted 7 lines
Quoted 1 line
This is, of course, not the only way of fixing this problem, but it seems to be the least invasive way, which is why I'm proposing these patches.
How's about we just remove that printk? Do
#define
J_ASSERT(e)BUG_ON(e)?The rest of the kernel seems to be able to cope with that...
Perfectly fine with me. I proposed the conditional on the grounds that someone probably had a purpose for the original J_ASSERT macro at some point in history, but if that purpose is long since obsolete, let's just get rid of it. I'll repost.
-- Chris
Hi,
Quoted 9 lines · Chris Snook
Quoted 5 lines
How's about we just remove that printk? Do
#define
J_ASSERT(e)BUG_ON(e)?The rest of the kernel seems to be able to cope with that...
Perfectly fine with me. I proposed the conditional on the grounds that someone probably had a purpose for the original
J_ASSERTmacro at some point in history, but if that purpose is long since obsolete, let's just get rid of it. I'll repost.
It did. The original J_ASSERT predates BUG() entirely, and was added so that we got the file/line-no information. But with the current BUG() macro, I can't see any reason for J_ASSERT still to try to gather that information itself.
--Stephen
Quoted 7 lines · Stephen C. Tweedie
Hi,
Quoted 3 lines · Chris Snook
Quoted 3 lines
How's about we just remove that printk? Do
#define
J_ASSERT(e)BUG_ON(e)?
ITYM #define J_ASSERT(e) BUG_ON(!e)
Quoted 4 lines
It did. The original
J_ASSERTpredatesBUG()entirely, and was added so that we got the file/line-no information. But with the currentBUG()macro, I can't see any reason forJ_ASSERTstill to try to gather that information itself.
Do you still want to keep J_ASSERT, or should all uses of it be replaced with BUG_ON?
(to put it another way; if you were writing JBD now, would you add your own J_ASSERT, or would you just use BUG_ON directly?)
Signature · 3 lines
"Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Hi,
Quoted 5 lines · Matthew Wilcox
Do you still want to keep
J_ASSERT, or should all uses of it be replaced withBUG_ON?(to put it another way; if you were writing JBD now, would you add your own
J_ASSERT, or would you just useBUG_ONdirectly?)
The _BH and _JH variants I'd keep, as they allow developers to hook state debugging printks into the output easily. J_ASSERT itself... I wouldn't think I'd bother reinventing BUG_ON these days.
--Stephen