harfbuzz / harfbuzz

HarfBuzz text shaping engine
http://harfbuzz.github.io/
Other
4.13k stars 628 forks source link

Update src/check-*.sh for harfbuzz-subset.so #767

Closed behdad closed 6 years ago

behdad commented 6 years ago

Some of those checks currently only check libharfbuzz.so; they should be updated to also check harfbuzz-subset, and possibly harfbuzz-icu as well.

For example, harfbuzz-subset has many internal symbols leaking into the binary ABI right now. The tests are not catching that.

ebraminio commented 6 years ago

This updates check-symbols.sh and check-libstdc++.sh but not check-defs.sh yet. I wonder however why hb-subset is detected to have dependency to libstdc++ while having the same CXXFLAGS (or maybe not? 🤔)? (at least on macOS). The "tested" variable should be improved but more and more I am thinking about porting these to python thus being able to run them on MSVC, apparently the equivalent for Windows are: nm: dumpbin.exe some.dll, ldd: dumpbin /dependents some.dll.

diff --git a/src/check-defs.sh b/src/check-defs.sh
index c669783..adb90a7 100755
--- a/src/check-defs.sh
+++ b/src/check-defs.sh
@@ -27,7 +27,7 @@ for def in $defs; do
        so=$libs/lib${lib}.$suffix
        if ! test -f "$so"; then continue; fi

-       # On mac, C symbols are prefixed with _
+       # On macOS, C symbols are prefixed with _
        if test $suffix = dylib; then prefix="_"; fi

        EXPORTED_SYMBOLS="`nm "$so" | grep ' [BCDGINRSTVW] .' | grep -v " $prefix"'\(_fini\>\|_init\>\|_fdata\>\|_ftext\>\|_fbss\>\|__bss_start\>\|__bss_start__\>\|__bss_end__\>\|_edata\>\|_end\>\|_bss_end__\>\|__end__\>\|__gcov_flush\>\|llvm_\)' | cut -d' ' -f3`"
diff --git a/src/check-libstdc++.sh b/src/check-libstdc++.sh
index e4aaeb2..653a566 100755
--- a/src/check-libstdc++.sh
+++ b/src/check-libstdc++.sh
@@ -21,16 +21,18 @@ else
 fi

 tested=false
-for suffix in so dylib; do
-   so=$libs/libharfbuzz.$suffix
-   if ! test -f "$so"; then continue; fi
+for soname in harfbuzz harfbuzz-icu harfbuzz-subset; do
+   for suffix in so dylib; do
+       so=$libs/lib$soname.$suffix
+       if ! test -f "$so"; then continue; fi

-   echo "Checking that we are not linking to libstdc++ or libc++"
-   if $LDD $so | grep 'libstdc[+][+]\|libc[+][+]'; then
-       echo "Ouch, linked to libstdc++ or libc++"
-       stat=1
-   fi
-   tested=true
+       echo "Checking that we are not linking to libstdc++ or libc++"
+       if $LDD $so | grep 'libstdc[+][+]\|libc[+][+]'; then
+           echo "Ouch, linked to libstdc++ or libc++"
+           stat=1
+       fi
+       tested=true
+   done
 done
 if ! $tested; then
    echo "check-libstdc++.sh: libharfbuzz shared library not found; skipping test"
diff --git a/src/check-symbols.sh b/src/check-symbols.sh
index 3adf65f..88e060a 100755
--- a/src/check-symbols.sh
+++ b/src/check-symbols.sh
@@ -17,24 +17,26 @@ fi

 echo "Checking that we are not exposing internal symbols"
 tested=false
-for suffix in so dylib; do
-   so=$libs/libharfbuzz.$suffix
-   if ! test -f "$so"; then continue; fi
+for soname in harfbuzz harfbuzz-icu harfbuzz-subset; do
+   for suffix in so dylib; do
+       so=$libs/lib$soname.$suffix
+       if ! test -f "$so"; then continue; fi

-   EXPORTED_SYMBOLS="`nm "$so" | grep ' [BCDGINRSTVW] .' | grep -v ' _fini\>\| _init\>\| _fdata\>\| _ftext\>\| _fbss\>\| __bss_start\>\| __bss_start__\>\| __bss_end__\>\| _edata\>\| _end\>\| _bss_end__\>\| __end__\>\| __gcov_flush\>\| ___gcov_flush\>\| llvm_\| _llvm_' | cut -d' ' -f3`"
+       EXPORTED_SYMBOLS="`nm "$so" | grep ' [BCDGINRSTVW] .' | grep -v ' _fini\>\| _init\>\| _fdata\>\| _ftext\>\| _fbss\>\| __bss_start\>\| __bss_start__\>\| __bss_end__\>\| _edata\>\| _end\>\| _bss_end__\>\| __end__\>\| __gcov_flush\>\| ___gcov_flush\>\| llvm_\| _llvm_' | cut -d' ' -f3`"

-   prefix=`basename "$so" | sed 's/libharfbuzz/hb/; s/-/_/g; s/[.].*//'`
+       prefix=`basename "$so" | sed 's/libharfbuzz/hb/; s/-/_/g; s/[.].*//'`

-   # On mac, C symbols are prefixed with _
-   if test $suffix = dylib; then prefix="_$prefix"; fi
+       # On macOS, C symbols are prefixed with _
+       if test $suffix = dylib; then prefix="_$prefix"; fi

