DLL: Missing extern in global variable declarations in libfmapi/libfmapi_class_identifier.h

[uckelman-sf]

Several global variables are declared in libfmapi/libfmapi_class_identifier.h, such as

LIBFMAPI_EXTERN_VARIABLE \
uint8_t libfmapi_class_identifier_mapi[ 16 ];

Because they are declared in a header and thus cannot be defined there, each such variable needs to be declared with the extern keyword.

The LIBFMAPI_EXTERN_VARIABLE macro is defined in `libfmapi/libfmapi_extern.h':

#if defined( __CYGWIN__ ) || defined( __MINGW32__ )
#define LIBFMAPI_EXTERN_VARIABLE	extern
#else
#define LIBFMAPI_EXTERN_VARIABLE	LIBFMAPI_EXTERN
#endif

#else
#define LIBFMAPI_EXTERN		/* extern */
#define LIBFMAPI_EXTERN_VARIABLE	extern

#endif /* !defined( HAVE_LOCAL_LIBFMAPI ) */

In some cases, LIBFMAPI_EXTERN_VARIABLE is defined to be extern, and in some cases to be whatever LIBFMAPI_EXTERN expands to.

LIBFMAPI_EXTERN is defined in include/libfmapi/extern.h:

#if defined( LIBFMAPI_DLL_EXPORT )
#define LIBFMAPI_EXTERN __declspec(dllexport)

#elif defined( LIBFMAPI_DLL_IMPORT )
#define LIBFMAPI_EXTERN extern __declspec(dllimport)

#else
#define LIBFMAPI_EXTERN extern

#endif

In the case where LIBFMAPI_DLL_EXPORT is defined, LIBFMAPI_EXTERN will be __declspec(dllexport), which does not contain extern, so in that case LIBFMAPI_EXTERN_VARIABLE will also not contain extern, thus leaving the global variables in libfmapi/libfmapi_class_identifier.h declared incorrectly.

As global variables declared in header files need to be declared extern in all cases, it would be more straightforward and less error-prone to declare them to be extern unconditionally at the site of declaration.

Additionally, since functions have extern linkage by default, it's not clear why the LIBFMAPI_EXTERN should ever contain extern at all---functions never require it, and global variables always do. The LIBFMAPI_EXTERN macro should probably be renamed so that it reflects its actual purpose, which is to act as as switch for __declspec(dllexport) and __declspec(dllimport).

[uckelman-sf]

Note that libclocale and libcnotify have the same problem with their EXTERN macros.

[joachimmetz]

Note that per README this is an experimental library. Also why do you need this library as a DLL? For which specific compiler/linker and version and options are you experiencing issues? (given that different compilers/linkers and options have different behaviors).

[uckelman-sf]

liblocale and libcnotify aren't experimental, and they have the same problem. libpff isn't experimental and it depends on libfmapi.

If you're going to use a compiled library with Python on Windows, you need it as a DLL.

I ran:

autoreconf -fi
mingw64-configure
mingw64-make

The linking command for the DLL was:

x86_64-w64-mingw32-gcc -shared  .libs/libfmapi.o .libs/libfmapi_class_identifier.o .libs/libfmapi_checksum.o .libs/libfmapi_codepage.o .libs/libfmapi_debug.o .libs/libfmapi_entry_identifier.o .libs/libfmapi_error.o .libs/libfmapi_lzfu.o .libs/libfmapi_one_off_entry_identifier.o .libs/libfmapi_property_type.o .libs/libfmapi_service_provider_identifier.o .libs/libfmapi_support.o .libs/libfmapi_value_type.o  -Wl,--whole-archive ../libcerror/.libs/libcerror.a ../libcthreads/.libs/libcthreads.a ../libcdata/.libs/libcdata.a ../libcnotify/.libs/libcnotify.a ../libuna/.libs/libuna.a ../libfdatetime/.libs/libfdatetime.a ../libfguid/.libs/libfguid.a ../libfwnt/.libs/libfwnt.a -Wl,--no-whole-archive  -lssp  -O2 -g -fstack-protector   -o .libs/libfmapi-1.dll -Wl,--enable-auto-image-base -Xlinker --out-implib -Xlinker .libs/libfmapi.dll.a

gcc version:

[juckelman@midas libfmapi-20240415]$ x86_64-w64-mingw32-gcc --version
x86_64-w64-mingw32-gcc (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38)
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Here you can see that the libfmapi_class_identifier_mapi symbol isn't exported:

[juckelman@midas libfmapi-20240415]$ nm libfmapi/.libs/libfmapi.dll.a | grep libfmapi_class_identifier_mapi

With everything else unchanged save for correcting the use of extern, symbols for the global variables do show up in libfmapi.dll.a as expected.

[joachimmetz]

liblocale and libcnotify aren't experimental, and they have the same problem. libpff isn't experimental and it depends on libfmapi.

libpff uses libfmapi as a static library, how would you run into this issue?

[uckelman-sf]

libpff uses libfmapi as a static library, how would you run into this issue?

libpff doesn't need to link statically---it can link to a libfmapi DLL without any problems once the incorrect use of extern is fixed.

[uckelman-sf]

I've created PR #8 for you, which fixes the problem.

[joachimmetz]

libpff doesn't need to link statically---it can link to a libfmapi DLL without any problems once the incorrect use of extern is fixed.

that is not the way it comes configured.

I've created PR #8 for you, which fixes the problem.

Aren't you now telling the linker to get the data from libfmapi during build but not actually getting this information at run time?

[uckelman-sf]

Aren't you now telling the linker to get the data from libfmapi during build but not actually getting this information at run time?

I don't believe so. What specifically makes you think that?

[joachimmetz]

I don't believe so. What specifically makes you think that?

Because it is comparable to the way a Windows static library exports its function and variables

[uckelman-sf]

What way are you referring to? Can you point me to some reference for it? So far as I know, extern is not involved with Windows exports at all.

(I've been using the patch I provided when producing Windows static libraries for several yeas now, and haven't encountered any issues with it. If there is something I'm doing incorrectly, I'd appreciate being made aware of it.)

[joachimmetz]

what do you mean with "Windows export" do you mean DLL exports?

https://github.com/libyal/libfmapi/blob/main/include/libfmapi/extern.h

unless you define LIBFMAPI_DLL_EXPORT or LIBFMAPI_DLL_IMPORT it will default to export

This is included in https://github.com/libyal/libfmapi/blob/main/libfmapi/libfmapi_extern.h

your PR just move externs around, but technically you are using the same definition as the static version of the library.

given that you are using mingw libtool is also involved.

(I've been using the patch I provided when producing Windows static libraries for several yeas now, and haven't encountered any issues with it. If there is something I'm doing incorrectly, I'd appreciate being made aware of it.)

I'm not saying that you will have issues, I'm telling you, you are basically using the library as static for the variables with the proposed changes. So your changes are not needed, just use libfmapi as a static library.

[uckelman-sf]

what do you mean with "Windows export" do you mean DLL exports?

I was paraphrasing what you said here:

Because it is comparable to the way a Windows static library exports its function and variables

I was hoping you'd tell me what you meant by that. I would still like to know what you were referring to specifically. Maybe an example would help clarify?

your PR just move externs around, but technically you are using the same definition as the static version of the library.`

