While working on bug 1445345 I ended up with a massive patch, which involved huge numbers of changes to various files. The patches I was writing felt very delicate, and I didn't feel like they were a good long term solution. I had the idea to instead try to systematically solve the problem. The macros which have historically been a problem in windows.h are function alias macros, like "#define CreateWindow CreateWindowA". These exist largely because there aren't good tools for implementing function name aliases in C. Fortunately for us, we use C++, so we have the **immense power** of templates and `auto` at our disposal, and don't need #define to create function aliases. So, I wrote a simple python script called MinWin.py - it preprocesses a source file which imports <windows.h>, collects all of the defines which are generated, finds the ones which are UpperCamelCase, undefines them, and then re-defines them as C++ function name aliases. The result is a header called MinWin.h which contains #include <windows.h> followed by a long stream of redefines. This means that code which uses these function aliases still works, while we also avoid the problems which occur when defines are used. It also means that MinWin.h and windows.h don't conflict, and we can use each wherever it is convenient. The script supports clang-cl, msvc, and mingw, which I believe are all of the compilers we support on windows.

MozReview-Commit-ID: J1BNGGSrtZX

Comment on attachment 8961888 [details] [diff] [review] Part 2: Change a bunch of code to use MinWin.h instead of windows.h Review of attachment 8961888 [details] [diff] [review]: ----------------------------------------------------------------- Just a few comments on the interesting-ish parts of this patch ::: dom/serviceworkers/test/gtest/TestReadWrite.cpp @@ +64,5 @@ > return file.forget(); > } > > bool > +CreateFileHelper(const nsACString& aData) This test function had to be renamed, as it used to be called CreateFile, which was #defined to CreateFileA. It would then overload the windows CreateFileA function, which was fine. The new CreateFile wrapper is defined in such a way that it cannot be overloaded, which means that the name had to be changed to avoid multiple definition conflicts. ::: netwerk/test/TestNamedPipeService.cpp @@ +244,5 @@ > return FALSE; > } > > static nsresult > +CreateNamedPipeHelper(LPHANDLE aServer, LPHANDLE aClient) This is the same situation as the CreateFileHelper case - and it's also in a test file. ::: xpcom/base/moz.build @@ +182,5 @@ > > +if CONFIG['OS_ARCH'] == 'WINNT': > + GENERATED_FILES += ['MinWin.h'] > + GENERATED_FILES['MinWin.h'].script = 'MinWin.py:gen_minwin' > + EXPORTS.mozilla += ['!MinWin.h'] I meant to put this hunk in the previous commit, not sure why it ended up in here :-/. I'll move it over before I commit. ::: xpcom/build/nsWindowsDllInterceptor.h @@ +1268,5 @@ > + if (len < 0) { > + // RIP-relative not yet supported > + MOZ_ASSERT_UNREACHABLE("Unrecognized opcode sequence"); > + return; > + } I have absolutely no idea why this hunk is here... It looks exactly the same?

