Compiling with clang on Windows does not work

[teepean]

Hi!

I was trying to build htslib on Windows (MSYS2, clang 14.0.0) using clang but it does not work at the moment.

Compilation produces several errors regarding dllexport:

`hfile.c:105:8: warning: redeclaration of 'hfile_init' should not add 'dllexport' attribute [-Wdll-attribute-on-redeclaration]
hFILE *hfile_init(size_t struct_size, const char *mode, size_t capacity)
^

./hfile_internal.h:98:8: note: previous declaration is here
hFILE *hfile_init(size_t struct_size, const char *mode, size_t capacity);
^

hfile.c:153:6: error: redeclaration of 'hfile_destroy' cannot add 'dllexport' attribute
void hfile_destroy(hFILE *fp)
^

./hfile_internal.h:110:6: note: previous declaration is here
void hfile_destroy(hFILE *fp);
^

hfile.c:203:5: error: redeclaration of 'hfile_set_blksize' cannot add 'dllexport' attribute
int hfile_set_blksize(hFILE *fp, size_t bufsiz) {
^

./hfile_internal.h:52:5: note: previous declaration is here
int hfile_set_blksize(hFILE *fp, size_t bufsiz);`

And so on. Any ideas how to fix this?

[jkbonfield]

I suspect this is a new clang warning that has appeared in clang 14. I don't think we've tested that yet. It looks like it was released in March 2022.

My local MSYS build has Clang-13.0, but I see at https://packages.msys2.org/package/mingw-w64-x86_64-clang there is a newer one available. I'll explore to see how to persuade it to upgrade (pacman can't see the new package currently).

Edit: I did work out how to install clang-14, but I now see it fails locally with clang-13 too. I'll look into a fix.

[jkbonfield]

This is just a generic Clang thing, which is basically incompatible with our Windows symbol exporting. See these two examples.

1. A warning generated by defining a function with dllexport attribute that was previously declared without it.

jkbon@nagafen MINGW64 ~/htslib-1.15
$ cat _.c
int foo(void);

__attribute__((dllexport))
int foo(void) {
  return 0;
}


jkbon@nagafen MINGW64 ~/htslib-1.15
$ clang -Wall -O2 -g -c _.c
_.c:4:5: warning: redeclaration of 'foo' should not add 'dllexport' attribute [-Wdll-attribute-on-redeclaration]
int foo(void) {
    ^
_.c:1:5: note: previous declaration is here
int foo(void);
    ^
1 warning generated.

2. An error generated this time, due to the addition of code that has actively used our previously declared function. Now the lack of attribute has become cast in stone, so function definition differing is now an error:

jkbon@nagafen MINGW64 ~/htslib-1.15
$ cat _.c
int foo(void);

int func2(void) {
  return foo();
}

__attribute__((dllexport))
int foo(void) {
  return 0;
}


jkbon@nagafen MINGW64 ~/htslib-1.15
$ clang -Wall -O2 -g -c _.c
_.c:8:5: error: redeclaration of 'foo' cannot add 'dllexport' attribute
int foo(void) {
    ^
_.c:1:5: note: previous declaration is here
int foo(void);
    ^
1 error generated.

The correction is to have the attribute on both function declarations and definitions:

jkbon@nagafen MINGW64 ~/htslib-1.15
$ cat _.c
__attribute__((dllexport))
int foo(void);

int func2(void) {
  return foo();
}

__attribute__((dllexport))
int foo(void) {
  return 0;
}


jkbon@nagafen MINGW64 ~/htslib-1.15
$ clang -Wall -O2 -g -c _.c

Practically speaking, as far as htslib goes this is done via the HTSLIB_EXPORT macro, and half of the above examples would be in a public header file (the function declaration) while the other bit is in the C file. We only do it for the actual function definitions themselves as the function declaration is used both internally by the library while compiling, and externally by third parties when linking against the library. Only one of these needs the __attribute__((dllexport)) if my memory serves me correctly. (There is dllimport too for the case of when linking against a library, but I think that may be deprecated.) We've been sloppy as GCC is happy to change the function declarations on-the-fly and works around this case, but clang does not (nor does MSVC). Technically Clang is the one working correct here, not gcc.

I've seen this done elsewhere by manipulating the header files so during compilation there is an BUILDING_LIBRARY macro or similar and when linking against it there is not, which in turn toggles the export macro on/off. This means the same header files can be used for both compilation of the library and external usage of it.

This is rather messy, and it's quite a bit of work to do and get correct.

For now I think the only work around is to use gcc, but I'll leave this issue open as it's a real problem that we need to work on.

[jkbonfield]

Ah a correction. I should have done make -k and it'd have become more obvious that most files do infact build. So how do they work and not this?

Well, my description of how it should be done turns out to actually be how it is done. The BUILDING_LIBRARY example is infact HTS_BUILDING_LIBRARY (hey I was pretty close given it was a random example!), and hts_defs.h does indeed have the appropriate logic to define HTSLIB_EXPORT to __attribute__((dllexport)) when building the library and empty when not (ie being used by a third party tool). We also have HTSLIB_EXPORT sprinkled through the public headers.

The problem here is it's absent from the internal headers.

So phew - this is a far less invasive change as it's basically 90% done already. I do wonder how we ever tested all that existing functionality though if it wasn't really needed for gcc? Or maybe it is in some specific cases? Anyway, this will probably get fixed sooner rather than later now I can see the problem more clearly.

Thanks for reporting it.

(Note there is some confusion here still to mull over... why are some internal functions defining themselves to be exported, when clearly they're internal? I suspect, but haven't yet checked, it's because some are called from static inline functions in the headers. It may be some of them need exports adding to the declarations, while others need exports removing from the definitions.

Or it's the abomination that is hfile-plugins which uses internal parts of the library that aren't normally considered external. Argh! Solution here is just to bless them as officially external probably. Will let the main project lead make that call. :-) )

[teepean]

Thank you for figuring this out!