-   echo "Processing $so"
-   if echo "$EXPORTED_SYMBOLS" | grep -v "^${prefix}_"; then
-       echo "Ouch, internal symbols exposed"
-       stat=1
-   fi
+       echo "Processing $so"
+       if echo "$EXPORTED_SYMBOLS" | grep -v "^${prefix}"; then
+           echo "Ouch, internal symbols exposed"
+           stat=1
+       fi

-   tested=true
+       tested=true
+   done
 done
 if ! $tested; then
    echo "check-symbols.sh: no shared library found; skipping test"
ebraminio commented 6 years ago

The mentioned issue is only available on autotools and not cmake as it is not generating separated shared objects yet but it shows maybe something is wrong with autotools config of hb-subset.

ebraminio commented 6 years ago

Hmm, libharfbuzz uses gcc for linking but libharfbuzz-subset libharfbuzz-icu are using g++, that probably explains the issue with autotools.

libtool: link: gcc -dynamiclib  -o .libs/libharfbuzz.0.dylib  .libs/libharfbuzz_la-hb-blob.o .libs/libharfbuzz_la-hb-buffer-serialize.o .libs/libharfbuzz_la-hb-buffer.o .libs/libharfbuzz_la-hb-common.o .libs/libharfbuzz_la-hb-face.o .libs/libharfbuzz_la-hb-font.o .libs/libharfbuzz_la-hb-ot-tag.o .libs/libharfbuzz_la-hb-set.o .libs/libharfbuzz_la-hb-shape.o .libs/libharfbuzz_la-hb-shape-plan.o .libs/libharfbuzz_la-hb-shaper.o .libs/libharfbuzz_la-hb-unicode.o .libs/libharfbuzz_la-hb-warning.o .libs/libharfbuzz_la-hb-aat-layout.o .libs/libharfbuzz_la-hb-ot-font.o .libs/libharfbuzz_la-hb-ot-layout.o .libs/libharfbuzz_la-hb-ot-map.o .libs/libharfbuzz_la-hb-ot-math.o .libs/libharfbuzz_la-hb-ot-shape.o .libs/libharfbuzz_la-hb-ot-shape-complex-arabic.o .libs/libharfbuzz_la-hb-ot-shape-complex-default.o .libs/libharfbuzz_la-hb-ot-shape-complex-hangul.o .libs/libharfbuzz_la-hb-ot-shape-complex-hebrew.o .libs/libharfbuzz_la-hb-ot-shape-complex-indic.o .libs/libharfbuzz_la-hb-ot-shape-complex-indic-table.o .libs/libharfbuzz_la-hb-ot-shape-complex-khmer.o .libs/libharfbuzz_la-hb-ot-shape-complex-myanmar.o .libs/libharfbuzz_la-hb-ot-shape-complex-thai.o .libs/libharfbuzz_la-hb-ot-shape-complex-tibetan.o .libs/libharfbuzz_la-hb-ot-shape-complex-use.o .libs/libharfbuzz_la-hb-ot-shape-complex-use-table.o .libs/libharfbuzz_la-hb-ot-shape-normalize.o .libs/libharfbuzz_la-hb-ot-shape-fallback.o .libs/libharfbuzz_la-hb-ot-var.o .libs/libharfbuzz_la-hb-fallback-shape.o .libs/libharfbuzz_la-hb-glib.o .libs/libharfbuzz_la-hb-ft.o .libs/libharfbuzz_la-hb-ucdn.o   -Wl,-force_load,hb-ucdn/.libs/libhb-ucdn.a  -lm -L/usr/local/Cellar/glib/2.54.3/lib -L/usr/local/opt/gettext/lib -lglib-2.0 -lintl -L/usr/local/opt/freetype/lib -lfreetype  -g -O2 -Wl,-framework -Wl,CoreFoundation   -install_name  /usr/local/lib/libharfbuzz.0.dylib -compatibility_version 10706 -current_version 10706.0 -Wl,-single_module

libtool: link: g++ -dynamiclib  -o .libs/libharfbuzz-subset.0.dylib  .libs/libharfbuzz_subset_la-hb-subset.o .libs/libharfbuzz_subset_la-hb-subset-glyf.o .libs/libharfbuzz_subset_la-hb-subset-plan.o   ./.libs/libharfbuzz.dylib -L/usr/local/Cellar/glib/2.54.3/lib -L/usr/local/opt/gettext/lib -L/usr/local/opt/freetype/lib -lm -lglib-2.0 -lintl -lfreetype  -g -O2   -install_name  /usr/local/lib/libharfbuzz-subset.0.dylib -compatibility_version 10706 -current_version 10706.0 -Wl,-single_module

libtool: link: g++ -dynamiclib  -o .libs/libharfbuzz-icu.0.dylib  .libs/libharfbuzz_icu_la-hb-icu.o   -L/usr/local/Cellar/icu4c/60.2/lib -licui18n -licuuc -licudata ./.libs/libharfbuzz.dylib -L/usr/local/Cellar/glib/2.54.3/lib -L/usr/local/opt/gettext/lib -L/usr/local/opt/freetype/lib -lm -lglib-2.0 -lintl -lfreetype  -g -O2   -install_name  /usr/local/lib/libharfbuzz-icu.0.dylib -compatibility_version 10706 -current_version 10706.0 -Wl,-single_module
ebraminio commented 6 years ago

Also as a self note, hb-subset module violates this https://github.com/harfbuzz/harfbuzz/blob/master/src/check-symbols.sh#L32 as it provides hb_subset as function but that line expects hbsubset at least:

-   if echo "$EXPORTED_SYMBOLS" | grep -v "^${prefix}_"; then
+   if echo "$EXPORTED_SYMBOLS" | grep -v "^${prefix}"; then