Comment on attachment 8961887 [details] [diff] [review] Part 1: Add MinWin.h, an autogenerated windows.h wrapper which rewrites problematic macros Review of attachment 8961887 [details] [diff] [review]: ----------------------------------------------------------------- Did you mean to have moz.build changes for this diff, to automatically run MinWin.py during the build process? What does this do to compile times? I'm particularly curious if the template matching and all the lambdas flying around do horrible things. I understand that this approach is somewhat more elegant than js/src/util/Windows.h, but would it be terrible to take the JS approach? No big problems, but I would like to discuss, so canceling review. ::: xpcom/base/MinWin.py @@ +5,5 @@ > + > +import buildconfig > +import mozpack.path as mozpath > + > +## -- Helpers -- I think it'd be helpful to have an overview comment describing our strategy, either here or in MinWin_in.cpp (probably here). @@ +8,5 @@ > + > +## -- Helpers -- > + > +# Matches a string which is UpperCamelCase > +# (NOTE: Seems to do a good-enough job. I focused on simplicitly) Nit: "simplicity" @@ +30,5 @@ > + > +def args_split(s): # Split a list of args or params. > + return [a.strip() for a in s.split(',')] if s.strip() else [] > + > +def effective_lines(s): lines_over_continuations, maybe? I guess effective_lines is a decent not overly-long name. @@ +31,5 @@ > +def args_split(s): # Split a list of args or params. > + return [a.strip() for a in s.split(',')] if s.strip() else [] > + > +def effective_lines(s): > + # Get the "effective" lines in s, considering \\ characters before Nit: "considering \ characters before", right, because we're working in prose and not Python strings? @@ +154,5 @@ > + for line in effective_lines(preprocessed): > + # Pull file info out of #line directives. > + lmatch = line_re.match(line) > + if lmatch: > + file = lmatch.group('file') For each of these ifs, we can just `continue` if the condition is true and we've done the necessary work associated with the match, correct? Since only one regex can match per line, yeah? @@ +160,5 @@ > + # Track what defines windows.h declares, and where. > + dmatch = define_re.match(line) > + if dmatch: > + di = DefineInfo(dmatch.group('name'), dmatch.group('rest'), file) > + defines[di.name] = di Do we want to assert here that `di.name not in defines`? @@ +165,5 @@ > + > + umatch = undef_re.match(line) > + if umatch: > + if umatch.group('name') in defines: > + del defines[umatch.group('name')] WDYT about just `defines.pop(umatch.group('name'), 0)`? @@ +173,5 @@ > + > +## -- Entry Point -- > + > +def gen_minwin(fd): > + minwin_in = mozpath.join(buildconfig.topsrcdir, 'xpcom/base/MinWin_in.cpp') Assuming this script was going to be invoked during GENERATED_FILES, it'd be better to list this file as an explicit input to the script, so that dependency tracking will work correctly and pick up changes to MinWin_in.cpp. @@ +221,5 @@ > + .replace("_MINWIN_H_SUBST_POINT", out) > + fd.write("// THIS FILE WAS GENERATED BY MinWin.py -- DO NOT EDIT\n\n") > + fd.write(body) > + > + return set([minwin_in]) # Declare a dependency on minwin Oh, I see you did the above! I think we'd prefer that it was listed as an input in moz.build, FWIW, but thank you for picking up on this! ::: xpcom/base/MinWin_in.cpp @@ +5,5 @@ > + * Code which uses the #undef-ed macros should continue to function, as they > + * are re-defined as C++ templated lambdas or name aliases. > + * > + * MinWin.py is the code generator which produces MinWin.h, using this file > + * (MinWin_in.cpp) as a template, and as the test preprocessor input. Nit: maybe this should be called MinWin_in.template or something, rather than .cpp? @@ +57,5 @@ > +template<size_t idx, typename R, typename... Args> > +struct minwinFnArg<idx, R __stdcall(Args...)> : minwinFnArg<idx, R(Args...)> {}; > +#endif > + > +#define ARG(fn, idx) typename minwinFnArg<idx, decltype(fn)>::Type And we're *sure* this isn't going to conflict with things in windows.h? Or anywhere else in our codebase, for that matter? @@ +62,5 @@ > + > +#ifdef UNICODE > +# define UNICODE_SUFFIXED(name) name ## W > +#else > +# define UNICODE_SUFFIXED(name) name ## A Same question for this macro.

