Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improved detection of POSIX #489

Merged
merged 21 commits into from
Dec 21, 2016
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ version: 1.0.{build}
environment:
matrix:
- COMPILER: "gcc"
MAKE_PARAMS: "test"
MAKE_PARAMS: '"make test && make lib && make -C tests fullbench-dll fullbench-lib"'
PLATFORM: "mingw64"
- COMPILER: "gcc"
MAKE_PARAMS: "test"
MAKE_PARAMS: "make test"
PLATFORM: "mingw32"
- COMPILER: "visual"
CONFIGURATION: "Debug"
Expand Down Expand Up @@ -62,8 +62,8 @@ build_script:
ECHO *** &&
make -v &&
cc -v &&
ECHO make %MAKE_PARAMS% &&
make %MAKE_PARAMS%
ECHO %MAKE_PARAMS% &&
sh -c %MAKE_PARAMS%
)
- if [%COMPILER%]==[gcc] if [%PLATFORM%]==[mingw64] (
COPY programs\zstd.exe bin\zstd.exe &&
Expand Down
2 changes: 2 additions & 0 deletions build/VS2010/fullbench-dll/fullbench-dll.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
<GenerateDebugInformation>true</GenerateDebugInformation>
<AdditionalLibraryDirectories>$(SolutionDir)bin\$(Platform)_$(Configuration);%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
<AdditionalDependencies>libzstd.lib;%(AdditionalDependencies)</AdditionalDependencies>
<ImageHasSafeExceptionHandlers>false</ImageHasSafeExceptionHandlers>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
Expand Down Expand Up @@ -138,6 +139,7 @@
<OptimizeReferences>true</OptimizeReferences>
<AdditionalLibraryDirectories>$(SolutionDir)bin\$(Platform)_$(Configuration);%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
<AdditionalDependencies>libzstd.lib;%(AdditionalDependencies)</AdditionalDependencies>
<ImageHasSafeExceptionHandlers>false</ImageHasSafeExceptionHandlers>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
Expand Down
2 changes: 2 additions & 0 deletions lib/dll/example/fullbench-dll.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
<GenerateDebugInformation>true</GenerateDebugInformation>
<AdditionalLibraryDirectories>$(SolutionDir)..\dll;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
<AdditionalDependencies>libzstd.lib;%(AdditionalDependencies)</AdditionalDependencies>
<ImageHasSafeExceptionHandlers>false</ImageHasSafeExceptionHandlers>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
Expand Down Expand Up @@ -141,6 +142,7 @@
<OptimizeReferences>true</OptimizeReferences>
<AdditionalLibraryDirectories>$(SolutionDir)..\dll;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
<AdditionalDependencies>libzstd.lib;%(AdditionalDependencies)</AdditionalDependencies>
<ImageHasSafeExceptionHandlers>false</ImageHasSafeExceptionHandlers>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
Expand Down
13 changes: 8 additions & 5 deletions lib/zstd.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@ extern "C" {

/* ===== ZSTDLIB_API : control library symbols visibility ===== */
#if defined(__GNUC__) && (__GNUC__ >= 4)
# define ZSTDLIB_API __attribute__ ((visibility ("default")))
#elif defined(ZSTD_DLL_EXPORT) && (ZSTD_DLL_EXPORT==1)
# define ZSTDLIB_API __declspec(dllexport)
# define ZSTDLIB_VISIBILITY __attribute__ ((visibility ("default")))
#else
# define ZSTDLIB_VISIBILITY
#endif
#if defined(ZSTD_DLL_EXPORT) && (ZSTD_DLL_EXPORT==1)
# define ZSTDLIB_API __declspec(dllexport) ZSTDLIB_VISIBILITY
#elif defined(ZSTD_DLL_IMPORT) && (ZSTD_DLL_IMPORT==1)
# define ZSTDLIB_API __declspec(dllimport) /* It isn't required but allows to generate better code, saving a function pointer load from the IAT and an indirect jump.*/
# define ZSTDLIB_API __declspec(dllimport) ZSTDLIB_VISIBILITY /* It isn't required but allows to generate better code, saving a function pointer load from the IAT and an indirect jump.*/
#else
# define ZSTDLIB_API
# define ZSTDLIB_API ZSTDLIB_VISIBILITY
Copy link
Contributor

@Cyan4973 Cyan4973 Dec 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of ZSTDLIB_VISIBILITY ?
Why does it help ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ZSTDLIB_VISIBILITY is only a helper macro. MinGW needs both __attribute__((visibility("default"))) and __declspec(dll{import, export}).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nick is right.
It solves a bug with __attribute__ ((visibility ("default"))) and broken DLL compilation with gcc/MinGW

#endif


Expand Down
3 changes: 2 additions & 1 deletion programs/bench.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
/* *************************************
* Includes
***************************************/
#include "util.h" /* Compiler options, UTIL_GetFileSize, UTIL_sleep */
#include "platform.h" /* Compiler options */
#include "util.h" /* UTIL_GetFileSize, UTIL_sleep */
#include <stdlib.h> /* malloc, free */
#include <string.h> /* memset */
#include <stdio.h> /* fprintf, fopen, ftello64 */
Expand Down
3 changes: 2 additions & 1 deletion programs/dibio.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
/*-*************************************
* Includes
***************************************/
#include "util.h" /* Compiler options, UTIL_GetFileSize, UTIL_getTotalFileSize */
#include "platform.h" /* Compiler options */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since platform.h is already included at top of util.h, is there any benefit at including it again in each user source file of util.h ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It cleary shows what we need platform.h for.

#include "util.h" /* UTIL_GetFileSize, UTIL_getTotalFileSize */
#include <stdlib.h> /* malloc, free */
#include <string.h> /* memset */
#include <stdio.h> /* fprintf, fopen, ftello64 */
Expand Down
3 changes: 2 additions & 1 deletion programs/fileio.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
/*-*************************************
* Includes
***************************************/
#include "util.h" /* Compiler options, UTIL_GetFileSize, _LARGEFILE64_SOURCE */
#include "platform.h" /* Compiler options */
#include "util.h" /* UTIL_GetFileSize, _LARGEFILE64_SOURCE */
#include <stdio.h> /* fprintf, fopen, fread, _fileno, stdin, stdout */
#include <stdlib.h> /* malloc, free */
#include <string.h> /* strcmp, strlen */
Expand Down
80 changes: 80 additions & 0 deletions programs/platform.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* Copyright (c) 2016-present, Przemyslaw Skibinski, Yann Collet, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

#ifndef PLATFORM_H_MODULE
#define PLATFORM_H_MODULE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you define (in a comment section) what's the objective of this file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I divided util.h into two files util.h and platform.h. Now we can use compiler macros (with POSIX macros) without functions declared in util.h. It's already used in datagen.
More was described here:
https://app.asana.com/0/91843333395082/231365028765940


#if defined (__cplusplus)
extern "C" {
#endif

/* **************************************
* Compiler Options
****************************************/
#if defined(__INTEL_COMPILER)
# pragma warning(disable : 177) /* disable: message #177: function was declared but never referenced */
Copy link
Contributor

@Cyan4973 Cyan4973 Dec 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, here is one typical issue with defining such compilation flag within platform.h :
now, non-referenced function warning is disabled across a large portion of source code (at least, any source which includes directly or indirectly platform.h).

That could be a good idea if there was some functions within platform.h that could uselessly trigger it, but that's not the case here : I don't see any function declaration in platform.h, only macros, so the flag is enabled on behalf of future users, such as, maybe, util.h.

But now, a user which only includes platform.h has a useful warning flag disabled. This flag could help him detect bugs before they get more complex to understand.

Disabling a warning flag can be completely legitimate, but it's better done close to, and even limited to, the section where it's needed, and better avoided as a "global setting".

If the point here is to disable it for util.h, better do it within util.h (and even better, re-enable it at the end of the file). I suspect it's related to UTIL_STATIC, in which case this is the better place to define it.

Copy link
Contributor Author

@inikep inikep Dec 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to util.h, next to a definition of UTIL_STATIC.

What about:
# pragma warning(disable : 4127)
Do you prefer to declare it in every .c file that needs it?

Copy link
Contributor

@Cyan4973 Cyan4973 Dec 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Disabling compilation flag should be done on a scope as minimal as possible.

#endif
#if defined(_MSC_VER)
# define _CRT_SECURE_NO_WARNINGS /* Disable some Visual warning messages for fopen, strncpy */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

# define _CRT_SECURE_NO_DEPRECATE /* VS2005 */
# pragma warning(disable : 4127) /* disable: C4127: conditional expression is constant */
# if _MSC_VER <= 1800 /* (1800 = Visual Studio 2013) */
# define snprintf sprintf_s /* snprintf unsupported by Visual <= 2013 */
# endif
#endif


/* **************************************
* Unix Large Files support (>4GB)
****************************************/
#if !defined(__LP64__) /* No point defining Large file for 64 bit */
# define _FILE_OFFSET_BITS 64 /* turn off_t into a 64-bit type for ftello, fseeko */
# if defined(__sun__) && !defined(_LARGEFILE_SOURCE) /* Sun Solaris 32-bits requires specific definitions */
# define _LARGEFILE_SOURCE /* fseeko, ftello */
# elif !defined(_LARGEFILE64_SOURCE)
# define _LARGEFILE64_SOURCE /* off64_t, fseeko64, ftello64 */
# endif
#endif


/* ************************************************************
* Detect POSIX version
* PLATFORM_POSIX_VERSION = -1 for non-Unix e.g. Windows
* PLATFORM_POSIX_VERSION = 0 for Unix-like non-POSIX
* PLATFORM_POSIX_VERSION >= 1 is equal to found _POSIX_VERSION
***************************************************************/
#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__)) || defined(__midipix__) || defined(__VMS))
/* UNIX-style OS. ------------------------------------------- */
# if (defined(__APPLE__) && defined(__MACH__)) || defined(__SVR4) || defined(_AIX) || defined(__hpux) \
|| defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) /* POSIX.1–2001 (SUSv3) conformant */
# define PLATFORM_POSIX_VERSION 200112L
# else
# if defined(__linux__) || defined(__linux)
# define _POSIX_C_SOURCE 200112L /* use feature test macro */
# endif
# include <unistd.h> /* declares _POSIX_VERSION */
# if defined(_POSIX_VERSION) /* POSIX compliant */
# define PLATFORM_POSIX_VERSION _POSIX_VERSION
# else
# define PLATFORM_POSIX_VERSION 0
# endif
# endif
#endif

#if !defined(PLATFORM_POSIX_VERSION)
# define PLATFORM_POSIX_VERSION -1
#endif



#if defined (__cplusplus)
}
#endif

#endif /* PLATFORM_H_MODULE */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the file makes sense :
it deals with system-specific adaptations, which has more to do with OS and / or compiler restrictions and abilities.

38 changes: 5 additions & 33 deletions programs/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,12 @@
extern "C" {
#endif

/* **************************************
* Compiler Options
****************************************/
#if defined(__INTEL_COMPILER)
# pragma warning(disable : 177) /* disable: message #177: function was declared but never referenced */
#endif
#if defined(_MSC_VER)
# define _CRT_SECURE_NO_WARNINGS /* Disable some Visual warning messages for fopen, strncpy */
# define _CRT_SECURE_NO_DEPRECATE /* VS2005 */
# pragma warning(disable : 4127) /* disable: C4127: conditional expression is constant */
#if _MSC_VER <= 1800 /* (1800 = Visual Studio 2013) */
# define snprintf sprintf_s /* snprintf unsupported by Visual <= 2013 */
#endif
#endif


/* Unix Large Files support (>4GB) */
#if !defined(__LP64__) /* No point defining Large file for 64 bit */
# define _FILE_OFFSET_BITS 64 /* turn off_t into a 64-bit type for ftello, fseeko */
# if defined(__sun__) && !defined(_LARGEFILE_SOURCE) /* Sun Solaris 32-bits requires specific definitions */
# define _LARGEFILE_SOURCE /* fseeko, ftello */
# elif !defined(_LARGEFILE64_SOURCE)
# define _LARGEFILE64_SOURCE /* off64_t, fseeko64, ftello64 */
# endif
#endif


/*-****************************************
* Dependencies
******************************************/
#include <stdlib.h> /* features.h with _POSIX_C_SOURCE, malloc */
#include "platform.h" /* Compiler options, PLATFORM_POSIX_VERSION */
#include <stdlib.h> /* malloc */
#include <stdio.h> /* fprintf */
#include <sys/types.h> /* stat, utime */
#include <sys/stat.h> /* stat */
Expand Down Expand Up @@ -88,7 +63,7 @@ extern "C" {
# define SET_HIGH_PRIORITY SetPriorityClass(GetCurrentProcess(), REALTIME_PRIORITY_CLASS)
# define UTIL_sleep(s) Sleep(1000*s)
# define UTIL_sleepMilli(milli) Sleep(milli)
#elif (defined(__unix__) || defined(__unix) || defined(__VMS) || defined(__midipix__) || (defined(__APPLE__) && defined(__MACH__)))
#elif PLATFORM_POSIX_VERSION >= 0 /* Unix-like operating system */
# include <unistd.h>
# include <sys/resource.h> /* setpriority */
# include <time.h> /* clock_t, nanosleep, clock, CLOCKS_PER_SEC */
Expand All @@ -98,7 +73,7 @@ extern "C" {
# define SET_HIGH_PRIORITY /* disabled */
# endif
# define UTIL_sleep(s) sleep(s)
# if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) || (defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE >= 199309L))
# if (defined(__linux__) && (PLATFORM_POSIX_VERSION >= 199309L)) || (PLATFORM_POSIX_VERSION >= 200112L) /* nanosleep requires POSIX.1-2001 */
# define UTIL_sleepMilli(milli) { struct timespec t; t.tv_sec=0; t.tv_nsec=milli*1000000ULL; nanosleep(&t, NULL); }
# else
# define UTIL_sleepMilli(milli) /* disabled */
Expand Down Expand Up @@ -325,10 +300,7 @@ UTIL_STATIC int UTIL_prepareFileList(const char *dirName, char** bufStart, size_
return nbFiles;
}

#elif (defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE >= 200112L)) || \
(defined(__APPLE__) && defined(__MACH__)) || defined(__SVR4) || defined(_AIX) || defined(__hpux) || \
defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) || \
(defined(__linux__) && defined(_POSIX_C_SOURCE)) /* opendir */
#elif defined(__linux__) || (PLATFORM_POSIX_VERSION >= 200112L) /* opendir, readdir require POSIX.1-2001 */
# define UTIL_HAS_CREATEFILELIST
# include <dirent.h> /* opendir, readdir */

Expand Down
15 changes: 12 additions & 3 deletions programs/zstdcli.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
/*-************************************
* Dependencies
**************************************/
#include "util.h" /* Compiler options, UTIL_HAS_CREATEFILELIST */
#include "platform.h" /* Compiler options, PLATFORM_POSIX_VERSION */
#include "util.h" /* UTIL_HAS_CREATEFILELIST, UTIL_createFileList */
#include <string.h> /* strcmp, strlen */
#include <errno.h> /* errno */
#include "fileio.h"
Expand All @@ -43,8 +44,7 @@
#if defined(MSDOS) || defined(OS2) || defined(WIN32) || defined(_WIN32) || defined(__CYGWIN__)
# include <io.h> /* _isatty */
# define IS_CONSOLE(stdStream) _isatty(_fileno(stdStream))
#elif defined(_POSIX_C_SOURCE) || defined(_XOPEN_SOURCE) || defined(_POSIX_SOURCE) || (defined(__APPLE__) && defined(__MACH__)) || \
defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) /* https://sourceforge.net/p/predef/wiki/OperatingSystems/ */
#elif (defined(__linux__) && (PLATFORM_POSIX_VERSION >= 1)) || (PLATFORM_POSIX_VERSION >= 200112L) /* isatty requires POSIX.1-2001 */
# include <unistd.h> /* isatty */
# define IS_CONSOLE(stdStream) isatty(fileno(stdStream))
#else
Expand Down Expand Up @@ -470,6 +470,15 @@ int main(int argCount, const char* argv[])

/* Welcome message (if verbose) */
DISPLAYLEVEL(3, WELCOME_MESSAGE);
#ifdef _POSIX_C_SOURCE
DISPLAYLEVEL(4, "_POSIX_C_SOURCE defined: %ldL\n", (long) _POSIX_C_SOURCE);
#endif
#ifdef _POSIX_VERSION
DISPLAYLEVEL(4, "_POSIX_VERSION defined: %ldL\n", (long) _POSIX_VERSION);
#endif
#ifdef PLATFORM_POSIX_VERSION
DISPLAYLEVEL(4, "PLATFORM_POSIX_VERSION defined: %ldL\n", (long) PLATFORM_POSIX_VERSION);
#endif

#ifdef UTIL_HAS_CREATEFILELIST
if (recursive) { /* at this stage, filenameTable is a list of paths, which can contain both files and directories */
Expand Down
3 changes: 2 additions & 1 deletion tests/datagencli.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@
/*-************************************
* Dependencies
**************************************/
#include "util.h" /* Compiler options */
#include "platform.h" /* Compiler options */
#include <stdio.h> /* fprintf, stderr */
#include "datagen.h" /* RDG_generate */
#include "mem.h" /* U32, U64 */


/*-************************************
Expand Down
3 changes: 2 additions & 1 deletion tests/fullbench.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
/*_************************************
* Includes
**************************************/
#include "util.h" /* Compiler options, UTIL_GetFileSize */
#include "platform.h" /* Compiler options */
#include "util.h" /* UTIL_GetFileSize */
#include <stdlib.h> /* malloc */
#include <stdio.h> /* fprintf, fopen, ftello64 */
#include <time.h> /* clock_t, clock, CLOCKS_PER_SEC */
Expand Down
3 changes: 2 additions & 1 deletion tests/paramgrill.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
/*-************************************
* Dependencies
**************************************/
#include "util.h" /* Compiler options, UTIL_GetFileSize */
#include "platform.h" /* Compiler options */
#include "util.h" /* UTIL_GetFileSize */
#include <stdlib.h> /* malloc */
#include <stdio.h> /* fprintf, fopen, ftello64 */
#include <string.h> /* strcmp */
Expand Down
3 changes: 2 additions & 1 deletion zlibWrapper/examples/zwrapbench.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
/* *************************************
* Includes
***************************************/
#include "util.h" /* Compiler options, UTIL_GetFileSize, UTIL_sleep */
#include "platform.h" /* Compiler options */
#include "util.h" /* UTIL_GetFileSize, UTIL_sleep */
#include <stdlib.h> /* malloc, free */
#include <string.h> /* memset */
#include <stdio.h> /* fprintf, fopen, ftello64 */
Expand Down