Skip to content

fix clang-cl _tzcnt_u64 not defined issue#1017

Merged
Cyan4973 merged 1 commit intolz4:devfrom
fanzeyi:fix-clang-cl-msvc
Aug 9, 2021
Merged

fix clang-cl _tzcnt_u64 not defined issue#1017
Cyan4973 merged 1 commit intolz4:devfrom
fanzeyi:fix-clang-cl-msvc

Conversation

@fanzeyi
Copy link
Copy Markdown
Contributor

@fanzeyi fanzeyi commented Aug 6, 2021

When building with Clang-cl on Windows, _tzcnt_u64 was not properly defined until https://bugs.llvm.org/show_bug.cgi?id=30506 was fixed.

This PR works around the issue by directly using the builtin intrinsic when it is building with Clang on Windows.

Test Plan:

To build lz4 with clang-cl on Windows (Before):

$ & 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Auxiliary\Build\vcvarsall.bat' x64
$ $Env:CC="C:\tools\toolchains\LLVM9.0.1\bin\clang-cl.exe"
$ make
make[1]: Entering directory 'C:/Users/zeyi/GitHub/lz4/lib'
compiling static library
clang-cl: warning: argument unused during compilation: '-O3' [-Wunused-command-line-argument]
lz4.c(523,30): warning: implicit declaration of function '_tzcnt_u64' is invalid in C99 [-Wimplicit-function-declaration]
            return (unsigned)_tzcnt_u64(val) >> 3;
                             ^
1 warning generated.
In file included from lz4hc.c:66:
./lz4.c(523,30): warning: implicit declaration of function '_tzcnt_u64' is invalid in C99 [-Wimplicit-function-declaration]
            return (unsigned)_tzcnt_u64(val) >> 3;
                             ^
1 warning generated.
creating library resource
/bin/sh: gcc: command not found
windres: preprocessing failed.
make[1]: *** [Makefile:112: liblz4-dll.o] Error 1
make[1]: Leaving directory 'C:/Users/zeyi/GitHub/lz4/lib'
make: *** [Makefile:57: lib-release] Error 2

With changes in this PR applied:

$ make
make[1]: Entering directory 'C:/Users/zeyi/GitHub/lz4/lib'
compiling static library
clang-cl: warning: argument unused during compilation: '-O3' [-Wunused-command-line-argument]
/bin/sh: gcc: command not found
windres: preprocessing failed.
make[1]: *** [Makefile:112: liblz4-dll.o] Error 1
make[1]: Leaving directory 'C:/Users/zeyi/GitHub/lz4/lib'
make: *** [Makefile:57: lib-release] Error 2

(Also a lib/Makefile change is required to make this actually work: changing .o to .obj)

Unfortunately I can't get the rest of the build working due to windres issues so I can't really run the test suite. :(

Cyan4973
Cyan4973 reviewed Aug 6, 2021
View reviewed changes
Comment thread lib/lz4.c Outdated
@Cyan4973
Copy link
Copy Markdown
Member

Cyan4973 commented Aug 6, 2021

Thanks for the suggestion @fanzeyi .
I think it's a nice one worth integrating.

It also makes me wonder if we should have a test dedicated for this scenario (compiling with Visual Studio using the llvm generator back-end) in order to catch this issue, and similar ones in the future.

@t-mat
Copy link
Copy Markdown
Contributor

t-mat commented Aug 7, 2021

I'm sure this PR improves compatibility.

But when I use MSVC 2019 (Version 16.10) with its clang-cl (version 11.0.0), this error doesn't occur.

I also confirmed that recent version of MSVC has immintrin.h which contains r374516.

// C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\Llvm\lib\clang\11.0.0\include\immintrin.h
#ifndef __IMMINTRIN_H
#define __IMMINTRIN_H
...
/* No feature check desired due to internal checks */
#include <bmiintrin.h>
...
#endif

I have no idea when they fixed this error. But at least when (__clang_major__ >= 11) is true, this issue doesn't occur.

@t-mat
Copy link
Copy Markdown
Contributor

t-mat commented Aug 7, 2021

It also makes me wonder if we should have a test dedicated for this scenario (compiling with Visual Studio using the llvm generator back-end) in order to catch this issue, and similar ones in the future.

I think it's great.

But I don't know how we can install multiple/specific version of clang-cl and their libraries without breaking MSVC environment.
Especially for older version of clang-cl. Initial version of clang-cl for MSVC2019 was clang-8 but recent version of MSVC2019 installs clang-11.

The problem is they don't have major version number. They just call it clang-cl. And since clang-cl translates cl.exe command line switches to clang's, it seems version of MSVC, its standard library, cl.exe and clang-cl are tightly binded.

Anyway, I think many MSVC user starts to use clang-cl recently, it's great to support it.

@Cyan4973
Copy link
Copy Markdown
Member

Cyan4973 commented Aug 8, 2021

Considering the current test situation, I wouldn't go as far as trying to have multiple versions of clang with each version of MS Visual.
Just having one test setup using clang on Visual Studio would already be an improvement.

I get it that it wouldn't necessarily catch the issue reported in this PR.
So this is likely an orthogonal topic.

@fanzeyi
Copy link
Copy Markdown
Contributor Author

fanzeyi commented Aug 9, 2021 •
edited
Loading

We are seeing this issue on Windows with LLVM 9.0.1 and my understanding is that MSVC toolchain does not participate here. The intrin.h header is provided by clang and LLVM. I never tested the clang bundled with MSVC though.

The LLVM bug was fixed in https://reviews.llvm.org/rG282eff38477ebf665f88f52411edd591067af883 and it seems to be included in LLVM 10.0.0 and after. We can have a guard like (__clang_major__ >= 10) then.

I've updated the PR to have nested macro guards. I wasn't able to find examples of other nested guards so the code style might be incorrect. Let me know what style you prefer.

@fanzeyi fanzeyi force-pushed the fix-clang-cl-msvc branch from 54b10a1 to 1a2e8f4 Compare August 9, 2021 18:30
@Cyan4973 Cyan4973 merged commit e78eeec into lz4:dev Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fanzeyi @Cyan4973 @t-mat