HB_DEBUG is broken, at least in clang build #1923

ebraminio · 2019-08-23T17:22:47Z

Sad that our bots didn't catch it https://circleci.com/gh/harfbuzz/harfbuzz/106776 (probably okay with gcc?), our HB_DEBUG builds are broken and needs this patch which didn't made sense that is why I am not uploading it as a PR.

diff --git a/src/hb-array.hh b/src/hb-array.hh
index d60c2494..9b7aa235 100644
--- a/src/hb-array.hh
+++ b/src/hb-array.hh
@@ -188,12 +188,11 @@ struct hb_array_t : hb_iter_with_fallback_t<hb_array_t<Type>, Type&>
   template <typename hb_serialize_context_t>
   hb_array_t copy (hb_serialize_context_t *c) const
   {
-    TRACE_SERIALIZE (this);
     auto* out = c->start_embed (arrayZ);
-    if (unlikely (!c->extend_size (out, get_size ()))) return_trace (hb_array_t ());
+    if (unlikely (!c->extend_size (out, get_size ()))) return hb_array_t ();
     for (unsigned i = 0; i < length; i++)
       out[i] = arrayZ[i]; /* TODO: add version that calls c->copy() */
-    return_trace (hb_array_t (out, length));
+    return hb_array_t (out, length);
   }
 
   template <typename hb_sanitize_context_t>
diff --git a/src/hb-debug.hh b/src/hb-debug.hh
index e6d06e31..102ad0ea 100644
--- a/src/hb-debug.hh
+++ b/src/hb-debug.hh
@@ -413,7 +413,7 @@ struct hb_no_trace_t {
 #define TRACE_SANITIZE(this) \
        hb_auto_trace_t<HB_DEBUG_SANITIZE, bool> trace \
        (&c->debug_depth, c->get_name (), this, HB_FUNC, \
-        " ");
+        " ")
 #else
 #define TRACE_SANITIZE(this) hb_no_trace_t<bool> trace
 #endif
@@ -425,7 +425,7 @@ struct hb_no_trace_t {
 #define TRACE_SERIALIZE(this) \
        hb_auto_trace_t<HB_DEBUG_SERIALIZE, bool> trace \
        (&c->debug_depth, "SERIALIZE", c, HB_FUNC, \
-        " ");
+        " ")
 #else
 #define TRACE_SERIALIZE(this) hb_no_trace_t<bool> trace
 #endif
@@ -437,7 +437,7 @@ struct hb_no_trace_t {
 #define TRACE_SUBSET(this) \
   hb_auto_trace_t<HB_DEBUG_SUBSET, bool> trace \
   (&c->debug_depth, c->get_name (), this, HB_FUNC, \
-   " ");
+   " ")
 #else
 #define TRACE_SUBSET(this) hb_no_trace_t<bool> trace
 #endif
@@ -454,7 +454,7 @@ struct hb_no_trace_t {
 #define TRACE_DISPATCH(this, format) \
        hb_auto_trace_t<context_t::max_debug_depth, typename context_t::return_t> trace \
        (&c->debug_depth, c->get_name (), this, HB_FUNC, \
-        "format %d", (int) format);
+        "format %d", (int) format)
 #else
 #define TRACE_DISPATCH(this, format) hb_no_trace_t<typename context_t::return_t> trace
 #endif
diff --git a/src/hb-open-type.hh b/src/hb-open-type.hh
index ad995750..9450d416 100644
--- a/src/hb-open-type.hh
+++ b/src/hb-open-type.hh
@@ -450,10 +450,9 @@ struct UnsizedArrayOf
 
   UnsizedArrayOf* copy (hb_serialize_context_t *c, unsigned count) const
   {
-    TRACE_SERIALIZE (this);
     auto *out = c->start_embed (this);
-    if (unlikely (!as_array (count).copy (c))) return_trace (nullptr);
-    return_trace (out);
+    if (unlikely (!as_array (count).copy (c))) return nullptr;
+    return out;
   }
 
   template <typename ...Ts>
diff --git a/src/hb-ot-cmap-table.hh b/src/hb-ot-cmap-table.hh
index 23ea4659..55fdd615 100644
--- a/src/hb-ot-cmap-table.hh
+++ b/src/hb-ot-cmap-table.hh
@@ -120,7 +120,7 @@ struct CmapSubtableFormat4
   {
     HBUINT16 *startCode = c->start_embed<HBUINT16> ();
     hb_codepoint_t prev_cp = 0xFFFF;
-    
+
     + it
     | hb_apply ([&] (const hb_item_type<Iterator> _)
                 {
@@ -157,7 +157,7 @@ struct CmapSubtableFormat4
     unsigned i = 0;
     hb_codepoint_t last_gid = 0, start_gid = 0, last_cp = 0xFFFF;
     bool use_delta = true;
-    
+
     HBINT16 *idDelta = c->start_embed<HBINT16> ();
     if ((char *)idDelta - (char *)startCode != (int) segcount * (int) HBINT16::static_size)
       return nullptr;
@@ -209,7 +209,7 @@ struct CmapSubtableFormat4
     HBUINT16 *idRangeOffset = c->allocate_size<HBUINT16> (HBUINT16::static_size * segcount);
     if (unlikely (!c->check_success (idRangeOffset))) return nullptr;
     if (unlikely ((char *)idRangeOffset - (char *)idDelta != (int) segcount * (int) HBINT16::static_size)) return nullptr;
- 
+
     + hb_range (segcount)
     | hb_filter ([&] (const unsigned _) { return idDelta[_] == 0; })
     | hb_apply ([&] (const unsigned i)
@@ -247,21 +247,21 @@ struct CmapSubtableFormat4
     if (unlikely (!endCode)) return;
 
     unsigned segcount = (c->length () - min_size) / HBUINT16::static_size;
-    
+
     // 2 bytes of padding.
     if (unlikely (!c->allocate_size<HBUINT16> (HBUINT16::static_size))) return; // 2 bytes of padding.
 
    // serialize startCode[]
     HBUINT16 *startCode = serialize_startcode_array (c, it);
     if (unlikely (!startCode)) return;
-    
+
     //serialize idDelta[]
     HBINT16 *idDelta = serialize_idDelta_array (c, it, endCode, startCode, segcount);
     if (unlikely (!idDelta)) return;
 
     HBUINT16 *idRangeOffset = serialize_rangeoffset_glyid (c, it, endCode, startCode, idDelta, segcount);
     if (unlikely (!c->check_success (idRangeOffset))) return;
- 
+
     if (unlikely (!c->check_assign(this->length, c->length () - table_initpos))) return;
     this->segCountX2 = segcount * 2;
     this->entrySelector = hb_max (1u, hb_bit_storage (segcount)) - 1;
@@ -899,9 +899,8 @@ struct EncodingRecord
                         void *base,
                         /* INOUT */ unsigned *objidx) const
   {
-    TRACE_SERIALIZE (this);
     auto *out = c->embed (this);
-    if (unlikely (!out)) return_trace (nullptr);
+    if (unlikely (!out)) return nullptr;
     out->subtable = 0;
 
     if (*objidx == 0)
@@ -914,7 +913,7 @@ struct EncodingRecord
     }
 
     c->add_link (out->subtable, *objidx, base);
-    return_trace (out);
+    return out;
   }
 
   HBUINT16     platformID;     /* Platform ID. */
diff --git a/src/hb-ot-layout-common.hh b/src/hb-ot-layout-common.hh
index 0866760b..12e97d54 100644
--- a/src/hb-ot-layout-common.hh
+++ b/src/hb-ot-layout-common.hh
@@ -231,8 +231,8 @@ struct LangSys
 
   LangSys* copy (hb_serialize_context_t *c) const
   {
-    TRACE_SERIALIZE (this);
-    return_trace (c->embed (*this));
+//     TRACE_SERIALIZE (this);
+    return c->embed (*this);
   }
 
   bool sanitize (hb_sanitize_context_t *c,
@@ -2020,8 +2020,7 @@ struct FeatureVariations
 
   FeatureVariations* copy (hb_serialize_context_t *c) const
   {
-    TRACE_SERIALIZE (this);
-    return_trace (c->embed (*this));
+    return c->embed (*this);
   }
 
   bool sanitize (hb_sanitize_context_t *c) const
diff --git a/src/hb-ot-layout-gsub-table.hh b/src/hb-ot-layout-gsub-table.hh
index 84e1b157..da287666 100644
--- a/src/hb-ot-layout-gsub-table.hh
+++ b/src/hb-ot-layout-gsub-table.hh
@@ -981,11 +981,10 @@ struct LigatureSubst
   template <typename context_t, typename ...Ts>
   typename context_t::return_t dispatch (context_t *c, Ts&&... ds) const
   {
-    TRACE_DISPATCH (this, u.format);
-    if (unlikely (!c->may_dispatch (this, &u.format))) return_trace (c->no_dispatch_return_value ());
+    if (unlikely (!c->may_dispatch (this, &u.format))) return c->no_dispatch_return_value ();
     switch (u.format) {
-    case 1: return_trace (c->dispatch (u.format1, hb_forward<Ts> (ds)...));
-    default:return_trace (c->default_return_value ());
+    case 1: return c->dispatch (u.format1, hb_forward<Ts> (ds)...);
+    default:return c->default_return_value ();
     }
   }
 
diff --git a/src/hb-ot-layout-gsubgpos.hh b/src/hb-ot-layout-gsubgpos.hh
index c6e6ac9c..820a8954 100644
--- a/src/hb-ot-layout-gsubgpos.hh
+++ b/src/hb-ot-layout-gsubgpos.hh
@@ -1417,13 +1417,11 @@ struct RuleSet
   bool apply (hb_ot_apply_context_t *c,
              ContextApplyLookupContext &lookup_context) const
   {
-    TRACE_APPLY (this);
-    return_trace (
+    return
     + hb_iter (rule)
     | hb_map (hb_add (this))
     | hb_map ([&] (const Rule &_) { return _.apply (c, lookup_context); })
     | hb_any
-    )
     ;
   }
 
@@ -2073,13 +2071,11 @@ struct ChainRuleSet
 
   bool apply (hb_ot_apply_context_t *c, ChainContextApplyLookupContext &lookup_context) const
   {
-    TRACE_APPLY (this);
-    return_trace (
+    return
     + hb_iter (rule)
     | hb_map (hb_add (this))
     | hb_map ([&] (const ChainRule &_) { return _.apply (c, lookup_context); })
     | hb_any
-    )
     ;
   }
 
diff --git a/src/hb-ot-name-table.hh b/src/hb-ot-name-table.hh
index 59bf4387..b2543562 100644
--- a/src/hb-ot-name-table.hh
+++ b/src/hb-ot-name-table.hh
@@ -101,11 +101,10 @@ struct NameRecord
                    const void *src_base,
                    const void *dst_base) const
   {
-    TRACE_SERIALIZE (this);
     auto *out = c->embed (this);
-    if (unlikely (!out)) return_trace (nullptr);
+    if (unlikely (!out)) return nullptr;
     out->offset.serialize_copy (c, offset, src_base, dst_base, length);
-    return_trace (out);
+    return out;
   }
 
   bool sanitize (hb_sanitize_context_t *c, const void *base) const

behdad · 2019-08-23T18:47:29Z

Would be more useful to explain how it's broken. :)

behdad · 2019-08-23T18:48:56Z

behdad:src 0 (master)$ make CPPFLAGS=-DHB_DEBUG
make  all-recursive
make[1]: Entering directory '/home/behdad/src/harfbuzz/clangbuild/src'
make[2]: Entering directory '/home/behdad/src/harfbuzz/clangbuild/src'
  CXX      main-main.o
In file included from ../../src/main.cc:27:
In file included from ../../src/hb-static.cc:34:
../../src/hb-ot-layout-common.hh:235:5: fatal error: cannot initialize return object of type 'OT::LangSys *' with an rvalue of type 'bool'
    return_trace (c->embed (*this));
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/hb-debug.hh:349:34: note: expanded from macro 'return_trace'
#define return_trace(RET) return trace.ret (RET, HB_FUNC, __LINE__)
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [Makefile:2549: main-main.o] Error 1
make[2]: Leaving directory '/home/behdad/src/harfbuzz/clangbuild/src'
make[1]: *** [Makefile:2939: all-recursive] Error 1
make[1]: Leaving directory '/home/behdad/src/harfbuzz/clangbuild/src'
make: *** [Makefile:1609: all] Error 2

behdad · 2019-08-23T18:49:38Z

No this is equally broken on gcc. Do we have no bots debugging with CPPFLAGS=-DHB_DEBUG?

behdad · 2019-08-23T18:50:27Z

This is actually catching real bugs.

ebraminio · 2019-08-23T18:51:15Z

We used to have https://circleci.com/gh/harfbuzz/harfbuzz/106776 but 🤦 it sets the flags at the wrong place apparently (used to work however).

ebraminio · 2019-08-23T18:55:54Z

Don't you had that semicolon issues? Probably needs clang?

behdad · 2019-08-23T18:57:21Z

No.

behdad · 2019-08-23T18:57:43Z

I tested with both clang and gcc.

behdad:~ 0$ clang --version
clang version 7.0.1 (Fedora 7.0.1-6.fc29)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
behdad:~ 0$ gcc --version
gcc (GCC) 8.3.1 20190223 (Red Hat 8.3.1-2)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ebraminio · 2019-08-23T19:06:40Z

$ clang -v
clang version 8.0.1 (tags/RELEASE_801/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-pc-linux-gnu/9.1.0
Found candidate GCC installation: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.1.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/9.1.0
Found candidate GCC installation: /usr/lib64/gcc/x86_64-pc-linux-gnu/9.1.0
Selected GCC installation: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.1.0
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64
$ make clean && make -j9 CPPFLAGS="-DHB_DEBUG" CC=clang CXX=clang++
Making clean in src
[...]
./hb-array.hh:191:27: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning
      [-Werror,-Wextra-semi-stmt]
    TRACE_SERIALIZE (this);
                          ^
In file included from main.cc:27:
In file included from ./hb-static.cc:29:
./hb-open-type.hh:79:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:135:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:234:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:348:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:358:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:433:27: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SERIALIZE (this);
                          ^
./hb-open-type.hh:441:27: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SERIALIZE (this);
                          ^
./hb-open-type.hh:453:27: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SERIALIZE (this);
                          ^
./hb-open-type.hh:462:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:473:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
                         ^
./hb-open-type.hh:509:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this
      warning [-Werror,-Wextra-semi-stmt]
    TRACE_SANITIZE (this);
[..]

#1923

behdad closed this as completed in 569426d Aug 23, 2019

behdad added a commit that referenced this issue Aug 23, 2019

[debug] Fix extra semicolon issue

541f3c2

#1923

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HB_DEBUG is broken, at least in clang build #1923

HB_DEBUG is broken, at least in clang build #1923

ebraminio commented Aug 23, 2019 •

edited

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

ebraminio commented Aug 23, 2019 •

edited

ebraminio commented Aug 23, 2019 •

edited

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

ebraminio commented Aug 23, 2019 •

edited

HB_DEBUG is broken, at least in clang build #1923

HB_DEBUG is broken, at least in clang build #1923

Comments

ebraminio commented Aug 23, 2019 • edited

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

ebraminio commented Aug 23, 2019 • edited

ebraminio commented Aug 23, 2019 • edited

behdad commented Aug 23, 2019

behdad commented Aug 23, 2019

ebraminio commented Aug 23, 2019 • edited

ebraminio commented Aug 23, 2019 •

edited

ebraminio commented Aug 23, 2019 •

edited

ebraminio commented Aug 23, 2019 •

edited

ebraminio commented Aug 23, 2019 •

edited