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

On WIN32, fix compile errors on Clang with MSVC headers #718

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rt39
Copy link

@Rt39 Rt39 commented Jul 9, 2024

@@ -21,6 +21,9 @@ if(FLAC__CPU_X86_64 OR FLAC__CPU_IA32)
if(WITH_AVX AND MSVC)
set_source_files_properties(fixed_intrin_avx2.c lpc_intrin_avx2.c stream_encoder_intrin_avx2.c lpc_intrin_fma.c PROPERTIES COMPILE_FLAGS /arch:AVX2)
endif()
if(WITH_AVX AND WIN32 AND NOT MSVC)
set_source_files_properties(fixed_intrin_avx2.c lpc_intrin_avx2.c stream_encoder_intrin_avx2.c lpc_intrin_fma.c PROPERTIES COMPILE_FLAGS -mavx2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why this is necessary. I regularly build with Clang on Win32 through MinGW, and that works perfectly fine without this patch.

So, either the version of Clang you're using is modified in some way, or there is something with the MSVC header files happening here.

In any case, the if() is too broad because it also applies to MinGW.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This still doesn't make any sense to me. Why would this only be necessary on Clang on Windows with MSVC headers? I'd rather understand this first. It seems like some logic is missing in https://github.com/xiph/flac/blob/master/src/libFLAC/include/private/cpu.h

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently this is a bug in ClangCL: llvm/llvm-project#53520

@@ -221,7 +221,7 @@ static char *posixly_correct;
/* Avoid depending on library functions or files
whose names are inconsistent. */

#ifndef getenv
#if !defined(getenv) && !(defined(__clang__) && defined(_MSC_VER))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too sure what this does, and why it doesn't work for your particular configuration. Maybe it can be removed completely? I think it catches some corner case that existed 30 years ago or something.

Copy link
Author

Choose a reason for hiding this comment

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

It seems on Windows, using Clang with MSVC headers would cause error: conflicting types for 'getenv'

X:\path\to\clang.exe -DHAVE_CONFIG_H -D_DARWIN_C_SOURCE -D_FORTIFY_SOURCE=2 -D_POSIX_PTHREAD_SEMANTICS -D_TANDEM_SOURCE -D__STDC_WANT_IEC_60559_BFP_EXT__ -D__STDC_WANT_IEC_60559_DFP_EXT__ -D__STDC_WANT_IEC_60559_FUNCS_EXT__ -D__STDC_WANT_IEC_60559_TYPES_EXT__ -D__STDC_WANT_LIB_EXT2__ -D__STDC_WANT_MATH_SPEC_FUNCS__ -IX:/path/to/flac/include -IX:/path/to/flac/build -Wall -Wextra -Wstrict-prototypes -Wmissing-prototypes -Waggregate-return -Wcast-align -Wnested-externs -Wshadow -Wundef -Wmissing-declarations -Winline  -DNDEBUG -O3 -DNDEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -Wdeclaration-after-statement -fstack-protector-strong -MD -MT src/share/getopt/CMakeFiles/getopt.dir/getopt.c.obj -MF src\share\getopt\CMakeFiles\getopt.dir\getopt.c.obj.d -o src/share/getopt/CMakeFiles/getopt.dir/getopt.c.obj -c X:/path/to/flac/src/share/getopt/getopt.c
X:/path/to/flac/src/share/getopt/getopt.c:225:14: error: conflicting types for 'getenv'
  225 | extern char *getenv (const char * name);
      |              ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\ucrt\stdlib.h:1184:28: note: previous declaration is here
 1184 |     _DCRTIMP char* __cdecl getenv(
      | 

Which is defined as

_Check_return_ _CRT_INSECURE_DEPRECATE(_dupenv_s)
_DCRTIMP char* __cdecl getenv(
    _In_z_ char const* _VarName
    );

in <stdlib.h>.

I think the best way to handle this error is to remove the extern declaration when it is clang and MSVC.

Copy link
Author

Choose a reason for hiding this comment

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

I think now it is only targeting when using Clang and MSVC headers, for MinGW headers contains unistd.h but MSVC ones does not. Adding cmake_policy is to suppress the warnings.

@Rt39 Rt39 changed the title On WIN32, fix compile errors on non MSVC compilers On WIN32, fix compile errors on Clang with MSVC headers Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants