Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exported symbols and dll builds #120

Closed
sezero opened this issue Aug 5, 2019 · 18 comments
Closed

exported symbols and dll builds #120

sezero opened this issue Aug 5, 2019 · 18 comments

Comments

@sezero
Copy link
Contributor

sezero commented Aug 5, 2019

libFLAC.dll built by MSVC exports only the symbols marked as FLAC_API,
because FLAC_API is defined properly as FLAC_API_EXPORTS in export.h.
libFLAC.dll built by MinGW exports all of the symbols it has, because
FLAC_API_EXPORTS is restricted to MSVC in export.h: this is obviously
wrong.

As a test, I tried something like this:

diff --git a/include/FLAC/export.h b/include/FLAC/export.h
index d52f0bb..628fe5f 100644
--- a/include/FLAC/export.h
+++ b/include/FLAC/export.h
@@ -59,7 +59,7 @@
 #if defined(FLAC__NO_DLL)
 #define FLAC_API
 
-#elif defined(_MSC_VER)
+#elif defined(_WIN32)
 #ifdef FLAC_API_EXPORTS
 #define	FLAC_API __declspec(dllexport)
 #else
diff --git a/configure.ac b/configure.ac
index 0228a12..c2edf0a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -493,6 +493,17 @@ if test x$enable_stack_smash_protection = "xyes" ; then
 	XIPH_GXX_STACK_PROTECTOR
 	fi
 
+if test x$enable_shared != "xyes" ; then
+	CPPFLAGS="-DFLAC__NO_DLL $CPPFLAGS"
+	fi
+
+AH_VERBATIM([FLAC_API_EXPORTS],
+[/* libtool defines DLL_EXPORT for windows dll builds,
+   but flac code relies on FLAC_API_EXPORTS instead. */
+#ifdef DLL_EXPORT
+# define FLAC_API_EXPORTS
+#endif])
+
 AC_CONFIG_FILES([ \
 	Makefile \
 	src/Makefile \

But that doesn't work, because flac.exe and metaflac.exe unfathomably
rely on the private symbols of windows_unicode_filenames.c which only
go into the dll and wrongly exported. MSVC builds aren't affected by
this, because those exes are linked to static libFLAC and not to the
dll.

What is a good / acceptable solution for this?

@erikd
Copy link
Member

erikd commented Aug 5, 2019

I don't use Windows. Maybe @lvqcl can help.

@lvqcl
Copy link
Contributor

lvqcl commented Aug 5, 2019

I don't know much how MinGW or configure.ac work, so I cannot help here.

Maybe it makes sense to add FLAC_API_EXPORTS to those functions from windows_unicode_filenames.c ?

Or leave it as is (if it's not a problem)?

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

Maybe it makes sense to add FLAC_API_EXPORTS to those functions
from windows_unicode_filenames.c ?

Or leave it as is (if it's not a problem)?

Adding them to api or leaving it as is will be a problem when someone
tries to get smart and assume that they really are official libFLAC apis
and when that changes or when they change people will start whining.
The dllexport or visibility attributes prevents just that.

So no, I don't think either would be a good idea.

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

One solution is going back to 1.3.0 way of doing it. I.e. move windows_unicode_filenames.c
to win_utf8_io and change libFLAC/Makefile.am like the 1.3.0 one (see:

if OS_IS_WINDOWS
)
an then do changes similar to I described in the first post above.

@erikd
Copy link
Member

erikd commented Aug 5, 2019

libFLAC.dll built by MinGW exports all of the symbols it has, because FLAC_API_EXPORTS is restricted to MSVC in export.h: this is obviously wrong.

I think that hints at the most acceptable solution.

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

libFLAC.dll built by MinGW exports all of the symbols it has, because FLAC_API_EXPORTS is restricted to MSVC in export.h: this is obviously wrong.

I think that hints at the most acceptable solution.

OK, will start working on this and create a pull request.

@lvqcl
Copy link
Contributor

lvqcl commented Aug 5, 2019

One solution is going back to 1.3.0 way of doing it. I.e. move windows_unicode_filenames.c
to win_utf8_io and change libFLAC/Makefile.am like the 1.3.0

IIRC there were complaints that libFLAC wasn't compatible with UWP, so it was changed.

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

One solution is going back to 1.3.0 way of doing it. I.e. move windows_unicode_filenames.c
to win_utf8_io and change libFLAC/Makefile.am like the 1.3.0

IIRC there were complaints that libFLAC wasn't compatible with UWP, so it was changed.

What was the incompatibility? The calls used in the helpers (possibly)
or the build procedure (unlikely)?

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

I have a patch working for autotools+mingw (tested by cross-compiling on linux).
Will try testing cmake (and possibly msvc) soon.

@lvqcl
Copy link
Contributor

lvqcl commented Aug 5, 2019

Also, I just built flac 1.3.0 with MinGW, and libFLAC-8.dll also exports some symbols that are not prefixed with FLAC, and some of them are necessary for flac/metaflac.

For example, flac.exe 1.3.0 uses the following functions from libFLAC-8.dll:

chmod_utf8
CreateFile_utf8@28
fopen_utf8
fprintf_utf8
get_utf8_argv
rename_utf8
_stat64_utf8
strlen_utf8
unlink_utf8
utime_utf8
vfprintf_utf8
win_get_console_width

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

I know. This has been a long-standing problem,.

@lvqcl
Copy link
Contributor

lvqcl commented Aug 5, 2019

What was the incompatibility?

#32
http://lists.xiph.org/pipermail/flac-dev/2017-January/006098.html
also: buggy 972454e

and IIRC there were some others, but I can't find them quickly.

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

What was the incompatibility?

#32
http://lists.xiph.org/pipermail/flac-dev/2017-January/006098.html
also: buggy 972454e

So, the issue was only with the function calls used in there.
The solution I suggest doesn't change any of the calls, but
just the build system.

@lvqcl
Copy link
Contributor

lvqcl commented Aug 5, 2019

For the record: flac.exe 1.3.3 needs these (internal and undocumented) functions from libFLAC-8.dll:

flac_internal_chmod_utf8
flac_internal_fopen_utf8
flac_internal_get_utf8_filenames
flac_internal_rename_utf8
flac_internal_set_utf8_filenames
flac_internal_stat64_utf8
flac_internal_unlink_utf8
flac_internal_utime_utf8

At least, they are now clearly marked as internal.

I know. This has been a long-standing problem,.

My point was that I can't see how "going back to 1.3.0 way of doing it" will solve this problem.

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

My point was that I can't see how "going back to 1.3.0 way of doing it" will solve this problem.

See #122

@erikd
Copy link
Member

erikd commented Aug 5, 2019

We will not be going back to the way 1.3.0 did it.

@sezero
Copy link
Contributor Author

sezero commented Aug 5, 2019

As I said in #122 I'm out of ideas. We can close this and #122 if you require and leave things as is.

@sezero
Copy link
Contributor Author

sezero commented Oct 10, 2019

Closing this as #147 is merged.

@sezero sezero closed this as completed Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants