Add visibility symbols #134

Tachi107 · 2022-03-22T18:50:22Z

They are required to properly build DLLs on Windows, and improve the quality of shared objects on Linux. See https://gcc.gnu.org/wiki/Visibility for details.

This issue was first discovered here: mesonbuild/wrapdb#340 (comment)

CC @stephanlachnit and @eli-schwartz

eli-schwartz · 2022-03-22T19:03:57Z

meson.build


+if get_option('default_library') != 'static'
+    extra_args += '-DINI_SHARED_LIB'
+endif


This is only necessary on Windows, and I think only with MSVC?, and in that case you need a bit more complicated of a check:

if default_library is shared, pass that arg

if default_library is both, error out with the following message: error('default_library=both is not supported with MSVC')

if default_library is static, skip

The problem here is that with default_library=both you build both static and shared, but there is no option to define different defines for each. And my understanding is MSVC is so... interestingly designed... that it downright breaks your library if you add a sharedlib declspec when building statically. This is not a problem with GCC or clang.

This is only necessary on Windows, and I think only with MSVC?

This is necessary on Windows, but it's nice to have on other platforms too, as it makes .so files cleaner and lighter.

Also, I'm not sure that erroring out is necessary, as you can actually build both libraries without issues even on Windows.

I check for != 'static' instead of == 'shared' because while you can compile the static library when -DINI_SHARED_LIB is defined, you can't compile the shared one when the macro is absent.

Edit: and also because you can't consume the shared library when INI_SHARED_LIB is not defined

Edit2: so, as a BothLibraries object in Meson behaves like a SharedLibrary when used, defining INI_SHARED_LIB when the lib type is both or shared seems reasonable to me

This is necessary on Windows, but it's nice to have on other platforms too, as it makes .so files cleaner and lighter.

No, I mean the -D define is unnecessary on other platforms, because on __GNUC__ (gcc/clang) you just unconditionally --without checking for INI_SHARED_LIB -- define an __attribute__ ((visibility ("default"))).

And thus, even without the define, your .so files are always cleaner and lighter. Because sane compiler.

Edit2: so, as a BothLibraries object in Meson behaves like a SharedLibrary when used, defining INI_SHARED_LIB when the lib type is both or shared seems reasonable to me.

The issue is that it will also install a StaticLibrary, and then attempting to use that from other build systems which are configured to search for and prefer static libraries, may produce broken results.

(I am no expert on Windows or MSVC. But I've been told that it "does" produce broken results -- you end up building both a dll and a static library, and the dll works, and the static library doesn't.)

Given that the default is shared, and defining both is something you'd need to opt into, the default behavior is fine anyway.

No, I mean the -D define is unnecessary on other platforms, because on __GNUC__ (gcc/clang) you just unconditionally --without checking for INI_SHARED_LIB -- define an __attribute__ ((visibility ("default"))).

Oh, you're right, I copied the various #ifs from another project of mine and didn't noticed that this was the case 😅️

The issue is that it will also install a StaticLibrary, and then attempting to use that from other build systems which are configured to search for and prefer static libraries, may produce broken results.

Yep, calling error() on Windows might be the only solution.

(I am no expert on Windows or MSVC. But I've been told that it "does" produce broken results -- you end up building both a dll and a static library, and the dll works, and the static library doesn't.)

Awesome.

eli-schwartz · 2022-03-22T19:05:39Z

meson.build

    ['ini.c'],
    include_directories : inc_inih,
-    c_args : arg_static,
+    c_args : [arg_static, extra_args, '-DINI_SHARED_LIB_BUILDING'],


This option is not needed except on Windows / MSVC, anyway you can "probably" use add_project_arguments() for this.

By the way I noticed that you can pass [arg_static, extra_args] to library.c_args but not declare_dependency.compile_args (it works with arg_static + extra_args though) - "List item must be one of <class 'str'>, not <class 'list'>"
Not sure if this is a Meson bug

Some places do implicit flattening at time of use, some do not. Personally I find implicit flattening makes me nervous.

eli-schwartz · 2022-03-22T19:06:47Z