My PR does not just move exerns around. It ensures that the global variables declared in headers are extern in all circumstances. That is the only correct way to declare global variables in C headers which could be included from more than one location.

you are basically using the library as static for the variables with the proposed changes

That's not correct. extern is unrelated to how libraries are linked, dynamically or statically. What extern controls for variables is whether a variable declared in the global scope can be seen across translation units, so that when you #include "libfmapi/libfmapi_class_identifier.h", everywhere that does so is able to refer to the same libfmapi_class_identifier_mapi variable.

[joachimmetz]

I was hoping you'd tell me what you meant by that.

My framing was very different, i was not saying "Windows exports" I was saying "a Windows static library".

So far as I know, extern is not involved with Windows exports at all.

For a DLL correct, see https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll-using-declspec-dllexport?view=msvc-170 for context

that means export does not play a role in this. The only things export plays a role is exactly what you call out "whether a variable declared in the global scope". So the variable gets just linked into the main executable and not dynamically linked at all.

That means there is no value to try to use libfmapi as a DLL for your purpose at this point.

[uckelman-sf]

We're clearly not communicating here. I'll try again on Monday to give the clearest, most concise explanation I can of the problem and its solution.

[joachimmetz]

We're clearly not communicating here

Maybe explain what you are trying to accomplish. For now I only see changes because you want things to be done in a certain way, without added value, and making maintenance more tedious.

[uckelman]

Would you explain why you think the patch makes maintenance more tedious?

[joachimmetz]

If you talk about communication break down you are not providing me the details I need. I unfortunately do not have the time to debate this indefinitely. You either provide me the information or I'll just close this out as something I currently don't have time for.

[uckelman]

I'll answer your questions the best I can:

• What I am trying to accomplish is to make the code correctly declare its global variables. (Forget everything I said about DLLs. None of it is relevant.)
• A variable at the global scope which is declared in a header that could be included from multiple source files MUST be declared as extern in C.
• When none of HAVE_LOCAL_LIBFMAPI, __CYGWIN__, and __MINGW32__ are defined, LIBFMAPI_EXTERN_VARIABLE is defined to be LIBFMAPI_EXTERN.
• When LIBFMAPI_DLL_EXPORT is defined, LIBFMAPI_EXTERN is __declspec(dllexport).
• libfmapi_class_identifier_mapi is declared as LIBFMAPI_EXTERN_VARIABLE uint8_t libfmapi_class_identifier_mapi[ 16 ];
• Therefore, in the case when HAVE_LOCAL_LIBFMAPI, __CYGWIN__, and __MINGW32__ are undefined and LIBFMAPI_DLL_EXPORT is defined, the declaration of libfmapi_class_identifier_mapi is __declspec(dllexport) uint8_t libfmapi_class_identifier_mapi[ 16 ];.
• Declaring libfmapi_class_identifier_mapi at the global scope in a header without declaring it extern is a bug.
• My patch eliminates the case where there is a bug. Be the change you wish to see in the world.

[joachimmetz]

Forget everything I said about DLLs. None of it is relevant.

Then why mention it to begin with? Why not clearly communicate to begin with? Why waste my time?

Also I would disagree that DLL/shared library is not relevant, the code path you highlight includes DLL export/import.

autoreconf -fi
mingw64-configure
mingw64-make

This will create a DLL given behavior of configure.

Libpff CI test show no issue with the current approach https://ci.appveyor.com/project/libyal/libpff/build/job/0jc8kloe7jcn6ycx

Now for DLL specifically adding the extern keyword with the dllspec might be needed https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html

But as you can see in the example this has different cases for different compilers/linkers.

Be the change you wish to see in the world.

Not sure what you intend to say with this, or what value it ads to solve this issue. It comes across as pedantic/arrogant especially in combination with "MUST be declared". Is that the change you want to see in the world more pedantic/arrogant people that push their option?

Your patch might fix whatever problem you are trying to solve. Which you have not clearly explained to me, besides stating what MUST or MUST NOT. However as maintainer how am I sure it does not break anything else? Also the proposed changes litter the code with more extern keywords instead of fixing LIBFMAPI_EXTERN and LIBFMAPI_EXTERN_VARIABLE.

I'll close this issue and will take a look to change the code to match https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html when time permits.

[uckelman]

Fair enough. I've done a terrible job of explaining this. I'll try again on a separate issue and open a new, cleaner PR.

[uckelman]

