diff mbox

[1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Message ID

20161129162452.28402-1-rgoldwyn@suse.de (mailing list archive)

State

Accepted

Headers

show

From: Goldwyn Rodrigues <rgoldwyn@suse.de>
To: linux-btrfs@vger.kernel.org
Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
Subject: [PATCH 1/2] btrfs-progs: Correct value printed by
	assertions/BUG_ON/WARN_ON
Date: Tue, 29 Nov 2016 10:24:52 -0600
Message-Id: <20161129162452.28402-1-rgoldwyn@suse.de>
Sender: linux-btrfs-owner@vger.kernel.org
Precedence: bulk

Commit Message

Goldwyn Rodrigues Nov. 29, 2016, 4:24 p.m. UTC

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(-)

Comments

David Sterba Nov. 30, 2016, 2:43 p.m. UTC | #1

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Qu Wenruo Dec. 5, 2016, 7:10 a.m. UTC | #2

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) ({                      \
>


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goldwyn Rodrigues Dec. 5, 2016, 11:38 a.m. UTC | #3

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) ({                      \
>>
> 
>

Qu Wenruo Dec. 6, 2016, 2:03 a.m. UTC | #4

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) ({                      \
>>>
>>
>>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goldwyn Rodrigues Dec. 6, 2016, 2:51 a.m. UTC | #5

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.

Qu Wenruo Dec. 6, 2016, 3:08 a.m. UTC | #6

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.
>


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goldwyn Rodrigues Dec. 6, 2016, 12:44 p.m. UTC | #7

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.

Qu Wenruo Dec. 7, 2016, 12:31 a.m. UTC | #8

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


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff mbox

Patch

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) ({                      \