Comment on attachment 8961888 [details] [diff] [review] Part 2: Change a bunch of code to use MinWin.h instead of windows.h Review of attachment 8961888 [details] [diff] [review]: ----------------------------------------------------------------- r=me on this + whatever changes are required based on discussion for part 1. ::: xpcom/build/nsWindowsDllInterceptor.h @@ +1268,5 @@ > + if (len < 0) { > + // RIP-relative not yet supported > + MOZ_ASSERT_UNREACHABLE("Unrecognized opcode sequence"); > + return; > + } I bet this hunk is to due line ending changes.

(In reply to Nathan Froyd [:froydnj] from comment #5) > Did you mean to have moz.build changes for this diff, to automatically run > MinWin.py during the build process? Yes, I did. They ended up accidentally in the second patch. > What does this do to compile times? I'm particularly curious if the > template matching and all the lambdas flying around do horrible things. I hope that they don't, but TBH I haven't measured it. If they cause build time problems I can probably use more template trickery to delay typechecking of them until they're used, which would probably mitigate the issue (as msvc and clang-cl do lazy typechecking of templates for compat reasons). > I understand that this approach is somewhat more elegant than > js/src/util/Windows.h, but would it be terrible to take the JS approach? I like this approach because unlike the JS approach it systematically avoids the issue, rather than fixing individual pain points as they're found. In addition, this approach keeps existing code which uses these aliases working, while that code has to be changed when we take the JS approach. In effect, I worry that the JS approach still makes it undesirable to include windows.h around the place, while this approach makes including windows.h generally a non-issue. > > ::: xpcom/base/MinWin.py > @@ +5,5 @@ > > + > > +import buildconfig > > +import mozpack.path as mozpath > > + > > +## -- Helpers -- > > I think it'd be helpful to have an overview comment describing our strategy, > either here or in MinWin_in.cpp (probably here). I can do that. > @@ +30,5 @@ > > + > > +def args_split(s): # Split a list of args or params. > > + return [a.strip() for a in s.split(',')] if s.strip() else [] > > + > > +def effective_lines(s): > > lines_over_continuations, maybe? I guess effective_lines is a decent not > overly-long name. Sure, works for me > @@ +31,5 @@ > > +def args_split(s): # Split a list of args or params. > > + return [a.strip() for a in s.split(',')] if s.strip() else [] > > + > > +def effective_lines(s): > > + # Get the "effective" lines in s, considering \\ characters before > > Nit: "considering \ characters before", right, because we're working in > prose and not Python strings? Oops, This used to be a doc string which I moved into a comment. > For each of these ifs, we can just `continue` if the condition is true and > we've done the necessary work associated with the match, correct? Since > only one regex can match per line, yeah? This is true. I can add those `continue`s if you'd like. > @@ +173,5 @@ > > + > > +## -- Entry Point -- > > + > > +def gen_minwin(fd): > > + minwin_in = mozpath.join(buildconfig.topsrcdir, 'xpcom/base/MinWin_in.cpp') > > Assuming this script was going to be invoked during GENERATED_FILES, it'd be > better to list this file as an explicit input to the script, so that > dependency tracking will work correctly and pick up changes to MinWin_in.cpp. > > ... > > Oh, I see you did the above! I think we'd prefer that it was listed as an > input in moz.build, FWIW, but thank you for picking up on this! Sure, I can change that. > Nit: maybe this should be called MinWin_in.template or something, rather > than .cpp? The .cpp extension is 'cause I wanted to make sure that if cl.exe or any other compiler inferred the language from the file extension it would pick the correct language. Not sure if that's an actual problem though, I just didn't want the file to be compiled in C mode. > And we're *sure* this isn't going to conflict with things in windows.h? > Or anywhere else in our codebase, for that matter? I made it a short macro name to make it easier to read the generated code. It's not hard to add a MINWIN_ prefix which should ensure that we don't conflict with anything else. I could also just generate the raw template code, but I find it verbose & hard to read. Same deal with the UNICODE_SUFFIXED macro.

(In reply to Nika Layzell [:mystor] from comment #7) > (In reply to Nathan Froyd [:froydnj] from comment #5) > > What does this do to compile times? I'm particularly curious if the > > template matching and all the lambdas flying around do horrible things. > > I hope that they don't, but TBH I haven't measured it. If they cause build > time problems I can probably use more template trickery to delay > typechecking of them until they're used, which would probably mitigate the > issue (as msvc and clang-cl do lazy typechecking of templates for compat > reasons). Please measure this on try or locally, whichever you like, both if possible. > > For each of these ifs, we can just `continue` if the condition is true and > > we've done the necessary work associated with the match, correct? Since > > only one regex can match per line, yeah? > > This is true. I can add those `continue`s if you'd like. I think that would be clearer, yes. > > Nit: maybe this should be called MinWin_in.template or something, rather > > than .cpp? > > The .cpp extension is 'cause I wanted to make sure that if cl.exe or any > other compiler inferred the language from the file extension it would pick > the correct language. Not sure if that's an actual problem though, I just > didn't want the file to be compiled in C mode. I don't understand this comment. MinWin_in.cpp is never going to be compiled; it's just serving as the basis for minwin.py to use for the actual header. And the result of minwin.py is going to be written to some .h file anyway... > > And we're *sure* this isn't going to conflict with things in windows.h? > > Or anywhere else in our codebase, for that matter? > > I made it a short macro name to make it easier to read the generated code. > It's not hard to add a MINWIN_ prefix which should ensure that we don't > conflict with anything else. > > Same deal with the UNICODE_SUFFIXED macro. I think these should use--at the very least--MOZ_ prefixes, and probably even MOZ_MINWIN_ prefixes to avoid potential conflicts.

Ran into a windows.h bogeyman again, and was reminded of this bug, so I'm doing some grave-digging here :-P > > > Nit: maybe this should be called MinWin_in.template or something, rather > > > than .cpp? > > > > The .cpp extension is 'cause I wanted to make sure that if cl.exe or any > > other compiler inferred the language from the file extension it would pick > > the correct language. Not sure if that's an actual problem though, I just > > didn't want the file to be compiled in C mode. > > I don't understand this comment. MinWin_in.cpp is never going to be > compiled; it's just serving as the basis for minwin.py to use for the actual > header. And the result of minwin.py is going to be written to some .h file > anyway... It actually is sorta "compiled" - the compiler is run on the file with flags to cause it to print out all defines. The cl.exe compiler will look at the passed-in file, and if it's a `.template` file, won't pick the correct language to build with, which might cause weird results (due to e.g. parsing failing).

ni? :glandium to take a look at the commit again, and give any feedback. I added a comment to the patch which I will copy here: I've pushed an updated version for review. A few notes: 1. This version builds on all supported platforms & passes all tests on try, unlike previous patches which would fail on platforms like mingw. 2. I still only wrap `windows.h` and not sub-windows headers. This is done because handling other headers as well would require additional complexity. This approach achieves the greatest impact without doing that extra work. We can look into potentially making this system more generic / better in the future. 3. This version should be easier to debug when the wrapping fails. A new define, `MOZ_WINDOWS_WRAPPER_DISABLED_REASON`, is set to a string reason explaining why the wrapper failed to load. 4. The code changes required have reduced even more, they are now: 1. Code which checks `_WINDOWS_` to emit an error now also checks `!defined(MOZ_WRAPPED_WINDOWS_H)` 2. `thebes` has been fixed to not use suffixed calls to a method with a name conflicting with a `windows.h` define. 3. In two 3rd-party libraries (`cairo` and `openvr`), `windows.h` wrapping is explicitly disabled, as they depend on the odd behaviours created by define name aliasing, and we don't want to modify external code.

Clearing NI until I look into refactoring IPC more in-depth

By default, windows.h exposes a large number of problematic define statements which are UpperCamelCase, such as a define from `CreateWindow` to `CreateWindow{A,W}`. These can mess up Gecko C++ code which depnds on them. The header also defines some traditional SCREAMING_SNAKE_CASE defines which can mess up our code. This patch adds a simple code generator which generates wrappers for these defines, and uses them to wrap the windows.h wrapper using the `stl_wrappers` mechanism, allowing us to use windows.h in more places.

Pushed by nika@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c97b4db60a Wrap windows.h to avoid problematic define statements, r=froydnj,glandium

Backed out 9 changesets (bug 1500948, bug 1500949, bug 1448426, bug 1487249, bug 1500950, bug 1500944) for causing talos crashes on ts_paint | application crashed [@ MOZ_CrashOOL(char const*, int, char const*)] Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/c12b84f575c3fe48d72f3c8c24a54b3dee1cc985 Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=429776feea6ba796345ef7e41b9c9d367335ef62 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=213326943&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=213331977&repo=mozilla-inbound :Nika Layzell Cold you please take a look?

Pushed by nika@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f95dd96d54 Wrap windows.h to avoid problematic define statements, r=froydnj,glandium

Backed out 13 changesets (bug 1500948, bug 1509362, bug 1509591, bug 1448426, bug 1500949, bug 1487249, bug 1509930, bug 1500950, bug 1500944) for causing crashes and assertion failures on PBackgroundParent.cpp:696 CLOSED TREE Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfe29337ffe3d93cd060077e2e999e72bb9b7cf Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=214255312&revision=c3fe435e473a463fbc22d4afa531bdedb757079c Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=214255312&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=214255794&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=214249038&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=214249630&repo=mozilla-inbound :Nika Layzell Could you please take a look?

Nika, you should try landing bug by bug.

Pushed by nika@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6dae0c1e5a Wrap windows.h to avoid problematic define statements, r=froydnj,glandium