Not sure what you intend to say with this, or what value it ads to solve this issue. It comes across as pedantic/arrogant especially in combination with "MUST be declared". Is that the change you want to see in the world more pedantic/arrogant people that push their option?

"MUST" is referencing the language used in RFCs. There's no reason to take offense at it.

I was simply trying to express that all of my comments are offered in a spirit of helpfulness. As I've benefited from your software, I feel it's my duty to try to be helpful in return. I'm saddened that you don't see it this way.

[uckelman]

Why not clearly communicate to begin with? Why waste my time?

A lot of projects have descriptions of how to report bugs, designed to save everyone time. You don't have one, so I gave you the kind of bug report I would want myself as a maintainer. Maybe you could write a description of how to report bugs for your projects, to save everyone time?

[joachimmetz]

A lot of projects have descriptions of how to report bugs, designed to save everyone time.

Let's say that my experience is that most people ignore the template and relevant documentation.

I was simply trying to express that all of my comments are offered in a spirit of helpfulness.

Understood, but this is the hard part of communication and best with asynchronous issue reports to stick to facts way and to provide the necessary context.

"MUST" is referencing the language used in RFCs.

In plain English "MUST" looks very different. How would one know you are using language from a specific RFC? I can strongly recommend to leave as little ambiguity as possible especially when you are talking to an international audience. Stick to neutral plain English (no metaphors) and be aware that details and/or nuances are easily lost.

There's no reason to take offense at it.

No offense taken, I'm just giving feedback on how it comes across.

I'll try again on a separate issue and open a new, cleaner PR.

For now no need, as I indicated this project is experimental. So it will likely change. What will benefit me at this stage is to understand your use case, given that in its current form the things you highlighted before are not causing issues for me.

[uckelman]

For now no need, as I indicated this project is experimental.

In that case, I will provide patches for several of the projects which are not experimental, as well.

[joachimmetz]

In that case, I will provide patches for several of the projects which are not experimental, as well.

I would recommend to wait with that and explain your use case(s) first. I'm wary for changes that are mostly because of someones preference.

[uckelman]

In order to help me know how to explain my use case, would you please identify which of the following you disagree with?

1. A variable at the global scope which is declared in a header that could be included from multiple source files must be declared as extern in C.
2. When HAVE_LOCAL_LIBFMAPI, __CYGWIN__, and __MINGW32__ are undefined and LIBFMAPI_DLL_EXPORT is defined, the declaration of libfmapi_class_identifier_mapi is __declspec(dllexport) uint8_t libfmapi_class_identifier_mapi[ 16 ];.
3. Declaring libfmapi_class_identifier_mapi at the global scope in a header without declaring it extern is a bug.

Knowing where we disagree would help me focus my explanation.

[joachimmetz]

In order to help me know how to explain my use case, would you please identify which of the following you disagree with?

I'm thinking in much more basic terms. How are trying to use libfmapi (or other libyal libraries) if so:

• for what purpose?
• how (as a DLL, shared library, static)?
• what is the license of the source you are using it from?

[uckelman]

I'm using libfmapi from libpff, building in five configurations---as shared libs for Linux, as 32-bit Windows DLLs, as 64-bit Windows DLLs, as 32-bit Windows static libs, and as 64-bit Windows static libs. There is no license, as this is for internal software which isn't being distributed.

I have answered your questions. Would you please answer my question?

[joachimmetz]

Would you please answer my question?

Which question? Do you mean "In order to help me know how to explain my use case, would you please identify which of the following you disagree with?"

You last comment comment just explained your use case. The thing I don't understand that if you are intended to use libpff, why do you need libfmapi as a stand-alone shared library?

Currently the recommended configuration is to build libfmapi as static into libpff. It is not yet ready to be intended to be used as an independent DLL.

A variable at the global scope which is declared in a header that could be included from multiple source files must be declared as extern in C.

If this is a public header maybe, also for a DLL extern __declspec this needs to be extern and this is something that needs to be tested with multiple compilers/linkers. If this is a private header it not needs to be.