meson.build

    pkg.generate(lib_inih,
        name : 'inih',
        description : 'simple .INI file parser',
+        extra_cflags : extra_args,


By only adding that value on MSVC, not on Linux, you can reduce unneeded arguments included in pkg-config files on Linux.

benhoyt · 2022-03-22T19:42:48Z

Thanks for the submission. Hmm, I don't mind messy build files, but I'm not a fan of polluting the ini.h and INIReader.h source code with this stuff. I will merge if we really need to, but is there no other way to make this work for Windows DLLs, e.g., with a command line parameter when linking?

Interested in others' thoughts too.

eli-schwartz · 2022-03-22T19:54:14Z

The thing is that some of this "mess" is needed anyway. You want to unconditionally define it to __attribute__ ((visibility ("default"))) on GCC/clang or anything else that supports the visibility attribute, because that leads to better GCC/clang optimizations and a leaner symbol table.

It's a one-line define on GCC/clang, plus adding the macro in front of each public symbol.

The problem is of course the MSVC compiler, which has... "interesting" design and requires a complex maze of conditions to determine whether or not to pass MSVC's competing attribute, declspec. And also, which value to use for the MSVC attribute.

MSVC also has the concept of .def files, which can be passed in meson using vs_module_defs: 'inih.def' and you get to manually maintain that file which has a list of public symbols.

Of course, you want to use GCC symbol visibility regardless of what you do with MSVC.

(Technically, GCC can do like the .def file too. GCC calls it an ld version script, and it uses a totally different format, but again, it's a list of public symbols marked as global, and a wildcard on all remaining local symbols. Use of an ld version script does NOT enable GCC to perform compiler optimizations on symbols that aren't public, because it doesn't know until link time that they aren't public. At link time, the private symbols still exist, they just get hidden.)

Tachi107 · 2022-03-22T20:33:27Z

Thanks for the submission. Hmm, I don't mind messy build files, but I'm not a fan of polluting the ini.h and INIReader.h source code with this stuff. I will merge if we really need to, but is there no other way to make this work for Windows DLLs, e.g., with a command line parameter when linking?

I know, it sucks, but the other options are even worse in my opinion (as Eli says, you'd have to maintain some files that would need to be (manually?) updated every time you modify your symbols). I don't know exactly how painful they are to adopt because I've never bothered to look into the alternatives, but having that set of #ifdefs and the various NAME_API is done in a lot of projects

They are required to properly build DLLs on Windows, and improve the quality of shared objects on Linux. See https://gcc.gnu.org/wiki/Visibility for details. This issue was first discovered here: mesonbuild/wrapdb#340 (comment)

benhoyt · 2022-03-23T06:05:48Z

Okay, thanks. I'll go ahead and merge this, given that it seems there aren't good workarounds. @stephanlachnit Could I please get your thumbs-up first?

stephanlachnit · 2022-03-23T15:26:10Z

Looks fine to me. In theorey we could also disable shared libraries on Windows and set the default to static there, as it seems quite unlikely to me that anyone would actually use that.
This would allow the code to stay the same, but if the code is more optimizied on Unix with the flags anyway it doesn't really matter.

PS: oh and it would be nice if you could tag r54 if merged.

benhoyt · 2022-03-23T19:30:45Z

Thanks for the contribution, @Tachi107!

benhoyt · 2022-03-23T19:35:23Z

@stephanlachnit r54 tagged and released: https://github.com/benhoyt/inih/releases/tag/r54

eli-schwartz · 2022-03-23T20:07:29Z

Thanks, inih is now updated on the WrapDB with passing CI for Windows/macOS/Linux. :)

Tachi107 · 2022-03-23T20:18:27Z

Thanks for merging!

Thanks: benhoyt/inih#134 (comment)

eli-schwartz suggested changes Mar 22, 2022

View reviewed changes

Add visibility symbols

61b20a1

They are required to properly build DLLs on Windows, and improve the quality of shared objects on Linux. See https://gcc.gnu.org/wiki/Visibility for details. This issue was first discovered here: mesonbuild/wrapdb#340 (comment)

Tachi107 force-pushed the windows-dll branch from 178e3fa to 61b20a1 Compare March 22, 2022 20:53

benhoyt merged commit 5a31af9 into benhoyt:master Mar 23, 2022

Tachi107 deleted the windows-dll branch March 23, 2022 20:18

Tachi107 added a commit to Tachi107/cloudflare-ddns that referenced this pull request Mar 24, 2022

build: only define TACHI_SHARED_LIB on Windows

c47f717

Thanks: benhoyt/inih#134 (comment)

Tachi107 added a commit to Tachi107/cloudflare-ddns that referenced this pull request Mar 24, 2022

build: only define TACHI_SHARED_LIB on Windows

55015c1

Thanks: benhoyt/inih#134 (comment)

SamuelMarks mentioned this pull request Jul 20, 2022

[inih] Upgrade from r51 to r56 microsoft/vcpkg#25879

Merged

Uh oh!

Add visibility symbols #134

Add visibility symbols #134

Uh oh!

Conversation

Tachi107 commented Mar 22, 2022

Uh oh!

eli-schwartz Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

eli-schwartz Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

Tachi107 Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-schwartz Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tachi107 Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

eli-schwartz Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

Tachi107 Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-schwartz Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

eli-schwartz Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

benhoyt commented Mar 22, 2022

Uh oh!

eli-schwartz commented Mar 22, 2022

Uh oh!

Tachi107 commented Mar 22, 2022

Uh oh!

benhoyt commented Mar 23, 2022

Uh oh!

stephanlachnit commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benhoyt commented Mar 23, 2022

Uh oh!

benhoyt commented Mar 23, 2022

Uh oh!

eli-schwartz commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tachi107 commented Mar 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tachi107 Mar 22, 2022 •

edited

Loading

eli-schwartz Mar 22, 2022 •

edited

Loading

Tachi107 Mar 22, 2022 •

edited

Loading

stephanlachnit commented Mar 23, 2022 •

edited

Loading

eli-schwartz commented Mar 23, 2022 •

edited

Loading