Closed Bug 1448426 Opened 6 years ago Closed 5 years ago

Wrap Windows.h to #undefs deceptive macros, replacing them with proper aliases

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: nika, Assigned: nika)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

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.
Attached file Example MinWin.h output (obsolete) —
MozReview-Commit-ID: J1BNGGSrtZX
Attachment #8961888 - Flags: review?(nfroyd)
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?
Attachment #8961886 - Attachment mime type: text/x-chdr → text/plain
See Also: → 1445345
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.
Attachment #8961887 - Flags: review?(nfroyd)
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.
Attachment #8961888 - Flags: review?(nfroyd) → review+
(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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
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).
Attachment #8961888 - Attachment is obsolete: true
Attachment #8961887 - Attachment is obsolete: true
Attachment #8961886 - Attachment is obsolete: true
Summary: Add MinWin.h, a helper header which undefines the deceptive function-like defines from Windows.h → Wrap Windows.h to #undefs deceptive macros, replacing them with proper aliases
Attachment #9012653 - Attachment description: Bug 1448426 - Wrap windows.h to avoid problematic define statements → Bug 1448426 - Part 2: Wrap windows.h to avoid problematic define statements
Attachment #9015961 - Attachment description: Bug 1448426 - Part 1: Include stl_wrappers as system headers → Bug 1448426 - Part 1: Include stl_wrappers as system headers,
Attachment #9012653 - Attachment description: Bug 1448426 - Part 2: Wrap windows.h to avoid problematic define statements → Bug 1448426 - Part 2: Wrap windows.h to avoid problematic define statements,
Attachment #9015961 - Attachment is obsolete: true
Attachment #9012653 - Attachment description: Bug 1448426 - Part 2: Wrap windows.h to avoid problematic define statements, → Bug 1448426 - Wrap windows.h to avoid problematic define statements,
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.
Flags: needinfo?(mh+mozilla)
Clearing NI until I look into refactoring IPC more in-depth
Flags: needinfo?(mh+mozilla)
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.
Attachment #9012653 - Attachment is obsolete: true
Depends on: 1506277
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
Status: NEW → ASSIGNED
Depends on: 1509362
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
Flags: needinfo?(nika)
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
https://hg.mozilla.org/mozilla-central/rev/5e6dae0c1e5a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: needinfo?(nika)
Depends on: 1511176
See Also: → 834505
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: