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

Support #pragma intrinsic and implement more builtins #20272

Closed
timurrrr opened this issue May 30, 2014 · 14 comments
Closed

Support #pragma intrinsic and implement more builtins #20272

timurrrr opened this issue May 30, 2014 · 14 comments
Labels
bugzilla Issues migrated from bugzilla c++

Comments

@timurrrr
Copy link
Contributor

Bugzilla Link 19898
Resolution FIXED
Resolved on Dec 02, 2016 15:00
Version trunk
OS Windows NT
Blocks #7187 #14079
CC @DougGregor,@ehsan,@ismail,@kcc,@nico,@rnk,@stbergmann

Extended Description

Repro (extracted from compiler-rt):

$ cat test.cpp
extern "C" void _ReadWriteBarrier();
#pragma intrinsic(_ReadWriteBarrier)

$ clang-cl -Wall -c test.cpp
test.cpp(2,9) : warning: unknown pragma ignored [-Wunknown-pragmas]
#pragma intrinsic(_ReadWriteBarrier)
^
1 warning generated.

@timurrrr
Copy link
Contributor Author

Another manifestation is that if I add "inline" like this:
extern "C" inline void _ReadWriteBarrier();
I get another warning too:
test.cpp(1,24) : warning: inline function '_ReadWriteBarrier' is not defined [-Wundefined-inline]

This warning is visible when doing "check-asan check-sa without the "-Wno-undefined-inline" flag.

@rnk
Copy link
Collaborator

rnk commented May 30, 2014

This would require completely changing our approach to MS intrinsics, which we've been avoiding for some time. We'd have to take everything in intrin.h and make them compiler builtins instead of static, alwaysinline, nodebug wrappers around our gcc-style builtins.

So far, we've been recommending that people include intrin.h, which is the MSDN documented way to get at these intrinsics.

@timurrrr
Copy link
Contributor Author

timurrrr commented Jul 9, 2014

The approach that yields a warning is used in lib/sanitizer_common/sanitizer_atomic_msvc.h. This header is included in a few places in the ASan RTL and is transitively included into lib/asan/asan_malloc_win.cc which defines its own malloc, free, etc. Ditto with lib/asan/asan_interceptors.cc and longjmp, lib/asan/asan_dll_thunk etc.
Including <intrin.h> brings in the "real" declaration of those function and we end up having conflicting declarations/definitions here and there.

I think we have a convention to avoid systen includes as much as possible in the ASan RTL. [Looping in Kostya]

Is the code like I wrote initially valid?
If yes -- we can just live with the warning disabled in CMake for a while.

@rnk
Copy link
Collaborator

rnk commented Jul 9, 2014

I'd disable the warning for now.

@ehsan
Copy link
Contributor

ehsan commented Sep 30, 2014

If we are not going to change our approach for MS intrinsics, perhaps we should just accept this pragma and don't do anything with it?

@rnk
Copy link
Collaborator

rnk commented Nov 25, 2014

This seems to be a common compatibility issue, so I think we should handle the pragma.

In Clang, #pragma intrinsic should check if the arguments are all recognized builtins. If they are not, we should emit a diagnostic telling the user that this intrinsic is not recognized as a compiler builtin and they should try including <intrin.h> to access the non-builtin intrinsics.

I don't think we need support for the counterpart, #pragma function, which demotes a builtin to a normal function.

We already have a short list of MSVC intrinsics implemented as builtins in Builtins.def. I think we should try to add most of the commonly used families of intrinsics (_Interlocked*) to this list. Anything that compiles down to inline assembly should IMO stay part of the intrin.h header, but if it's sufficiently useful that it can be an LLVM intrinsic, then I think we should thread it through that way.

@rnk
Copy link
Collaborator

rnk commented Mar 2, 2015

I hit this again in this way:

$ cat t.cpp
#include <windows.h>
int main() {
MemoryBarrier();
}

$ clang-cl t.cpp
t-6c0216.obj : error LNK2019: unresolved external symbol __faststorefence referenced in function main
t.exe : fatal error LNK1120: 1 unresolved externals

This made the clang-cl self-host not use atomics, which is silly. Force including intrin.h fixes it, but that's a hack.

