zstd is compressing symbolic links with `std=c99`

[lzutao]

@Cyan4973 Hmm, I might find a test error that zstd have been
compressed the symbolic link.

_Originally posted by @lzutao in https://github.com/facebook/zstd/issues/1605#issuecomment-487978531_

[terrelln]

This is a configuration issue with meson where zstd is built without being able to detect symlinks.

We use the function UTIL_isLink(). When I build zstd with meson I found that __STRICT_ANSI__ was defined.

Somewhere in the meson build, the flag -std=c99 (or some other C-version) must be being used. If you switch that to -std=gnu99 the build should work. Alternatively, we need a way to get the lstat() declaration (and definition) when __STRICT_ANSI__ is defined.

[lzutao]

@terrelln In https://linux.die.net/man/2/lstat and http://man7.org/linux/man-pages/man2/stat.2.html,
I don't see lstat requires __STRICT_ANSI__ defined though.

[terrelln]

See https://github.com/facebook/zstd/pull/1347, when __STRICT_ANSI__ is defined some systems won't expose lstat(). I know that it isn't specified in the header, but it failed in practice. See the failing build here.

Curiously, when I removed the macro guard, the build no longer failed. I suspect that the CircleCI image was updated.

[lzutao]

This may not be relevant but I get this warning when building
with make c99build on CentOS 5 (gcc (GCC) 5.5.0)

gcc -I../lib -I../lib/common -I../lib/compress -I../lib/dictBuilder -I../lib/deprecated -I../programs -std=c99 -Werror -Wall -Wextra -Wcast-qual -Wcast-align -Wshadow -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement -Wstrict-prototypes -Wundef -Wvla -Wformat=2 -Winit-self -Wfloat-equal -Wwrite-strings -Wredundant-decls -Wmissing-prototypes -g -DDEBUGLEVEL=1   -DZSTD_MULTITHREAD -pthread ../programs/util.c ../programs/timefn.c poolTests.c ../lib/common/pool.c ../lib/common/threading.c ../lib/common/zstd_common.c ../lib/common/error_private.c -o poolTests
In file included from poolTests.c:14:0:
poolTests.c: In function 'waitFn':
../programs/util.h:66:99: error: implicit declaration of function 'nanosleep' [-Werror=implicit-function-declaration]
 #      define UTIL_sleepMilli(milli) { struct timespec t; t.tv_sec=0; t.tv_nsec=milli*1000000ULL; nanosleep(&t, NULL); }
                                                                                                   ^
poolTests.c:71:3: note: in expansion of macro 'UTIL_sleepMilli'
   UTIL_sleepMilli(1);
   ^
cc1: all warnings being treated as errors

So zstd is being tested with std=gnu99 by default?
I think I should change to the same std version in meson
build file to avoid error when using run meson test.

[michaelforney]

I ran into this issue as well. I think the feature-test macros in https://github.com/facebook/zstd/blob/21bb78e90806f4a3b5dad2d59d46e951bcb13165/programs/util.c#L110-L117 aren't being used correctly. Feature-test macros should be defined by the application to tell the libc which declarations to expose. Since zstd doesn't define any of these macros anywhere (apart from _POSIX_C_SOURCE in programs/platform.h), it doesn't make any sense to test for them.

I also don't think __STRICT_ANSI__ is relevant here. This just means that the compiler doesn't support GNU C extensions (which is the case with -std=c99), and prevents libc headers from using these features. However, -std=c99 will also disable the default set of feature-test macros defined by the compiler, which is probably the reason that lstat couldn't be found in #1347.

I see in other locations, the macro PLATFORM_POSIX_VERSION is used for similar purposes, so maybe it could be used here as well:

diff --git a/programs/util.c b/programs/util.c
index 7b827d45..6190bca5 100644
--- a/programs/util.c
+++ b/programs/util.c
@@ -107,19 +107,11 @@ int UTIL_isSameFile(const char* file1, const char* file2)
 U32 UTIL_isLink(const char* infilename)
 {
 /* macro guards, as defined in : https://linux.die.net/man/2/lstat */
-#ifndef __STRICT_ANSI__
-#if defined(_BSD_SOURCE) \
-    || (defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE >= 500)) \
-    || (defined(_XOPEN_SOURCE) && defined(_XOPEN_SOURCE_EXTENDED)) \
-    || (defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE >= 200112L)) \
-    || (defined(__APPLE__) && defined(__MACH__)) \
-    || defined(__OpenBSD__) \
-    || defined(__FreeBSD__)
+#if PLATFORM_POSIX_VERSION >= 200112L
     int r;
     stat_t statbuf;
     r = lstat(infilename, &statbuf);
     if (!r && S_ISLNK(statbuf.st_mode)) return 1;
-#endif
 #endif
     (void)infilename;
     return 0;

[cafarm]

Thanks @michaelforney! That resolved the issue for me.

[michaelforney]

Also, I'm not sure where the 200112L number in linux-manpages came from. POSIX says that lstat was first introduced in issue 1 (https://pubs.opengroup.org/onlinepubs/9699919799/functions/lstat.html#tag_16_174_11), and I don't see any conditional declaration in glibc's sys/stat.h.

So maybe the check should just be #if PLATFORM_POSIX_VERSION?