Bug 60125

Summary: 0.9.11 sometimes fails to link due to failure to inline SortedArrayOf<>::search func
Product: HarfBuzz Reporter: gmad <graham>
Component: srcAssignee: Behdad Esfahbod <freedesktop>
Severity: major    
Priority: medium CC: freedesktop
Version: unspecified   
Hardware: x86 (IA32)   
OS: Linux (All)   
i915 platform: i915 features:
Attachments: patch to fix template func inlining failure on some GCC installs

Description gmad 2013-01-31 15:45:33 UTC
Created attachment 74001 [details]
patch to fix template func inlining failure on some GCC installs

There's a minor problem with GCC failing to properly inline one of the templated search functions (resulting in failure to link) which is addressed by the attached patch against 0.9.11 to move the definition of the func outside the class decl.
Comment 1 Behdad Esfahbod 2013-02-05 02:05:52 UTC
Thanks.  I'll try it.  Much appreciated, since this has been an unsolved problem for a while.
Comment 2 Behdad Esfahbod 2013-02-05 03:58:43 UTC
Humm, I had to add back inline.  Without inline I consider the patch wrong.  Please see if it still helps:

diff --git a/src/hb-open-type-private.hh b/src/hb-open-type-private.hh
index 90f2836..5370c67 100644
--- a/src/hb-open-type-private.hh
+++ b/src/hb-open-type-private.hh
@@ -945,9 +945,13 @@ struct HeadlessArrayOf
 /* An array with sorted elements.  Supports binary searching. */
 template <typename Type>
 struct SortedArrayOf : ArrayOf<Type> {
   template <typename SearchType>
-  inline int search (const SearchType &x) const {
+  inline int search (const SearchType &x) const;
+template <typename Type>
+template <typename SearchType>
+inline int SortedArrayOf<Type>::search (const SearchType &x) const {
     unsigned int count = this->len;
     /* Linear search is *much* faster for small counts. */
     if (likely (count < 32)) {
@@ -962,9 +966,7 @@ struct SortedArrayOf : ArrayOf<Type> {
       const Type *p = (const Type *) bsearch (&x, this->array, this->len, sizeof (this->array[0]), (hb_compare_func_t) Cmp::cmp);
       return p ? p - this->array : -1;
-  }
 } /* namespace OT */
Comment 3 gmad 2013-02-06 05:33:54 UTC
Adding inline back in re-broke the patch for me (I again failed to link),
but using __attribute((always_inline)) instead of the weaker 'inline' seems to do the trick.

Happy compromise? ;)
Comment 4 Behdad Esfahbod 2013-02-06 19:26:28 UTC
Meh.  Yeah.  Does it work if we don't move the definition out?  And does it work if we have both inline and the attribute?
Comment 5 Behdad Esfahbod 2013-02-06 19:26:58 UTC
What are the GCC versions that you've observed this on BTW?
Comment 6 gmad 2013-02-09 15:04:11 UTC
Sorry for the delay, but here's what I found out:

- It affects gcc-4.3.4 (which I was using due to reqs of OTHER code),
  it seems to inline correctly on >= gcc-4.5.4

- Just putting the __attribute((always_inline)) on the decl within the template does not fix the problem (you get the same link issue as the plain 'inline' decl)

Probably not a necessary patch for anyone not compiling in the stone age, but maybe a nice work-around to pass along for the desperate.

Comment 7 Behdad Esfahbod 2013-12-04 22:56:42 UTC
No resolution.  Lets close.  Thanks.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.