Avoiding windows.h

[Bruce Dawson]

I'm a Chrome developer and I've been working on reducing how frequently windows.h is included in source files when building Chrome for Windows. It used to be (2018) that more than 78% of Chrome's translation units included windows.h When I picked up this project again in June this had been reduced and about half of Chrome's translation units pulled in windows.h. Since June I have reduced this to less than 22%. About 9% of the remaining uses of windows.h (2% of source files used when building Chrome) are in v8 - 761 translation units.

The advantages of reducing usage of windows.h are (very slightly) reduced compile times, reduced namespace pollution (no more #define DrawText DrawTextW), reduced warning-flag manipulation, etc. Mostly it's about avoiding those macro definitions.

So...

I'm looking at getting v8 to use windows.h less. The techniques are well understood and proven and are best summarized in the contents of windows_types.h. This creates the minimal set of typedefs and defines needed to compile Chrome's header files and portable code. In a few cases (CONDITION_VARIABLE, for instance) it is necessary to define Chrome proxy's for Windows types, and cast between them when calling Windows functions.

This is a medium sized project that will require landing lots of small CLs. I've done enough investigation to get a sense of the scope. I'll need to create V8 proxy-types for CONDITION_VARIABLE, SRWLOCK, and CRITICAL_SECTION, create typedefs for HANDLE (this is easy - there's no casting required), and explicitly include windows.h in a few source files that actually need it. And I'll need to fix whatever other issues pop up, but I'm not expecting anything worse than what I've seen before. I think that the vast majority of v8 files will be able to compile without windows.h.

The overall project is tracked by crbug.com/796644 and the bugs which block it.

Is this a project that V8 would support? If so, can I get a volunteer/sponsor who I can work with for advice and some code reviews?

[Bruce Dawson]

To make this more concrete I created a V8 CL that demonstrates what is involved, using obj/v8/v8_libbase/condition-variable.obj as the test case. That file builds. Other files do not.

Some of the files are just hacked into compliance, but it gives a sense of how it would work.

--

Bruce Dawson, he/him

[Leszek Swirski]

Hi Bruce,

Thanks for looking into this, I'd be happy to do code reviews and give advice if you're ok with the EMEA timezone delay - otherwise we've a couple of people in PST too.

One question about the proxy types: are we at risk of triggering undefined behaviour by reinterpret casting between the proxy types and the Windows types? We've gone to some lengths to be UBSan clean and I wouldn't want to regress that.

Cheers,

Leszek

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAE5mQiMF_Yx4cvVKtfhw1bGxqdKCx0U7%3DEz%2BMqGm%2BO%2BBG47U7w%40mail.gmail.com.

[Bruce Dawson]

The proxy-type issue is indeed the one wart in this project. I know that Chrome (at least on Windows, and probably everywhere) has decided not to enforce strict aliasing, but V8 may not have given up on that.

The proxy types are rarely if ever read from or written to using the proxy type, but I'm not sure if that makes them legal or if that just means that every reference is UB - I suspect every reference is UB. And, I can't guarantee that they are only read/written using the real type - object copying would necessarily copy the bits using the proxy type.

I don't think that windows.h can be removed without using this technique (not unless every one of these becomes a pointer to an allocated object). In the Chromium code-base I decided that the ugliness of the aliasing was less than the ugliness of windows-h-everywhere. V8 may, of course, come to a different conclusion.

I updated my CL which uses the proxy technique on a couple of types so that it will build. That will let us see what the UBSan bots think of it. Is the regular dry run sufficient?

You received this message because you are subscribed to a topic in the Google Groups "v8-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/v8-dev/MbE-A6QBOCM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAGRskv-kw1aFXyK%2BQwmOffvh3TVZvWgHHw6MBpoRMeGhGTJ2Hg%40mail.gmail.com.

--

Bruce Dawson, he/him

[Leszek Swirski]

I don't suppose we could just type erase via void*? That should at least be UB-clean. I do think we, in reality, have also given up on strict aliasing, but we nevertheless do try to go via bitcast where possible. I kicked off some UBSan bots on your CL, let's see what they say.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAE5mQiMsatJLmHW9Aufq5EVob3DYQCYa8h_xs7-bfVBLXngOjA%40mail.gmail.com.

[Leszek Swirski]

Or by using std::aligned_storage + some static asserts that sizes match, if we need to actually allocate storage for these types...

[Bruce Dawson]

I'll try std::aligned_storage. It sounds like that will be perfect. I can just drop it in to replace the existing declarations that I created.

You can see the 99% working CL at crrev.com/c//3042215. The only problem is that something doesn't work with intrin.h on cppgc. I include intrin.h and then get an error saying:

note: include the header <intrin.h> or explicitly provide a declaration for '__readfsdword'

I'm also hitting problems where "git cl format" reorders my #includes in a way that breaks compilation and I don't know how to prevent that. If I can get suggestions for those two issues then I can finish this off.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAGRskv9h3KKW59O4VswHZhCkSdPg-wx6JLy0Mc7jkDw640woWg%40mail.gmail.com.

--

Bruce Dawson, he/him

[Dan Elphick]

git cl format won't reorder headers separated by a comment (possibly needs a newline as well but that looks more natural anyway).

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAE5mQiPtfWwttimwqxoMVkT0aO8q-hjSLCeZgoFDY0jjO62STg%40mail.gmail.com.

[Bruce Dawson]

Okay, that makes sense. In Chromium you just need a blank line but I'll add an explanatory comment as well.

Any thoughts on intrin.h? It works in my debug builds in the context of Chromium, but not on the bots. I think intrin.h must be different there. I can keep exploring if nobody knows off the top of their head.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CALH_77tLNROkW7dDzj%3Dg4HK%2BMk%3DrZKRgYP%3Dq%3DTWtkMaeb1-Sow%40mail.gmail.com.

--

Bruce Dawson, he/him

[Bruce Dawson]

This is all working now, including using std::aligned_storage to avoid undefined behavior. You can see the ready-for-review CL at crrev.com/c/3042215. It drops the number of V8 translation units that include windows.h in a build of Chrome for Windows from 760+ to 16. Not bad.

--

Bruce Dawson, he/him