HTSLIB_EXPORT pessimises clang error messages

[jmarshall]

Consider the following program, which misuses hts_open():

#include <htslib/hts.h>

htsFile *do_open(const char *fname) {
    return hts_open(fname);
}

Compiling this with GCC produces an accurate error message:

$ gcc-9 -I. bork.c 
bork.c: In function 'do_open':
bork.c:4:12: error: too few arguments to function 'hts_open'
    4 |     return hts_open(fname);
      |            ^~~~~~~~
In file included from bork.c:1:
./htslib/hts.h:474:10: note: declared here
  474 | htsFile *hts_open(const char *fn, const char *mode);
      |          ^~~~~~~~

However compiling it with Clang produces the following diagnostics:

$ clang -I. bork.c 
bork.c:4:26: error: too few arguments to function call, expected 2, have 1
    return hts_open(fname);
           ~~~~~~~~      ^
./htslib/hts.h:473:1: note: 'hts_open' declared here
HTSLIB_EXPORT
^
./htslib/hts_defs.h:109:23: note: expanded from macro 'HTSLIB_EXPORT'
#define HTSLIB_EXPORT HTS_DLL_EXPORT
                      ^
./htslib/hts_defs.h:101:24: note: expanded from macro 'HTS_DLL_EXPORT'
#define HTS_DLL_EXPORT __attribute__((__visibility__("default")))
                       ^
1 error generated.

Here the error message at bork.c:4 is accurate, but the notes showing you the location of the hts_open declaration are useless and voluminous.

This inaccurate location for the declaration is surely a Clang bug; e.g., it was reported as bug 23564 in 2015, but sadly the lack of response on that bug report also gives you a clue as to how likely this is to be fixed in Clang any time soon.

To be precise, Clang is reporting the location of the declaration as the first character (post macro expansion) that is in any way a part of the declaration (and in our headers that is now on the line before the interesting one — the point of showing the declaration is to show you the parameters it was expecting), and more egregiously if that character is part of a macro expanding to something non-empty, it shows a trace of the macro expansions (which are irrelevant in this case).

[jmarshall]

This can be worked around in several ways:

extern HTSLIB_EXPORT htsFile *hts_open(const char *fn, const char *mode);

htsFile HTSLIB_EXPORT *hts_open(const char *fn, const char *mode);

HTSLIB_EXPORT_PRE
htsFile *hts_open(const char *fn, const char *mode) HTSLIB_EXPORT_POST;

The first two make sure the first character is not part of the macro and put the interesting bit on the first line. In my limited testing on Windows and Illumos and reading of the __declspec documentation, they both work as expected for the __declspec and __global forms of HTSLIB_EXPORT too.

The last one introduces an extra macro to go after the declaration, and htslib/hts_defs.h is rearranged so that the __attribute__ form goes in HTSLIB_EXPORT_POST instead of HTSLIB_EXPORT_PRE (or however the macros are named), so this approach doesn't change the expansion for the __declspec or __global forms.

If the maintainers agree and have a preference for one of these transformations, I am happy to prepare a PR accordingly. I'm thinking probably the extern approach is the least appalling…?

[jkbonfield]

Indeed the extern approach looks best to me.

All on one line without extern is an alternative. It gives a sensible error at least, although it's still excessively verbose.

[jmarshall]

All on one line without extern is an alternative

That is true, especially if we compress the two macros into one to reduce the spurious notes (and I'd toy with renaming it to HTS_EXPORT to parallel the others and shorten it a little):

$ clang -I. bork.c 
bork.c:4:26: error: too few arguments to function call, expected 2, have 1
    return hts_open(fname);
           ~~~~~~~~      ^
./htslib/hts.h:464:1: note: 'hts_open' declared here
HTS_EXPORT htsFile *hts_open(const char *fn, const char *mode);
^
./htslib/hts_defs.h:101:20: note: expanded from macro 'HTS_EXPORT'
#define HTS_EXPORT __attribute__((__visibility__("default")))
                   ^
1 error generated.

[jmarshall]

Actually I guess this ugly but shorter approach is also available (I always forget that we have newfangled preprocessor varargs now):

// #ifdefs to select one of the following
#define HTS_EXPORT(...)  __declspec(dllexport) __VA_ARGS__
#define HTS_EXPORT(...)  __VA_ARGS__  __attribute__((__visibility__("default")))

HTS_EXPORT(htsFile *hts_open(const char *fn, const char *mode));

…never mind, that one's good for clang but makes GCC spell out the HTS_EXPORT definition in a spurious note…

[daviesrob]

I think they were put on the line above mainly to stop the actual function definition from disappearing off to the right hand side of the screen. There was also an idea that you could easily find the exported functions using grep -A 1 HTSLIB_EXPORT ..., which is why they were consistently put in the same place. Really, it's somewhat annoying that they have to go on the front.

HTS_EXPORT on the same line would probably be the best work-around.

[jmarshall]

This bug has now been fixed in mainline Clang, which will pinpoint the hts_open identifier in the declaration similarly to the GCC diagnostic.

Unfortunately the fix missed the branch for the imminent release 10 by some weeks, so I guess it might turn up in macOS's system compiler as soon as WWDC in June 2021 (sigh…) but it's difficult to hazard a guess as Apple hasn't given any clues as to how Xcode's Clang corresponds to upstream LLVM versions for some years.

However seeing as it will one day be fixed for Clang users, I'm now proposing to leave HTSlib's headers alone and just simplify the HTSLIB_EXPORT macro definition so there's only one stupid “expanded from macro” diagnostic. See PR #1029.