Page MenuHomePhabricator

[MSVC] Add ARM support to intrin.h for MSVC compatibility
ClosedPublic

Authored by mstorsjo on Jul 25 2016, 1:29 PM.

Details

Reviewers
rengolin
compnerd
Summary

This fixes compiling with headers from the Windows SDK for ARM, where the YieldProcessor function (in winnt.h) refers to _ARM_BARRIER_ISHST.

The actual MSVC armintr.h contains a lot more definitions, but this is enough to build code that uses the Windows SDK but doesn't use ARM intrinsics directly.

An alternative would to just keep the addition to intrin.h (to include armintr.h), but not actually ship armintr.h, instead having clang's intrin.h include armintr.h from MSVC's include directory. (That one works fine with clang, at least for building code that uses the Windows SDK.)

Diff Detail

Event Timeline

mstorsjo updated this revision to Diff 65407.Jul 25 2016, 1:29 PM
mstorsjo retitled this revision from to [MSVC] Add ARM support to intrin.h for MSVC compatibility.
mstorsjo updated this object.
mstorsjo added a subscriber: cfe-commits.
mstorsjo updated this object.Aug 1 2016, 1:10 AM
rengolin added a subscriber: compnerd.

Can you add a test on test/Headers, like the others, please?

I'm not well versed on Windows. @compnerd?

lib/Headers/armintr.h
2

Maybe a hint that this is "ARM Windows Intrinsics"?

28

Maybe an ifdef Windows wrapper, to make sure not to mess up *nix environments?

Can you add a test on test/Headers, like the others, please?

Sure, there seems to be a good spot for that in ms-intrin.cpp.

lib/Headers/armintr.h
2

Sure

28

I can add the same as at the top of intrin.h, i.e. this:

/* Only include this if we're compiling for the windows platform. */
#ifndef _MSC_VER
#include_next <intrin.h>
#else
rengolin added inline comments.Aug 1 2016, 4:46 AM
lib/Headers/armintr.h
28

Could be. I know you have that on the including header, but nothing stops people from including this one too by accident. Also, it makes it more clear where it should work and where it shouldn't.

mstorsjo added inline comments.Aug 1 2016, 5:01 AM
lib/Headers/armintr.h
28

No, I meant just like you want, but instead of #ifdef _WIN32 use #ifndef _MSC_VER, and use #include_next <armintr.h> for when not working in MSVC mode, i.e. behave as if our header didn't exist at all.

So I agree that it's useful to have on this individual header as well, not only rely on this guard in the intrin.h.

rengolin added inline comments.Aug 1 2016, 5:07 AM
lib/Headers/armintr.h
28

Sounds good.

mstorsjo updated this revision to Diff 66306.Aug 1 2016, 5:30 AM

Added a test, added "windows" to the header title line, added an #ifndef _MSC_VER #include_next to the header.

rengolin accepted this revision.Aug 1 2016, 6:04 AM
rengolin added a reviewer: rengolin.

LGTM. Thanks!

This revision is now accepted and ready to land.Aug 1 2016, 6:04 AM
compnerd added inline comments.Aug 1 2016, 5:30 PM
lib/Headers/armintr.h
26

Hmm, why not do __has_header instead? armv7-windows-itanium won't define _MSC_VER but you may have the header from the SDK.

mstorsjo added inline comments.Aug 1 2016, 11:19 PM
lib/Headers/armintr.h
26

Hmm, so you mean like this?

#if __has_include_next(<armintr.h>)
#include_next <armintr.h>
#else
// Normal file content
typedef enum ...

That would cause use in armv7-windows-msvc mode to use the SDK's armintr.h instead of ours - which as far as I know also is fine. (The issue I'm mainly fixing is that including <intrin.h> is expected to bring in this, but currently it doesn't.)

I'm not sure I follow your argument completely though - armv7-windows-itanium doesn't define _MSC_VER, and thus would already try to include the next armintr.h, which would be picked up from the SDK if it exists there.

Ping - any reply from @compnerd?

In any case, feel free to formulate the ifdefs whichever way you want on commit as well (since I don't have commit access), as long as the included test passes.

compnerd edited edge metadata.Aug 5 2016, 9:39 AM

I had reversed the condition in my head. Why include_next the header if not on Windows?

Why include_next the header if not on Windows?

Mostly because intrin.h uses the same logic. Normally, there's probably no such system header in other SDKs, but say if you happen to have a custom one named like that (on a non-windows platform unaware that windows happens to have a system header named like that) and clang's builtin header gets picked first, include the next one in the include directory chain instead - i.e. pretend like this one doesn't exist. (That is, either pick the one you want, or error out saying it isn't found.)

I don't mind particularly much though, I just picked this logic since intrin.h was using it as well. Just having the header be #ifdef _MSC_VER {content} #endif, or #ifndef _MSC_VER #error #else {content} #endif also would be fine for me.

The include_next is to make sure that, whatever other environment that happens to have the same include name, should pick it up if the user or the environment provides one, or errors out if it doesn't. Pretty standard.

compnerd accepted this revision.Aug 5 2016, 5:29 PM
compnerd edited edge metadata.

Ah, okay. Sounds good.

Can someone commit this for me?

compnerd closed this revision.Aug 6 2016, 11:06 AM

SVN r277928