myrao / libyuv

Automatically exported from code.google.com/p/libyuv
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

libyuv's inline assembly ifdefs are incompatible with clang on Windows #341

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Cloning from Chromium tracker: http://crbug.com/391927

-----

There are three pairs files in libyuv that have #ifdefs patterns designed to 
compile with Clang/GCC on posix and MSVC on Windows:
scale_posix.cc / scale_win.cc
compare_posix.cc / compare_win.cc
row_posix.cc / row_win.cc

The _win.cc variant looks for "#if !defined(LIBYUV_DISABLE_X86) && 
defined(_MSC_VER)" and the _posix.cc variant looks for "#if 
!defined(LIBYUV_DISABLE_X86) && (defined(__x86_64__) || defined(__i386__))".

The trouble is that Clang on Windows defines both of those macros, so it will 
compile the bodies of both the _win.cc and _posix.cc variants, which ultimately 
will cause link errors.

What solution would work best for libyuv?  Long term, my preference is to 
compile the _win.cc variant, even though Clang today cannot compile it: 
http://llvm.org/PR20023

Original issue reported on code.google.com by rnk@chromium.org on 7 Jul 2014 at 11:44

GoogleCodeExporter commented 8 years ago
In the chrome bug you say this occurs on r1025.  Thanks.
_MSC_VER is defined for clang, as well as Visual C.

So I guess the solution is to change defined(_MSC_VER) to !defined(__clang__) 
depending on context.

I can probably do the change blindly, but if you could tell me how to get 
and/or build with clang that would help.
The version of clang I have on Windows is old, and is literally only the C 
compiler, not C++.  And I dont have build files for clang on Windows.

Original comment by fbarch...@chromium.org on 8 Jul 2014 at 2:51

GoogleCodeExporter commented 8 years ago
In Chromium we support GYP_DEFINES=clang=1 these days, but if you just want to 
test things with libyuv directly, you can get prebuilt clang Windows binaries 
from http://llvm.org/builds/.

You can actually select it as a platform toolset in VS.  Unfortunately, I know 
that neither the _posix.cc or _win.cc files will build right now due both of 
these bugs:
http://llvm.org/PR20023 - msvc version
http://llvm.org/PR20201 - posix version

If we fix the posix version in Clang, then we will fail to link libyuv, which 
is worse for us than failing to compile, because we cannot continue the build 
by falling back to MSVC.

Original comment by rnk@google.com on 8 Jul 2014 at 7:01

GoogleCodeExporter commented 8 years ago
I have hit the exact same issue in Firefox.  __i386__ and __x86_64__ are gcc 
only macros which clang-should not define.  http://reviews.llvm.org/D4415 fixes 
that.

Original comment by ehsan.ak...@gmail.com on 8 Jul 2014 at 9:37

GoogleCodeExporter commented 8 years ago
installed a version 3.4 clang
Files all compile, except rotate, so I'll start with that:

d:\src\libyuv\trunk>clang -c -Iinclude source/rotate.c
In file included from source/rotate.c:11:
source/./rotate.cc:391:8: error: expected identifier or '('
extern "C" void TransposeUVWx8_SSE2(const uint8* src, int src_stride,
       ^
source/./rotate.cc:1017:22: error: use of undeclared identifier 
'TransposeUVWx8_SSE2'
    TransposeUVWx8 = TransposeUVWx8_SSE2;
                     ^
2 errors generated.

Original comment by fbarch...@chromium.org on 8 Jul 2014 at 9:41

GoogleCodeExporter commented 8 years ago
r1031 fixes rotate.cc
My test was simple compiles, not links or running.
clang -c -Iinclude source/compare.c            
clang -c -Iinclude source/compare_common.c     
clang -c -Iinclude source/compare_neon.c       
clang -c -Iinclude source/compare_posix.c      
clang -c -Iinclude source/compare_win.c        
clang -c -Iinclude source/convert.c            
clang -c -Iinclude source/convert_argb.c       
clang -c -Iinclude source/convert_from.c       
clang -c -Iinclude source/convert_from_argb.c  
clang -c -Iinclude source/convert_jpeg.c       
clang -c -Iinclude source/convert_to_argb.c    
clang -c -Iinclude source/convert_to_i420.c    
clang -c -Iinclude source/cpu_id.c             
clang -c -Iinclude source/format_conversion.c  
clang -c -Iinclude source/mjpeg_decoder.c      
clang -c -Iinclude source/mjpeg_validate.c     
clang -c -Iinclude source/planar_functions.c   
clang -c -Iinclude source/rotate.c             
clang -c -Iinclude source/rotate_argb.c        
clang -c -Iinclude source/rotate_mips.c        
clang -c -Iinclude source/rotate_neon.c        
clang -c -Iinclude source/row_any.c            
clang -c -Iinclude source/row_common.c         
clang -c -Iinclude source/row_mips.c           
clang -c -Iinclude source/row_neon.c           
clang -c -Iinclude source/row_posix.c          
clang -c -Iinclude source/row_win.c            
clang -c -Iinclude source/scale.c              
clang -c -Iinclude source/scale_argb.c         
clang -c -Iinclude source/scale_common.c       
clang -c -Iinclude source/scale_mips.c         
clang -c -Iinclude source/scale_neon.c         
clang -c -Iinclude source/scale_posix.c        
clang -c -Iinclude source/scale_win.c          
clang -c -Iinclude source/video_common.c     

Is this correct for clang on windows?

set GYP_DEFINES="target_arch=ia32 clang=1"
call python gyp_libyuv -fninja libyuv_test.gyp
ninja -C out\Release

Original comment by fbarch...@chromium.org on 8 Jul 2014 at 10:42

GoogleCodeExporter commented 8 years ago
Some documentation on pragmas
http://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas

clang-cl has errors.  e.g.
clang-cl -Wno-deprecated-declarations  -c -Iinclude source/cpu_id.c

Original comment by fbarch...@chromium.org on 9 Jul 2014 at 1:13

GoogleCodeExporter commented 8 years ago
All files build with clang-cl.
Followup needed to re-enable assembly.

Original comment by fbarch...@chromium.org on 9 Jul 2014 at 5:21

GoogleCodeExporter commented 8 years ago
Fixed in r1033

Compiler available here
http://llvm.org/builds/downloads/LLVM-3.5.r210650-win32.exe

Instead of 
cl /c /Iinclude source/scale.c

clang-cl -Wno-deprecated-declarations -c -Iinclude source/scale.c

Original comment by fbarch...@chromium.org on 12 Jul 2014 at 1:24