* [PATCH v3] btrfs-progs: Fix disable backtrace assert error
@ 2017-01-18 3:04 Qu Wenruo
2017-01-18 11:38 ` Goldwyn Rodrigues
0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2017-01-18 3:04 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, Goldwyn Rodrigues
Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
notted for backtrace enabled and disabled case.
Such behavior makes us easier to pass value wrong, and in fact it did
cause us to pass wrong condition for ASSERT().
Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
manually, this patch will use ASSERT() to implement the resting
ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
but only one.
Also, move WARN_ON() out of the ifdef branch, as it's completely the
same for both branches.
Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
Sorry for late update, being digging the dev-replace/scrub bug
v2:
Keep ASSERT() outputing meaningful error string, use ASSERT() to
implement BUG_ON() so only the abused BUG_ON() output is affected.
Suggested by David.
v3:
Update commit message, since we use ASSERT() instead of BUG_ON() as
main assert function now.
---
kerncompat.h | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/kerncompat.h b/kerncompat.h
index 19ed3fc0..fe23774e 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -291,18 +291,15 @@ static inline void assert_trace(const char *assertion, const char *filename,
abort();
exit(1);
}
-
-#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)!(c))
-#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
#else
-#define BUG_ON(c) assert(!(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
-#define ASSERT(c) assert(!(c))
-#define BUG() assert(0)
+#define ASSERT(c) assert(c)
#endif
+#define BUG_ON(c) ASSERT(!(c))
+#define BUG() BUG_ON(1)
+#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
+
#define container_of(ptr, type, member) ({ \
const typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type,member) );})
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3] btrfs-progs: Fix disable backtrace assert error
2017-01-18 3:04 [PATCH v3] btrfs-progs: Fix disable backtrace assert error Qu Wenruo
@ 2017-01-18 11:38 ` Goldwyn Rodrigues
2017-01-19 0:27 ` Qu Wenruo
0 siblings, 1 reply; 5+ messages in thread
From: Goldwyn Rodrigues @ 2017-01-18 11:38 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs; +Cc: dsterba
On 01/17/2017 09:04 PM, Qu Wenruo wrote:
> Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
> by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
> parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
> notted for backtrace enabled and disabled case.
>
> Such behavior makes us easier to pass value wrong, and in fact it did
> cause us to pass wrong condition for ASSERT().
>
> Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
> manually, this patch will use ASSERT() to implement the resting
> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
> but only one.
>
> Also, move WARN_ON() out of the ifdef branch, as it's completely the
> same for both branches.
>
> Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
> Sorry for late update, being digging the dev-replace/scrub bug
>
> v2:
> Keep ASSERT() outputing meaningful error string, use ASSERT() to
> implement BUG_ON() so only the abused BUG_ON() output is affected.
> Suggested by David.
> v3:
> Update commit message, since we use ASSERT() instead of BUG_ON() as
> main assert function now.
> ---
> kerncompat.h | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/kerncompat.h b/kerncompat.h
> index 19ed3fc0..fe23774e 100644
> --- a/kerncompat.h
> +++ b/kerncompat.h
> @@ -291,18 +291,15 @@ static inline void assert_trace(const char *assertion, const char *filename,
> abort();
> exit(1);
> }
> -
> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)!(c))
> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
> #else
> -#define BUG_ON(c) assert(!(c))
> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
> -#define ASSERT(c) assert(!(c))
> -#define BUG() assert(0)
> +#define ASSERT(c) assert(c)
> #endif
>
> +#define BUG_ON(c) ASSERT(!(c))
The problem with this is that you are killing the value printed as a
part of the trace for BUG_ON(). The main reason why commit
00e769d04c2c83029d6c71 was written. Please be careful with your notting
especially when the values are being printed.
> +#define BUG() BUG_ON(1)
> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
> +
> #define container_of(ptr, type, member) ({ \
> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
> (type *)( (char *)__mptr - offsetof(type,member) );})
>
--
Goldwyn
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] btrfs-progs: Fix disable backtrace assert error
2017-01-18 11:38 ` Goldwyn Rodrigues
@ 2017-01-19 0:27 ` Qu Wenruo
2017-01-19 3:42 ` Goldwyn Rodrigues
0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2017-01-19 0:27 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: dsterba
At 01/18/2017 07:38 PM, Goldwyn Rodrigues wrote:
>
>
> On 01/17/2017 09:04 PM, Qu Wenruo wrote:
>> Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
>> by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
>> parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
>> notted for backtrace enabled and disabled case.
>>
>> Such behavior makes us easier to pass value wrong, and in fact it did
>> cause us to pass wrong condition for ASSERT().
>>
>> Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
>> manually, this patch will use ASSERT() to implement the resting
>> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
>> but only one.
>>
>> Also, move WARN_ON() out of the ifdef branch, as it's completely the
>> same for both branches.
>>
>> Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>> Sorry for late update, being digging the dev-replace/scrub bug
>>
>> v2:
>> Keep ASSERT() outputing meaningful error string, use ASSERT() to
>> implement BUG_ON() so only the abused BUG_ON() output is affected.
>> Suggested by David.
>> v3:
>> Update commit message, since we use ASSERT() instead of BUG_ON() as
>> main assert function now.
>> ---
>> kerncompat.h | 13 +++++--------
>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/kerncompat.h b/kerncompat.h
>> index 19ed3fc0..fe23774e 100644
>> --- a/kerncompat.h
>> +++ b/kerncompat.h
>> @@ -291,18 +291,15 @@ static inline void assert_trace(const char *assertion, const char *filename,
>> abort();
>> exit(1);
>> }
>> -
>> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
>> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
>> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)!(c))
>> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
>> #else
>> -#define BUG_ON(c) assert(!(c))
>> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
>> -#define ASSERT(c) assert(!(c))
>> -#define BUG() assert(0)
>> +#define ASSERT(c) assert(c)
>> #endif
>>
>> +#define BUG_ON(c) ASSERT(!(c))
>
> The problem with this is that you are killing the value printed as a
> part of the trace for BUG_ON(). The main reason why commit
> 00e769d04c2c83029d6c71 was written. Please be careful with your notting
> especially when the values are being printed.
>
This is designed.
As ASSERT() is more meaningful than the abused BUG_ON(), I changed it to
print correct value for ASSERT() and ignore BUG_ON().
Thanks,
Qu
>
>> +#define BUG() BUG_ON(1)
>> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
>> +
>> #define container_of(ptr, type, member) ({ \
>> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>> (type *)( (char *)__mptr - offsetof(type,member) );})
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] btrfs-progs: Fix disable backtrace assert error
2017-01-19 0:27 ` Qu Wenruo
@ 2017-01-19 3:42 ` Goldwyn Rodrigues
2017-01-19 12:08 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Goldwyn Rodrigues @ 2017-01-19 3:42 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs; +Cc: dsterba
On 01/18/2017 06:27 PM, Qu Wenruo wrote:
>
>
> At 01/18/2017 07:38 PM, Goldwyn Rodrigues wrote:
>>
>>
>> On 01/17/2017 09:04 PM, Qu Wenruo wrote:
>>> Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
>>> by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
>>> parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
>>> notted for backtrace enabled and disabled case.
>>>
>>> Such behavior makes us easier to pass value wrong, and in fact it did
>>> cause us to pass wrong condition for ASSERT().
>>>
>>> Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
>>> manually, this patch will use ASSERT() to implement the resting
>>> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
>>> but only one.
>>>
>>> Also, move WARN_ON() out of the ifdef branch, as it's completely the
>>> same for both branches.
>>>
>>> Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> ---
>>> Sorry for late update, being digging the dev-replace/scrub bug
>>>
>>> v2:
>>> Keep ASSERT() outputing meaningful error string, use ASSERT() to
>>> implement BUG_ON() so only the abused BUG_ON() output is affected.
>>> Suggested by David.
>>> v3:
>>> Update commit message, since we use ASSERT() instead of BUG_ON() as
>>> main assert function now.
>>> ---
>>> kerncompat.h | 13 +++++--------
>>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/kerncompat.h b/kerncompat.h
>>> index 19ed3fc0..fe23774e 100644
>>> --- a/kerncompat.h
>>> +++ b/kerncompat.h
>>> @@ -291,18 +291,15 @@ static inline void assert_trace(const char
>>> *assertion, const char *filename,
>>> abort();
>>> exit(1);
>>> }
>>> -
>>> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__,
>>> (long)(c))
>>> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>> (long)(c))
>>> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,
>>> (long)!(c))
>>> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
>>> #else
>>> -#define BUG_ON(c) assert(!(c))
>>> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>> (long)(c))
>>> -#define ASSERT(c) assert(!(c))
>>> -#define BUG() assert(0)
>>> +#define ASSERT(c) assert(c)
>>> #endif
>>>
>>> +#define BUG_ON(c) ASSERT(!(c))
>>
>> The problem with this is that you are killing the value printed as a
>> part of the trace for BUG_ON(). The main reason why commit
>> 00e769d04c2c83029d6c71 was written. Please be careful with your notting
>> especially when the values are being printed.
>>
>
> This is designed.
>
> As ASSERT() is more meaningful than the abused BUG_ON(), I changed it to
> print correct value for ASSERT() and ignore BUG_ON().
If ASSERT is meaningful, correct it. But please don't make BUG_ON worse
and leave it as it is, at least until the time you can remove the
BUG_ONs or convert it. It should print the correct value of why it did
bug, or else it will print just 1, which is of no debug value.
>
> Thanks,
> Qu
>
>>
>>> +#define BUG() BUG_ON(1)
>>> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>> (long)(c))
>>> +
>>> #define container_of(ptr, type, member) ({ \
>>> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>>> (type *)( (char *)__mptr - offsetof(type,member) );})
>>>
>>
>
>
--
Goldwyn
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] btrfs-progs: Fix disable backtrace assert error
2017-01-19 3:42 ` Goldwyn Rodrigues
@ 2017-01-19 12:08 ` David Sterba
0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2017-01-19 12:08 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: Qu Wenruo, linux-btrfs
On Wed, Jan 18, 2017 at 09:42:45PM -0600, Goldwyn Rodrigues wrote:
> >>> +#define BUG_ON(c) ASSERT(!(c))
> >>
> >> The problem with this is that you are killing the value printed as a
> >> part of the trace for BUG_ON(). The main reason why commit
> >> 00e769d04c2c83029d6c71 was written. Please be careful with your notting
> >> especially when the values are being printed.
> >>
> >
> > This is designed.
> >
> > As ASSERT() is more meaningful than the abused BUG_ON(), I changed it to
> > print correct value for ASSERT() and ignore BUG_ON().
>
> If ASSERT is meaningful, correct it. But please don't make BUG_ON worse
> and leave it as it is, at least until the time you can remove the
> BUG_ONs or convert it. It should print the correct value of why it did
> bug, or else it will print just 1, which is of no debug value.
Agreed, we want to see the exact value.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-19 12:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-18 3:04 [PATCH v3] btrfs-progs: Fix disable backtrace assert error Qu Wenruo
2017-01-18 11:38 ` Goldwyn Rodrigues
2017-01-19 0:27 ` Qu Wenruo
2017-01-19 3:42 ` Goldwyn Rodrigues
2017-01-19 12:08 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).