ability create static library #85

nicolaichuk · 2016-09-26T22:10:56Z

add msvc "/Wall"

stevengj · 2016-09-27T15:04:10Z

utf8proc.h

 #else
-#  define UTF8PROC_DLLEXPORT
+#  if defined(__GNUC__) && __GNUC__ >= 4
+#    define UTF8PROC_DLLEXPORT __attribute__ ((visibility("hidden")))


Why would we use hidden visibility here?

Put simply, it hides most of the ELF symbols which would have previously (and unnecessarily) been public. This means:

It very substantially improves load times of your DSO (Dynamic Shared Object). For example, a huge C++ template-based library which was tested (the TnFOX Boost.Python bindings library) now loads in eight seconds rather than over six minutes!

It lets the optimiser produce better code. PLT indirections (when a function call or variable access must be looked up via the Global Offset Table such as in PIC code) can be completely avoided, thus substantially avoiding pipeline stalls on modern processors and thus much faster code. Furthermore when most of the symbols are bound locally, they can be safely elided (removed) completely through the entire DSO. This gives greater latitude especially to the inliner which no longer needs to keep an entry point around "just in case".

It reduces the size of your DSO by 5-20%. ELF's exported symbol table format is quite a space hog, giving the complete mangled symbol name which with heavy template usage can average around 1000 bytes. C++ templates spew out a huge amount of symbols and a typical C++ library can easily surpass 30,000 symbols which is around 5-6Mb! Therefore if you cut out the 60-80% of unnecessary symbols, your DSO can be megabytes smaller!

Much lower chance of symbol collision. The old woe of two libraries internally using the same symbol for different things is finally behind us with this patch. Hallelujah!

All new code should have this support from the beginning!

read more https://gcc.gnu.org/wiki/Visibility

I understand why you would want to hide symbols that you are not exporting. We are already doing this by declaring internal functions as static (which works because all of our code is in a single .c file). But UTF8PROC_DLLEXPORT is used for exported functions. Surely we don't want to hide those symbols?

Also, this code is for when UTF8PROC_BUILD_AS_LIB is defined, so we are not even building a shared library and the visibility attribute is ignored, no?

stevengj · 2016-09-27T15:05:19Z

utf8proc.h

+#      define UTF8PROC_DLLEXPORT __declspec(dllimport)
+#    endif
+#  elif defined(__GNUC__) && __GNUC__ >= 4
+#    define UTF8PROC_DLLEXPORT __attribute__ ((visibility("default")))


If we are building a static library, it doesn't seem like the visibility attribute is relevant, since that seems to correspond to the ELF ABI for shared objects.

ifNdef UTF8PROC_BUILD_AS_LIB

in this place we are building shared library.

My bad; in that case the comment applies to the visibility("hidden") above.

In any case, this seems a bit superfluous, since the visibility is default by default.

stevengj · 2016-09-27T15:06:15Z

CMakeLists.txt

+  set(_utf8proc_build_type "SHARED")
+else(UTF8PROC_BUILD_AS_SHARED)
+  list(APPEND _utf8proc_additional_defined 
+    "-DUTF8PROC_BUILD_AS_LIB"


BUILD_AS_STATIC seems more appropriate as the platform-independent antonym of "shared"

Or maybe UTF8PROC_STATIC_EXPORTS

stevengj · 2016-09-27T15:07:44Z

CMakeLists.txt

+  set(_utf8proc_is_set_build_shared_libs ${BUILD_SHARED_LIBS})
+endif(DEFINED BUILD_SHARED_LIBS)
+
+set(UTF8PROC_BUILD_AS_SHARED ${_utf8proc_is_set_build_shared_libs} CACHE BOOL "If present and true, this will cause utf8proc to be built shared unless the utf8proc was explicitly added as a static library.")


what does "explicitly added as a static library" mean?

Sorry, my english is not good. Maybe you formulate a better this help phrase.

stevengj · 2016-09-27T15:08:40Z

CMakeLists.txt

-)
+# configure build type
+
+set(_utf8proc_is_set_build_shared_libs TRUE)


This is a weird variable name.

You can change to any other :)

Keno · 2016-09-27T20:19:12Z

utf8proc.h

 #else
-#  define UTF8PROC_DLLEXPORT
+#  if defined(__GNUC__) && __GNUC__ >= 4
+#    define UTF8PROC_DLLEXPORT __attribute__ ((visibility("hidden")))


The whole point of these symbols is that they are the public API of this library. Making them hidden makes little sense. End users can decide whether to ask to linker to hide them or not when they build this into their libraries and applications.

When i build static library - this meen that i want use this library for internal purposes only. And it hide for external user.
When i want external uses - i will build SHARED or MODULE version.

I think that using visibility("default") for all symbols - is legacy gcc practices.

When i build static library - this meen that i want use this library for internal purposes only. And it hide for external user.

It just means that this code is not intended to be used as a shared library (by itself). People would very well decide to use -Wl,--whole-archive and re-export those symbols. Visibility is an API concern and whether or not this library is build shared does not affect what the library's public API is.

Maybe.
If you want you can change this to visibility("default").
It is not principal thing for me.

Yes, just leave that part as-is.

nicolaichuk · 2016-09-27T21:37:07Z

CMakeLists.txt

