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

[Qu Wenruo]

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 <rgold...@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 <rgold...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html