We could handle this entirely in clang by emitting an inline asm call, but that's gross. We could add the LLVM intrinsic, or see if there's one we can reuse.

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 4, 2015

I hit this again in this way:

ReactOS build is still blocked by this. Tested r241339.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 7, 2015

Some of the link errors we get with:

clang version 3.8.0 (trunk 251783)
Target: i686-pc-windows-msvc
Thread model: posix

are:

ldrinit.c.obj : error LNK2019: unresolved external symbol __mm_pause referenced in function _LdrpInitSecurityCookie@4
ldrinit.c.obj : error LNK2019: unresolved external symbol ___ull_rshift referenced in function _LdrpInitSecurityCookie@4
rtl.lib(actctx.c.obj) : error LNK2019: unresolved external symbol __byteswap_ushort referenced in function "?filt$0@0@check_actctx@@" (?filt$0@0@check_actctx@@)
rtl.lib(bitmap.c.obj) : error LNK2019: unresolved external symbol __BitScanReverse referenced in function _RtlFindMostSignificantBit@8
rtl.lib(bitmap.c.obj) : error LNK2019: unresolved external symbol __BitScanForward referenced in function _RtlFindLeastSignificantBit@8
rtl.lib(except.c.obj) : error LNK2019: unresolved external symbol __ReturnAddress referenced in function _RtlUnwind@16
rtl.lib(slist.c.obj) : error LNK2019: unresolved external symbol __InterlockedCompareExchange64 referenced in function @​RtlInterlockedPushListSList@16
rtl.lib(interlck.c.obj) : error LNK2001: unresolved external symbol __InterlockedCompareExchange64
rtl.lib(network.c.obj) : error LNK2019: unresolved external symbol __byteswap_ulong referenced in function _RtlIpv4StringToAddressW@16

@ismail
Copy link
Contributor

ismail commented Mar 6, 2016

Apparently Google's HighwayHash (https://github.com/google/highwayhash/) uses this:

#if MSC_VERSION
#include <intrin.h>
#pragma intrinsic(_ReadWriteBarrier)
#define COMPILER_FENCE _ReadWriteBarrier()
#elif GCC_VERSION
#define COMPILER_FENCE asm volatile("" : : : "memory")
#else
#define COMPILER_FENCE
#endif

@nico
Copy link
Contributor

nico commented Mar 9, 2016

Comment 10: That should work fine today.

Any time you explicitly include intrin.h, things should be fine. We'll ignore the pragma intrinsic line and use what's in intrin.h, and that works (hence the recommendation to build with /FIIntrin.h in clang-cl builds for now).

What doesn't work is to manually declare an intrinsic's prototype and then say pragma intrinsic, like so:

void __cpuid(int info[4], int fn);
#pragma intrinsic(__cpuid)

With cl, this will work: The first line provides a declaration, and the second tells the compiler to use an intrinsic. With clang-cl, __cpuid is implemented as an always-inline function in the intrin.h header shipping with clang-cl. Since that's not included here, the definition of __cpuid() isn't available and hence you'll get a link error for __cpuid if it's called.

In code you control, you can always explicitly #include <intrin.h> to fix this. However, some system headers do declare functions and then mark some of them as pragma intrinsic, and that's why you'll e.g. end up with undefined references to __stosb if you call memset(foo, 0, ...): The system header expands memset() to something that's an inline function that calls __stosb, and the system header expects that __stosb won't require intrin.h nor a definition for __stosb since it declares it as intrinsic.

Fixing that is what this bug is about. In practice, we should figure out which intrinsics are forward-declared and used by system headers in this way, and add support for those.

@rnk
Copy link
Collaborator

rnk commented Oct 14, 2016

Albert implemented #pragma intrinsic and a lot of builtins for it. It may not cover every case, but the situation should be substantially improved now.

@nico
Copy link
Contributor

nico commented Dec 2, 2016

Yes, let's mark this done. If someone is missing anything from the current implementation, let's have targeted bugs for that.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 27, 2021

mentioned in issue #7187

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 9, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla c++
Projects
None yet
Development

No branches or pull requests

6 participants