8000 fix clang-cl _tzcnt_u64 not defined issue by fanzeyi · Pull Request #1017 · lz4/lz4 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix clang-cl _tzcnt_u64 not defined issue #1017

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 9, 2021
Merged

fix clang-cl _tzcnt_u64 not defined issue #1017

merged 1 commit into from
Aug 9, 2021

Conversation

fanzeyi
Copy link
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
@Cyan4973
Copy link
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
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
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
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
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.

@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
0