Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2076483002: clang/win: Stop force-including intrin.h everywhere. (Closed)

Created:
2 years, 9 months ago by Nico
Modified:
2 years, 8 months ago
Reviewers:
hans
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

clang/win: Stop force-including intrin.h everywhere. MSVC and clang-cl offer a bunch of functions that are built-in to the compiler: Intrinsics. When an intrinsic is called, the compiler can emit specialized code for intrinsics. (It's free to emit a regular call to a function too though.) intrin.h includes prototypes (and with clang-cl, definitions) for intrinsics. MSVC also supports `#pragma intrinsics(name)`, which allows marking the intrinsic named "name" as something that should preferably be treated as an intrinsic (roughly ~inlined) instead of as a call. With this, it is possible to manually declare an intrinsic and then use that pragma, and since it'll be treated as an intrinsic, no linker error will happen. clang-cl doesn't yet implement `#pragma intrinsic`, so e.g. void __cpuidex(int CPUInfo[4], int info_type, int ecxvalue); #pragma intrinsic(__cpuidex) // later, call __cpuidex() will result in a linker error, since clang-cl sees the declaration for __cpuidex(), but then clang-cl ignores the pragma line, and then later it thinks the call is just a call to a function that isn't defined anywhere. This is not a problem: Just #include <intrin.h> instead of manually declaring the function. With clang-cl, intrin.h contains a definition of __cpuidex (and the other intrinsics), and no linker error will be emitted (and the definition is always_inline, so it's fast). There is just one wrinkle: Some system headers (e.g. windows.h) do manually declare intrinsics, mark them `#pragma intrinsic`, and then call them from other inline functions defined in system headers. So if some of our code calls one of these other inline functions, it needs intrin.h without us knowing about it. Luckily, we know only a single function where this is an issue in practice: SecureZeroMemory(), which calls __stosb. So we had to add explicit and mysterious includes for <intrin.h> to the two files that call that function. If more examples of this pop up, we can reevaluate if we want to force-include this header everywhere, but for now it seems overkill to inject this header into every translation unit just because two translation units need it. BUG=592745 Committed: https://crrev.com/ac7670452f51ecfd1061749b3d5e15f8544a55ee Cr-Commit-Position: refs/heads/master@{#401613}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -12 lines) Patch
M build/common.gypi View 1 chunk +0 lines, -5 lines 0 comments Download
M build/config/win/BUILD.gn View 1 chunk +0 lines, -7 lines 0 comments Download

Messages

Total messages: 21 (11 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2076483002/1
2 years, 9 months ago (2016-06-16 23:09:43 UTC) #3
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/39033)
2 years, 9 months ago (2016-06-17 00:43:55 UTC) #5
Nico
2 years, 8 months ago (2016-06-23 01:39:31 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2076483002/1
2 years, 8 months ago (2016-06-23 01:40:16 UTC) #10
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
2 years, 8 months ago (2016-06-23 02:48:44 UTC) #12
hans
Nice! LGTM, and thanks for the excellent change description! Nit: > If more examples of ...
2 years, 8 months ago (2016-06-23 15:28:24 UTC) #13
Nico
done, thanks!
2 years, 8 months ago (2016-06-23 15:30:35 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2076483002/1
2 years, 8 months ago (2016-06-23 15:31:02 UTC) #17
commit-bot: I haz the power
Committed patchset #1 (id:1)
2 years, 8 months ago (2016-06-23 15:36:15 UTC) #19
commit-bot: I haz the power
2 years, 8 months ago (2016-06-23 15:38:06 UTC) #21
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/ac7670452f51ecfd1061749b3d5e15f8544a55ee
Cr-Commit-Position: refs/heads/master@{#401613}

Powered by Google App Engine
This is Rietveld 408576698