Declaring libfmapi_class_identifier_mapi at the global scope in a header without declaring it extern is a bug.

You appear to very obsessed about wanting to call it a bug. The code is experimental, not finished, where does it say your use case is supported?

Now I finally understand what you are trying to accomplish, why not define LIBFMAPI_EXTERN_VARIABLE like the example in https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html and make sure it is properly set in different contexts. However you'll have to test it with different compiler/linkers.

However a question that I need to answer first, if this is really the desired API for libfmapi to have.

[uckelman]

Which question? Do you mean "In order to help me know how to explain my use case, would you please identify which of the following you disagree with?"

Yes, that is the question I wanted you to answer.

The thing I don't understand that if you are intended to use libpff, why do you need libfmapi as a stand-alone shared library?

You can run into linking problems involving multiply defined symbols if you build libpff into libfmapi but then also link to libfmapi because you're using it directly.

Currently the recommended configuration is to build libfmapi as static into libpff. It is not yet ready to be intended to be used as an independent DLL.

Wouldn't it be nice for DLLs to work correctly? That is part of why I am offering you this patch. I have a few more I could also give you for building DLLs if you'd like them.

If this is a public header maybe, also for a DLL extern __declspec this needs to be extern and this is something that needs to be tested with multiple compilers/linkers. If this is a private header it not needs to be.

This answers my question. You're mistaken about what extern does.

Variables declared in the global scope in private headers also need to be declared extern. I can see why the libtool guide is misleading. It includes extern in its example so that the reader doesn't leave it out entirely for global variables intended to be imported or exported, but that gives the incorrect impression that extern is related specifically to building DLLs. It is not.

Consider what happens if you have a variable declared with LIBFMAPI_EXTERN_VARIABLE and you decide to make it private instead. If you remove the LIBFMAPI_EXTERN_VARIABLE, it will lose its extern, and you'll now have an incorrect declaration unless you remember to add an extern back to the variable's declaration. The point I was trying and apparently failed to make earlier is that writing the extern explicitly in the declaration helps avoid this mistake. The libtool docs offer an example that's too compressed to make this clear, unfortunately.

Now I finally understand what you are trying to accomplish, why not define LIBFMAPI_EXTERN_VARIABLE like the example in https://www.gnu.org/software/libtool/manual/html_node/Windows-DLLs.html and make sure it is properly set in different contexts. However you'll have to test it with different compiler/linkers.

I will give you a patch on Monday which follows the libtool example. What they've done is not optimal for maintainability in my opinion, as noted above, but it is correct.

However a question that I need to answer first, if this is really the desired API for libfmapi to have.

Neither the patch I've given you already nor the one I will give you tomorrow have any effect on the libfmapi API or your ability to adjust it.

[joachimmetz]

You can run into linking problems involving multiply defined symbols if you build libpff into libfmapi but then also link to libfmapi because you're using it directly.

you mean link libfmapi into libpff? You can also run into linking problems when using shared libraries.

This answers my question. You're mistaken about what extern does.
Variables declared in the global scope in private headers also need to be declared extern.

You incorrectly interpret my answer. I'm not saying anything about needing to use extern or not. I'm saying that depending on the private usage the variable does not need to be included in the global scope.

I can see why the libtool guide is misleading. It includes extern in its example so that the reader doesn't leave it out entirely for global variables intended to be imported or exported, but that gives the incorrect impression that extern is related specifically to building DLLs.

Then I recommend getting that updated first.

[uckelman-sf]

You incorrectly interpret my answer. I'm not saying anything about needing to use extern or not. I'm saying that depending on the private usage the variable does not need to be included in the global scope.

I had not understood until now that you were talking about variables being removed from the global scope. We're in agreement that those should not be extern.

I've opened a second PR, #9, this time following what the libtool docs recommend, as per your preference.

[uckelman-sf]

I've opened libyal/libclocale#8 and libyal/libcnotify#5, which fix the same problem in libclocale and libcnotify, respectively.