* [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
@ 2016-11-29 16:24 Goldwyn Rodrigues
2016-11-30 14:43 ` David Sterba
2016-12-05 7:10 ` Qu Wenruo
0 siblings, 2 replies; 9+ messages in thread
From: Goldwyn Rodrigues @ 2016-11-29 16:24 UTC (permalink / raw)
To: linux-btrfs; +Cc: Goldwyn Rodrigues
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
results in printing the value zero for each bug/warning. For example:
volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
This is not useful. Instead changed to print the value of the parameter
passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
to accomodate pointers being passed.
Also, consolidated assert() and BUG() into ifndef.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
kerncompat.h | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
diff --git a/kerncompat.h b/kerncompat.h
index ed9a042..9bd25bd 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -88,39 +88,36 @@ static inline void print_trace(void)
}
static inline void assert_trace(const char *assertion, const char *filename,
- const char *func, unsigned line, int val)
+ const char *func, unsigned line, long val)
{
- if (val)
+ if (!val)
return;
if (assertion)
- fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
+ fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
filename, line, func, assertion, val);
else
- fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
+ fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
filename, line, func, val);
print_trace();
abort();
exit(1);
}
-#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
-#else
-#define BUG() assert(0)
#endif
static inline void warning_trace(const char *assertion, const char *filename,
- const char *func, unsigned line, int val,
+ const char *func, unsigned line, long val,
int trace)
{
- if (val)
+ if (!val)
return;
if (assertion)
fprintf(stderr,
- "%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
+ "%s:%d: %s: Warning: assertion `%s` failed, value %ld\n",
filename, line, func, assertion, val);
else
fprintf(stderr,
- "%s:%d: %s: Warning: assertion failed, value %d.\n",
+ "%s:%d: %s: Warning: assertion failed, value %ld.\n",
filename, line, func, val);
#ifndef BTRFS_DISABLE_BACKTRACE
if (trace)
@@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
#define vfree(x) free(x)
#ifndef BTRFS_DISABLE_BACKTRACE
-#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, !(c), 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), 1)
+#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__, !(c), 0)
-#endif
-
-#ifndef BTRFS_DISABLE_BACKTRACE
-#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (c))
-#else
-#define ASSERT(c) assert(c)
+#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c), 0)
+#define ASSERT(c) assert(!(c))
+#define BUG() assert(0)
#endif
#define container_of(ptr, type, member) ({ \
--
2.10.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-11-29 16:24 [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON Goldwyn Rodrigues
@ 2016-11-30 14:43 ` David Sterba
2016-12-05 7:10 ` Qu Wenruo
1 sibling, 0 replies; 9+ messages in thread
From: David Sterba @ 2016-11-30 14:43 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
On Tue, Nov 29, 2016 at 10:24:52AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
> results in printing the value zero for each bug/warning. For example:
> volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
>
> This is not useful. Instead changed to print the value of the parameter
> passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
> to accomodate pointers being passed.
>
> Also, consolidated assert() and BUG() into ifndef.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-11-29 16:24 [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON Goldwyn Rodrigues
2016-11-30 14:43 ` David Sterba
@ 2016-12-05 7:10 ` Qu Wenruo
2016-12-05 11:38 ` Goldwyn Rodrigues
1 sibling, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2016-12-05 7:10 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
Hi, Goldwyn and David,
This patch seems to cause btrfs test case 023 to fail.
Bisect leads me to this patch.
$ ./btrfs check ~/quota_balance_loop_backref.raw.restored
Checking filesystem on /home/adam/quota_balance_loop_backref.raw.restored
UUID: c33c5ce3-3ad9-4320-9201-c337c04e0051
checking extents
btrfs: cmds-check.c:12284: build_roots_info_cache: Assertion `!(ret ==
0)' failed.
Aborted (core dumped)
And gdb backref:
#0 0x00007ffff6fd204f in raise () from /usr/lib/libc.so.6
#1 0x00007ffff6fd347a in abort () from /usr/lib/libc.so.6
#2 0x00007ffff6fcaea7 in __assert_fail_base () from /usr/lib/libc.so.6
#3 0x00007ffff6fcaf52 in __assert_fail () from /usr/lib/libc.so.6
#4 0x0000000000440426 in build_roots_info_cache (info=0x6f43c0) at
cmds-check.c:12284
#5 0x0000000000440945 in repair_root_items (info=0x6f43c0) at
cmds-check.c:12412
#6 0x00000000004418c3 in cmd_check (argc=2, argv=0x7fffffffe100) at
cmds-check.c:12892
#7 0x000000000040a74c in main (argc=2, argv=0x7fffffffe100) at btrfs.c:301
For frame 4:
(gdb) frame 4
#4 0x0000000000440426 in build_roots_info_cache (info=0x6f43c0) at
cmds-check.c:12284
12284 ASSERT(ret == 0);
(gdb) list
12279 rii->cache_extent.start = root_id;
12280 rii->cache_extent.size = 1;
12281 rii->level = (u8)-1;
12282 entry = &rii->cache_extent;
12283 ret = insert_cache_extent(roots_info_cache, entry);
12284 ASSERT(ret == 0);
12285 } else {
12286 rii = container_of(entry, struct root_item_info,
12287 cache_extent);
12288 }
(gdb) print ret
$1 = 0
For me, ASSERT(ret == 0) seems quite safe and common here.
Doesn't the patch changed the ASSERT() behavior?
Thanks,
Qu
At 11/30/2016 12:24 AM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
> results in printing the value zero for each bug/warning. For example:
> volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
>
> This is not useful. Instead changed to print the value of the parameter
> passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
> to accomodate pointers being passed.
>
> Also, consolidated assert() and BUG() into ifndef.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> kerncompat.h | 35 +++++++++++++++--------------------
> 1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/kerncompat.h b/kerncompat.h
> index ed9a042..9bd25bd 100644
> --- a/kerncompat.h
> +++ b/kerncompat.h
> @@ -88,39 +88,36 @@ static inline void print_trace(void)
> }
>
> static inline void assert_trace(const char *assertion, const char *filename,
> - const char *func, unsigned line, int val)
> + const char *func, unsigned line, long val)
> {
> - if (val)
> + if (!val)
> return;
> if (assertion)
> - fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
> + fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
> filename, line, func, assertion, val);
> else
> - fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
> + fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
> filename, line, func, val);
> print_trace();
> abort();
> exit(1);
> }
>
> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
> -#else
> -#define BUG() assert(0)
> #endif
>
> static inline void warning_trace(const char *assertion, const char *filename,
> - const char *func, unsigned line, int val,
> + const char *func, unsigned line, long val,
> int trace)
> {
> - if (val)
> + if (!val)
> return;
> if (assertion)
> fprintf(stderr,
> - "%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
> + "%s:%d: %s: Warning: assertion `%s` failed, value %ld\n",
> filename, line, func, assertion, val);
> else
> fprintf(stderr,
> - "%s:%d: %s: Warning: assertion failed, value %d.\n",
> + "%s:%d: %s: Warning: assertion failed, value %ld.\n",
> filename, line, func, val);
> #ifndef BTRFS_DISABLE_BACKTRACE
> if (trace)
> @@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
> #define vfree(x) free(x)
>
> #ifndef BTRFS_DISABLE_BACKTRACE
> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, !(c), 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), 1)
> +#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__, !(c), 0)
> -#endif
> -
> -#ifndef BTRFS_DISABLE_BACKTRACE
> -#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (c))
> -#else
> -#define ASSERT(c) assert(c)
> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c), 0)
> +#define ASSERT(c) assert(!(c))
> +#define BUG() assert(0)
> #endif
>
> #define container_of(ptr, type, member) ({ \
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-12-05 7:10 ` Qu Wenruo
@ 2016-12-05 11:38 ` Goldwyn Rodrigues
2016-12-06 2:03 ` Qu Wenruo
0 siblings, 1 reply; 9+ messages in thread
From: Goldwyn Rodrigues @ 2016-12-05 11:38 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
Hi Qu,
Yes, the assert for ifdef BTRFS_DIABLE_BACKTRACE is not correct. The
condition should not have a not(!).
Thanks for reporting.
On 12/05/2016 01:10 AM, Qu Wenruo wrote:
> Hi, Goldwyn and David,
>
> This patch seems to cause btrfs test case 023 to fail.
>
> Bisect leads me to this patch.
>
>
> $ ./btrfs check ~/quota_balance_loop_backref.raw.restored
> Checking filesystem on /home/adam/quota_balance_loop_backref.raw.restored
> UUID: c33c5ce3-3ad9-4320-9201-c337c04e0051
> checking extents
> btrfs: cmds-check.c:12284: build_roots_info_cache: Assertion `!(ret ==
> 0)' failed.
> Aborted (core dumped)
>
>
> And gdb backref:
> #0 0x00007ffff6fd204f in raise () from /usr/lib/libc.so.6
> #1 0x00007ffff6fd347a in abort () from /usr/lib/libc.so.6
> #2 0x00007ffff6fcaea7 in __assert_fail_base () from /usr/lib/libc.so.6
> #3 0x00007ffff6fcaf52 in __assert_fail () from /usr/lib/libc.so.6
> #4 0x0000000000440426 in build_roots_info_cache (info=0x6f43c0) at
> cmds-check.c:12284
> #5 0x0000000000440945 in repair_root_items (info=0x6f43c0) at
> cmds-check.c:12412
> #6 0x00000000004418c3 in cmd_check (argc=2, argv=0x7fffffffe100) at
> cmds-check.c:12892
> #7 0x000000000040a74c in main (argc=2, argv=0x7fffffffe100) at btrfs.c:301
>
>
> For frame 4:
> (gdb) frame 4
> #4 0x0000000000440426 in build_roots_info_cache (info=0x6f43c0) at
> cmds-check.c:12284
> 12284 ASSERT(ret == 0);
> (gdb) list
> 12279 rii->cache_extent.start = root_id;
> 12280 rii->cache_extent.size = 1;
> 12281 rii->level = (u8)-1;
> 12282 entry = &rii->cache_extent;
> 12283 ret = insert_cache_extent(roots_info_cache, entry);
> 12284 ASSERT(ret == 0);
> 12285 } else {
> 12286 rii = container_of(entry, struct root_item_info,
> 12287 cache_extent);
> 12288 }
> (gdb) print ret
> $1 = 0
>
> For me, ASSERT(ret == 0) seems quite safe and common here.
> Doesn't the patch changed the ASSERT() behavior?
>
> Thanks,
> Qu
>
> At 11/30/2016 12:24 AM, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
>> results in printing the value zero for each bug/warning. For example:
>> volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
>>
>> This is not useful. Instead changed to print the value of the parameter
>> passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
>> to accomodate pointers being passed.
>>
>> Also, consolidated assert() and BUG() into ifndef.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>> kerncompat.h | 35 +++++++++++++++--------------------
>> 1 file changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/kerncompat.h b/kerncompat.h
>> index ed9a042..9bd25bd 100644
>> --- a/kerncompat.h
>> +++ b/kerncompat.h
>> @@ -88,39 +88,36 @@ static inline void print_trace(void)
>> }
>>
>> static inline void assert_trace(const char *assertion, const char
>> *filename,
>> - const char *func, unsigned line, int val)
>> + const char *func, unsigned line, long val)
>> {
>> - if (val)
>> + if (!val)
>> return;
>> if (assertion)
>> - fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
>> + fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
>> filename, line, func, assertion, val);
>> else
>> - fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
>> + fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
>> filename, line, func, val);
>> print_trace();
>> abort();
>> exit(1);
>> }
>>
>> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
>> -#else
>> -#define BUG() assert(0)
>> #endif
>>
>> static inline void warning_trace(const char *assertion, const char
>> *filename,
>> - const char *func, unsigned line, int val,
>> + const char *func, unsigned line, long val,
>> int trace)
>> {
>> - if (val)
>> + if (!val)
>> return;
>> if (assertion)
>> fprintf(stderr,
>> - "%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
>> + "%s:%d: %s: Warning: assertion `%s` failed, value %ld\n",
>> filename, line, func, assertion, val);
>> else
>> fprintf(stderr,
>> - "%s:%d: %s: Warning: assertion failed, value %d.\n",
>> + "%s:%d: %s: Warning: assertion failed, value %ld.\n",
>> filename, line, func, val);
>> #ifndef BTRFS_DISABLE_BACKTRACE
>> if (trace)
>> @@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
>> #define vfree(x) free(x)
>>
>> #ifndef BTRFS_DISABLE_BACKTRACE
>> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
>> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>> !(c), 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), 1)
>> +#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__,
>> !(c), 0)
>> -#endif
>> -
>> -#ifndef BTRFS_DISABLE_BACKTRACE
>> -#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (c))
>> -#else
>> -#define ASSERT(c) assert(c)
>> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>> (long)(c), 0)
>> +#define ASSERT(c) assert(!(c))
This should be assert(c), without the not(!)
>> +#define BUG() assert(0)
>> #endif
>>
>> #define container_of(ptr, type, member) ({ \
>>
>
>
--
Goldwyn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-12-05 11:38 ` Goldwyn Rodrigues
@ 2016-12-06 2:03 ` Qu Wenruo
2016-12-06 2:51 ` Goldwyn Rodrigues
0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2016-12-06 2:03 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
BTW, the DISABLE_BACKTRACE branch seems quite different from backtrace one.
#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)
Condition of BUG_ON/ASSERT/BUG are all logical notted for DISABLE_BACKTRACE.
While WARN_ON() of both branch are the same condition.
This seems quite confusing to me.
Any idea to make it more straightforward?
Thanks,
Qu
At 12/05/2016 07:38 PM, Goldwyn Rodrigues wrote:
> Hi Qu,
>
> Yes, the assert for ifdef BTRFS_DIABLE_BACKTRACE is not correct. The
> condition should not have a not(!).
>
> Thanks for reporting.
>
> On 12/05/2016 01:10 AM, Qu Wenruo wrote:
>> Hi, Goldwyn and David,
>>
>> This patch seems to cause btrfs test case 023 to fail.
>>
>> Bisect leads me to this patch.
>>
>>
>> $ ./btrfs check ~/quota_balance_loop_backref.raw.restored
>> Checking filesystem on /home/adam/quota_balance_loop_backref.raw.restored
>> UUID: c33c5ce3-3ad9-4320-9201-c337c04e0051
>> checking extents
>> btrfs: cmds-check.c:12284: build_roots_info_cache: Assertion `!(ret ==
>> 0)' failed.
>> Aborted (core dumped)
>>
>>
>> And gdb backref:
>> #0 0x00007ffff6fd204f in raise () from /usr/lib/libc.so.6
>> #1 0x00007ffff6fd347a in abort () from /usr/lib/libc.so.6
>> #2 0x00007ffff6fcaea7 in __assert_fail_base () from /usr/lib/libc.so.6
>> #3 0x00007ffff6fcaf52 in __assert_fail () from /usr/lib/libc.so.6
>> #4 0x0000000000440426 in build_roots_info_cache (info=0x6f43c0) at
>> cmds-check.c:12284
>> #5 0x0000000000440945 in repair_root_items (info=0x6f43c0) at
>> cmds-check.c:12412
>> #6 0x00000000004418c3 in cmd_check (argc=2, argv=0x7fffffffe100) at
>> cmds-check.c:12892
>> #7 0x000000000040a74c in main (argc=2, argv=0x7fffffffe100) at btrfs.c:301
>>
>>
>> For frame 4:
>> (gdb) frame 4
>> #4 0x0000000000440426 in build_roots_info_cache (info=0x6f43c0) at
>> cmds-check.c:12284
>> 12284 ASSERT(ret == 0);
>> (gdb) list
>> 12279 rii->cache_extent.start = root_id;
>> 12280 rii->cache_extent.size = 1;
>> 12281 rii->level = (u8)-1;
>> 12282 entry = &rii->cache_extent;
>> 12283 ret = insert_cache_extent(roots_info_cache, entry);
>> 12284 ASSERT(ret == 0);
>> 12285 } else {
>> 12286 rii = container_of(entry, struct root_item_info,
>> 12287 cache_extent);
>> 12288 }
>> (gdb) print ret
>> $1 = 0
>>
>> For me, ASSERT(ret == 0) seems quite safe and common here.
>> Doesn't the patch changed the ASSERT() behavior?
>>
>> Thanks,
>> Qu
>>
>> At 11/30/2016 12:24 AM, Goldwyn Rodrigues wrote:
>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>
>>> The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
>>> results in printing the value zero for each bug/warning. For example:
>>> volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
>>>
>>> This is not useful. Instead changed to print the value of the parameter
>>> passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
>>> to accomodate pointers being passed.
>>>
>>> Also, consolidated assert() and BUG() into ifndef.
>>>
>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>> ---
>>> kerncompat.h | 35 +++++++++++++++--------------------
>>> 1 file changed, 15 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/kerncompat.h b/kerncompat.h
>>> index ed9a042..9bd25bd 100644
>>> --- a/kerncompat.h
>>> +++ b/kerncompat.h
>>> @@ -88,39 +88,36 @@ static inline void print_trace(void)
>>> }
>>>
>>> static inline void assert_trace(const char *assertion, const char
>>> *filename,
>>> - const char *func, unsigned line, int val)
>>> + const char *func, unsigned line, long val)
>>> {
>>> - if (val)
>>> + if (!val)
>>> return;
>>> if (assertion)
>>> - fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
>>> + fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
>>> filename, line, func, assertion, val);
>>> else
>>> - fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
>>> + fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
>>> filename, line, func, val);
>>> print_trace();
>>> abort();
>>> exit(1);
>>> }
>>>
>>> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
>>> -#else
>>> -#define BUG() assert(0)
>>> #endif
>>>
>>> static inline void warning_trace(const char *assertion, const char
>>> *filename,
>>> - const char *func, unsigned line, int val,
>>> + const char *func, unsigned line, long val,
>>> int trace)
>>> {
>>> - if (val)
>>> + if (!val)
>>> return;
>>> if (assertion)
>>> fprintf(stderr,
>>> - "%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
>>> + "%s:%d: %s: Warning: assertion `%s` failed, value %ld\n",
>>> filename, line, func, assertion, val);
>>> else
>>> fprintf(stderr,
>>> - "%s:%d: %s: Warning: assertion failed, value %d.\n",
>>> + "%s:%d: %s: Warning: assertion failed, value %ld.\n",
>>> filename, line, func, val);
>>> #ifndef BTRFS_DISABLE_BACKTRACE
>>> if (trace)
>>> @@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
>>> #define vfree(x) free(x)
>>>
>>> #ifndef BTRFS_DISABLE_BACKTRACE
>>> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
>>> -#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>> !(c), 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), 1)
>>> +#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__,
>>> !(c), 0)
>>> -#endif
>>> -
>>> -#ifndef BTRFS_DISABLE_BACKTRACE
>>> -#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (c))
>>> -#else
>>> -#define ASSERT(c) assert(c)
>>> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__,
>>> (long)(c), 0)
>>> +#define ASSERT(c) assert(!(c))
>
> This should be assert(c), without the not(!)
>
>>> +#define BUG() assert(0)
>>> #endif
>>>
>>> #define container_of(ptr, type, member) ({ \
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-12-06 2:03 ` Qu Wenruo
@ 2016-12-06 2:51 ` Goldwyn Rodrigues
2016-12-06 3:08 ` Qu Wenruo
0 siblings, 1 reply; 9+ messages in thread
From: Goldwyn Rodrigues @ 2016-12-06 2:51 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
On 12/05/2016 08:03 PM, Qu Wenruo wrote:
> BTW, the DISABLE_BACKTRACE branch seems quite different from backtrace one.
>
> #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)
>
> Condition of BUG_ON/ASSERT/BUG are all logical notted for
> DISABLE_BACKTRACE.
> While WARN_ON() of both branch are the same condition.
WARN_ON is using warning_trace as opposed to assert, and that is the
reason it is not notted.
>
> This seems quite confusing to me.
>
> Any idea to make it more straightforward?
>
I just kept it the same as before. warning_trace was using an extra
variable, trace, which was not needed because the print_trace was
already in ifndefs.
If you are talking about keeping WARN_ON outside of ifndef, yes, that
will reduce the code further by another line.
--
Goldwyn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-12-06 2:51 ` Goldwyn Rodrigues
@ 2016-12-06 3:08 ` Qu Wenruo
2016-12-06 12:44 ` Goldwyn Rodrigues
0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2016-12-06 3:08 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:
>
>
> On 12/05/2016 08:03 PM, Qu Wenruo wrote:
>> BTW, the DISABLE_BACKTRACE branch seems quite different from backtrace one.
>>
>> #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)
>>
>> Condition of BUG_ON/ASSERT/BUG are all logical notted for
>> DISABLE_BACKTRACE.
>> While WARN_ON() of both branch are the same condition.
>
> WARN_ON is using warning_trace as opposed to assert, and that is the
> reason it is not notted.
>
>>
>> This seems quite confusing to me.
>>
>> Any idea to make it more straightforward?
>>
>
> I just kept it the same as before. warning_trace was using an extra
> variable, trace, which was not needed because the print_trace was
> already in ifndefs.
I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
So that we don't need to manually logical not the condition.
For example:
#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）(c))
and
#define ASSERT(c) assert((c))
This looks much more straightforward, and easier to expose bug at review
time.
Thanks,
Qu
>
> If you are talking about keeping WARN_ON outside of ifndef, yes, that
> will reduce the code further by another line.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-12-06 3:08 ` Qu Wenruo
@ 2016-12-06 12:44 ` Goldwyn Rodrigues
2016-12-07 0:31 ` Qu Wenruo
0 siblings, 1 reply; 9+ messages in thread
From: Goldwyn Rodrigues @ 2016-12-06 12:44 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
On 12/05/2016 09:08 PM, Qu Wenruo wrote:
>
>
> At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:
>>
>>
>> On 12/05/2016 08:03 PM, Qu Wenruo wrote:
>>> BTW, the DISABLE_BACKTRACE branch seems quite different from
>>> backtrace one.
>>>
>>> #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)
>>>
>>> Condition of BUG_ON/ASSERT/BUG are all logical notted for
>>> DISABLE_BACKTRACE.
>>> While WARN_ON() of both branch are the same condition.
>>
>> WARN_ON is using warning_trace as opposed to assert, and that is the
>> reason it is not notted.
>>
>>>
>>> This seems quite confusing to me.
>>>
>>> Any idea to make it more straightforward?
>>>
>>
>> I just kept it the same as before. warning_trace was using an extra
>> variable, trace, which was not needed because the print_trace was
>> already in ifndefs.
>
> I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
> So that we don't need to manually logical not the condition.
First of all, ASSERT and BUG_ON have opposite meanings. ASSERT() checks
if the condition is true and continues (halts if false). BUG_ON() "bugs"
if condition is true and halts (continues if false). So you would have
to use opposite conditions.
>
> For example:
> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）
> (c))
> and
> #define ASSERT(c) assert((c))
>
> This looks much more straightforward, and easier to expose bug at review
> time.
Could you explain with a patch? You idea seems to add more code than
reduce it.
--
Goldwyn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON
2016-12-06 12:44 ` Goldwyn Rodrigues
@ 2016-12-07 0:31 ` Qu Wenruo
0 siblings, 0 replies; 9+ messages in thread
From: Qu Wenruo @ 2016-12-07 0:31 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs, David Sterba; +Cc: Goldwyn Rodrigues
At 12/06/2016 08:44 PM, Goldwyn Rodrigues wrote:
>
>
> On 12/05/2016 09:08 PM, Qu Wenruo wrote:
>>
>>
>> At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:
>>>
>>>
>>> On 12/05/2016 08:03 PM, Qu Wenruo wrote:
>>>> BTW, the DISABLE_BACKTRACE branch seems quite different from
>>>> backtrace one.
>>>>
>>>> #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)
>>>>
>>>> Condition of BUG_ON/ASSERT/BUG are all logical notted for
>>>> DISABLE_BACKTRACE.
>>>> While WARN_ON() of both branch are the same condition.
>>>
>>> WARN_ON is using warning_trace as opposed to assert, and that is the
>>> reason it is not notted.
>>>
>>>>
>>>> This seems quite confusing to me.
>>>>
>>>> Any idea to make it more straightforward?
>>>>
>>>
>>> I just kept it the same as before. warning_trace was using an extra
>>> variable, trace, which was not needed because the print_trace was
>>> already in ifndefs.
>>
>> I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
>> So that we don't need to manually logical not the condition.
>
> First of all, ASSERT and BUG_ON have opposite meanings. ASSERT() checks
> if the condition is true and continues (halts if false). BUG_ON() "bugs"
> if condition is true and halts (continues if false). So you would have
> to use opposite conditions.
I know, I mean, for both backtrace disabled and enabled case, the
condition should be the same.
If not the same condition, it means assert_trace() has different meaning
than original assert.
>
>>
>> For example:
>> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）
>> (c))
>> and
>> #define ASSERT(c) assert((c))
>>
>> This looks much more straightforward, and easier to expose bug at review
>> time.
>
> Could you explain with a patch? You idea seems to add more code than
> reduce it.
Sure, will submit one soon.
Thanks,
Qu
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-12-07 0:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-29 16:24 [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON Goldwyn Rodrigues
2016-11-30 14:43 ` David Sterba
2016-12-05 7:10 ` Qu Wenruo
2016-12-05 11:38 ` Goldwyn Rodrigues
2016-12-06 2:03 ` Qu Wenruo
2016-12-06 2:51 ` Goldwyn Rodrigues
2016-12-06 3:08 ` Qu Wenruo
2016-12-06 12:44 ` Goldwyn Rodrigues
2016-12-07 0:31 ` Qu Wenruo
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).