samtools / htslib

C library for high-throughput sequencing data formats
Other
785 stars 447 forks source link

undeclared: gettimeofday #1606

Closed clausecker closed 1 year ago

clausecker commented 1 year ago

When compiling on FreeBSD, gettimeofday is used without a declaration in some test files:

htscodecs/tests/rANS_static_test.c:148:5: warning: implicit declaration of function 'gettimeofday' is invalid in C99 [-Wimplicit-function-declaration]
    gettimeofday(&tv1, NULL);
    ^
1 warning generated.
htscodecs/tests/rANS_static4x16pr_test.c:148:5: warning: implicit declaration of function 'gettimeofday' is invalid in C99 [-Wimplicit-function-declaration]
    gettimeofday(&tv1, NULL);
    ^
1 warning generated.
htscodecs/tests/arith_dynamic_test.c:142:5: warning: implicit declaration of function 'gettimeofday' is invalid in C99 [-Wimplicit-function-declaration]
    gettimeofday(&tv1, NULL);
    ^
1 warning generated.

This can be fixed by including <sys/time.h> in these files.

jmarshall commented 1 year ago

If you look in those files you'll see they do in fact include <sys/time.h>.

On FreeBSD, this header provides a declaration of gettimeofday() only if __XSI_VISIBLE is non-zero. This is non-zero by default or if the code defines _XOPEN_SOURCE (see cdefs.h), but will be reset to zero if you choose strict ANSI, C99, or C11 mode. Did you select any of those?

As recently predicted might become necessary in https://github.com/samtools/htslib/issues/1586#issuecomment-1500144683 :smile:, HTSlib can ensure this is non-zero by ensuring _XOPEN_SOURCE is defined to an appropriate value. There's an old draft of that in this xopen_source branch that probably needs some 500 vs 600 work (see also samtools/htslib#1246).

clausecker commented 1 year ago

I compiled htslib 1.17 with no changes on FreeBSD and tried to build and run the unit tests when this issue happened.

jkbonfield commented 1 year ago

I tried the latest freebsd with just ./configure;make and it works fine.

What compiler are you using, and are you setting any specific C flags?

clausecker commented 1 year ago

The error occurs when building the unit tests (make test). Did you try that?

I am building htslib 1.17 with clang. We pass

-O2 -pipe  -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing

as extra C flags when building this port.

jmarshall commented 1 year ago

It's a warning, not an error. Hence when these files are compiled via (HTSlib's build system's) make test the diagnostic messages scroll past quickly and are easily missed. (And the tests go on to run successfully anyway.)

FreeBSD's cdefs.h also sidesteps the default activation of __XSI_VISIBLE when _POSIX_C_SOURCE is set and _XOPEN_SOURCE is not set.

It turns out HTSlib's Makefile's rules for htscodecs/test/*.o add ‑D_POSIX_C_SOURCE=200112L to their compilation. This is incorrect; it should be ‑D_XOPEN_SOURCE=600 instead, as these test source files use an XSI function as well as POSIX functionality.

That's the short-term fix. The better fix would be to set _XOPEN_SOURCE globally in the HTSlib source code (as in the xopen_source branch previously mentioned), but that would be something probably best done early in a development cycle.

clausecker commented 1 year ago

It's a warning, not an error. Hence when these files are compiled via (HTSlib's build system's) make test the diagnostic messages scroll past quickly and are easily missed. (And the tests go on to run successfully anyway.)

Strictly speaking, calling an undeclared function causes behaviour to be undefined ever since C99. It's just accepted as a legacy feature by recent compilers. You should definitely fix this.

jkbonfield commented 1 year ago

I tried make check, with clang and opts above (bar the /usr/local/include bit as I'm only using official packages) and see no warnings.

However this was an htscodecs build. I'm not sure we're talking about the same thing here (eg @jmarshall's comments above). Htscodecs when built from htslib has a completely different Makefile and build system, so maybe this is infact an htslib bug rather than htscodecs one? I haven't yet tested that scenario (it's soooo slow on this laptop due to inability to use kvm qemu driver and no permission to install VM tools, sigh). I'll start it going...

clausecker commented 1 year ago

@jkbonfield Yes, the is an htslibs issue. I don't know why the issue was moved to this repository.

jkbonfield commented 1 year ago

Incorrect assumptions :)

I'll move it back. Thanks for clarifying

jmarshall commented 1 year ago

It was transferred here because it appeared to be an htscodecs issue (the *.c source files come from htscodecs), but the problem turns out to be in HTSlib's build rules for the htscodecs object files. It would have been more obvious that this was the case if the OP had included the exact compilation lines from the make output as well as the diagnostics. But we got there pretty quickly anyway.


Strictly speaking, calling an undeclared function causes behaviour to be undefined ever since C99. […] You should definitely fix this.

Indeed. Feel free to read the rest of the comment you're replying to. The point, as it whizzes by…

jkbonfield commented 1 year ago

There's already a commentd out line to force XOPEN_SOURCE for building, which I added mainly for compatibility testing. https://github.com/samtools/htslib/blob/develop/Makefile#L37

I agree this ought to be more mainstream. I haven't yet looked at your branch, but think this is worth doing as it is basically a requirement and until now it's just been unnecessary to make this explicit.

jkbonfield commented 1 year ago

I've tried with clang 14.0.5 on freebsd 13.2-RELEASE with the compiler options you mention and cannot reproduce this warning.

Edit: scratch that - forgot the make check bit. Well now I can reproduce, so should be easy to test fixes. Thanks