+  set(_utf8proc_is_set_build_shared_libs ${BUILD_SHARED_LIBS})
+endif(DEFINED BUILD_SHARED_LIBS)
+
+set(UTF8PROC_BUILD_AS_SHARED ${_utf8proc_is_set_build_shared_libs} CACHE BOOL "If present and true, this will cause utf8proc to be built shared unless the utf8proc was explicitly added as a static library.")


Also. If you want. You can add support build as MODULE too.

in this case better rewrite something like this:

if(_utf8proc_is_set_build_shared_libs) set(UTF8PROC_BUILD_TYPE "SHARED" CACHE STRING "<some help message>") else(_utf8proc_is_set_build_shared_libs) set(UTF8PROC_BUILD_TYPE "STATIC" CACHE STRING "<some help message>") endif(_utf8proc_is_set_build_shared_libs) set_property(CACHE UTF8PROC_BUILD_TYPE PROPERTY STRINGS "MODULE;SHARED;STATIC") ..... if("${UTF8PROC_BUILD_TYPE}" STREQUAL "STATIC") list(APPEND _utf8proc_additional_defined "-DUTF8PROC_BUILD_AS_LIB" ) else("${UTF8PROC_BUILD_TYPE}" STREQUAL "STATIC") list(APPEND _utf8proc_additional_defined "-DUTF8PROC_EXPORTS" ) endif("${UTF8PROC_BUILD_TYPE}" STREQUAL "STATIC") .... add_library (utf8proc ${UTF8PROC_BUILD_TYPE} utf8proc.c utf8proc.h )

nicolaichuk · 2016-10-03T23:30:31Z

ping...

stevengj · 2016-10-04T18:45:04Z

You need to update the PR to address the comments above...

erengy · 2016-12-27T05:20:13Z

Just chiming in to say that I'd appreciate an option for static linking. I've been manually editing the header to #define UTF8PROC_DLLEXPORT for some time now, so even a simple ifdef would help:

#ifdef UTF8PROC_STATIC
#  define UTF8PROC_DLLEXPORT
#else
// (current definitions)
#endif

DanielSWolf · 2017-08-01T17:05:30Z

Same here. I need static linking, so I'll have to modify your source code. A simple ifdef would be great, as suggested by @erengy above and by @madscientist in issue #96.

stevengj · 2017-08-01T20:57:48Z

If anyone feels motivated to fix this PR, that would be great.

maxbrunsfeld · 2017-08-09T16:43:32Z

@stevengj I'm not familiar with cmake, but I'd love to get this working at a basic level. Would you merge a PR that just makes @erengy's suggested change to the header?

stevengj · 2018-04-30T01:38:30Z

Closed by #123.

ability create static library

7f42070

add msvc "/Wall"

stevengj reviewed Sep 27, 2016

View reviewed changes

Keno reviewed Sep 27, 2016

View reviewed changes

nicolaichuk commented Sep 27, 2016

View reviewed changes

vtjnash mentioned this pull request Feb 18, 2017

Would like to be able to disable DLLEXPORT #96

Closed

This was referenced Apr 6, 2018

#define UTF8PROC_STATIC to disable DLLEXPORT #122

Closed

Static library support improvements #123

Merged

stevengj closed this Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ability create static library #85

ability create static library #85

nicolaichuk commented Sep 26, 2016

stevengj Sep 27, 2016

nicolaichuk Sep 27, 2016 •

edited

Loading

stevengj Oct 4, 2016

stevengj Oct 4, 2016

stevengj Sep 27, 2016

nicolaichuk Sep 27, 2016

stevengj Oct 4, 2016

stevengj Oct 4, 2016

stevengj Sep 27, 2016

stevengj Sep 27, 2016

stevengj Sep 27, 2016

nicolaichuk Sep 27, 2016 •

edited

Loading

stevengj Sep 27, 2016

nicolaichuk Sep 27, 2016

Keno Sep 27, 2016

nicolaichuk Sep 27, 2016 •

edited

Loading

Keno Sep 27, 2016

nicolaichuk Sep 27, 2016 •

edited

Loading

Keno Sep 27, 2016

nicolaichuk Sep 27, 2016 •

edited

Loading

nicolaichuk commented Oct 3, 2016

stevengj commented Oct 4, 2016

erengy commented Dec 27, 2016

DanielSWolf commented Aug 1, 2017

stevengj commented Aug 1, 2017

maxbrunsfeld commented Aug 9, 2017

stevengj commented Apr 30, 2018

ability create static library #85

ability create static library #85

Conversation

nicolaichuk commented Sep 26, 2016

Choose a reason for hiding this comment

nicolaichuk Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaichuk Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaichuk Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaichuk Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaichuk Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

nicolaichuk commented Oct 3, 2016

stevengj commented Oct 4, 2016

erengy commented Dec 27, 2016

DanielSWolf commented Aug 1, 2017

stevengj commented Aug 1, 2017

maxbrunsfeld commented Aug 9, 2017

stevengj commented Apr 30, 2018

nicolaichuk Sep 27, 2016 •

edited

Loading

nicolaichuk Sep 27, 2016 •

edited

Loading

nicolaichuk Sep 27, 2016 •

edited

Loading

nicolaichuk Sep 27, 2016 •

edited

Loading

nicolaichuk Sep 27, 2016 •

edited

Loading