TLS_OUT_OF_INDEXES is declared in windows.h but is not included in ThreadLocal.h resulting in lots of missing identifier errors.

Comment on attachment 8911207 [details] Bug 1402355 Require TLS_OUT_OF_INDEXES to be defined before ThreadLocalKeyStoragewill be defined on Windows https://reviewboard.mozilla.org/r/182702/#review187972 We have discouraged including `<windows.h>` in random headers if we can avoid it, because of all the cruft that it brings in. Could we just define the necessary macro here, with `#ifndef` guards? OTOH, I see ThreadLocal.h already uses `TlsAlloc` and so forth, which are clearly only declared in `<windows.h>`...

Nathan and I crossed reviews. Try Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42d76e843c8366b363b599030044e4af7e876ee6

So I'm getting build breakage from this change because processthreadsapii.h pulls in basetsd.h. basetsd.h defines some types like INT8 which is a an 8-bit integer type: typedef unsigned char UINT8, *PUINT8; This conflicts with nrappkit which says INT8 means an 8-byte integer type. We get a typedef clash: > z:/build/build/src/media/mtransport/third_party/nrappkit/src/util/libekr\r_types.h(204,25): error: typedef redefinition with different types ('uint64_t' (aka 'unsigned long long') vs 'unsigned char') > typedef R_DEFINED_UINT8 UINT8; > > z:\build\build\src\vs2015u3\SDK\Include\10.0.14393.0\shared\basetsd.h(79,29): note: previous definition is here > typedef unsigned char UINT8, *PUINT8;

The part that uses TLS_OUT_OF_INDEXES in this file is only used if the file that includes the header really wants to. We can make the contract be that if you want that, you need to include windows.h or whatever first. Assuming TLS_OUT_OF_INDEXES is a #define and not a const, you could just put the whole section in a #ifdef.

(In reply to Mike Hommey [:glandium] from comment #6) > The part that uses TLS_OUT_OF_INDEXES in this file is only used if the file > that includes the header really wants to. We can make the contract be that > if you want that, you need to include windows.h or whatever first. So I took this approach, and it works, although it required fixing up 9 files.

What I meant was to add ifdef TLS_OUT_OF_INDEXES around class ThreadLocalKeyStorage.

(In reply to Mike Hommey [:glandium] from comment #9) > What I meant was to add ifdef TLS_OUT_OF_INDEXES around class > ThreadLocalKeyStorage. Ah. I tried this, but it was a bit of a mess: - For the MinGW build I need to fix up the files regardless; otherwise TLS_OUT_OF_INDEXES is not defined and we revert to the pthread-based class. But in some places it _is_ defined, so we're probably mixing the class type. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc7e009ec57a4dab2089816998c0f143fe3e6508&selectedJob=132943737 - Somehow, for the vanilla Windows build, the same thing occurs: I'm seeing errors in the pthread-based class: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a923c95bb77f80fd3a5a59885ac4c9e8ad66f8d&selectedJob=132945883

That's because you changed #ifdef XP_WIN into #if defined(XP_WIN) && defined(TLS_OUT_OF_INDEXES). The point is to make ifdef XP_WIN a noop if TLS_OUT_OF_INDEXES is not defined, not make it use the alternative pthread implementation.

Ah, thank you for the repeated guidance. This seems to work, I cleaned up the commit and flagged you for review. New try runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2b1d8ee564c9f034640e04bbad9193087809139 (Windows) https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea4cb579d980bf6a3d544dbbe251a90363733465 (MinGW - should fail with a xul.dll linking error from Bug 1402385)

Comment on attachment 8911207 [details] Bug 1402355 Require TLS_OUT_OF_INDEXES to be defined before ThreadLocalKeyStoragewill be defined on Windows https://reviewboard.mozilla.org/r/182702/#review189086 ::: mfbt/ThreadLocal.h:91 (Diff revision 4) > }; > > -#ifdef XP_WIN > +#if defined(XP_WIN) > +/* > + * ThreadLocalKeyStorage uses Thread Local APIs that are declared in > + * processthreadsapi.h. To use this class on Windows, include this file The rules for this and that are obscure and I'm not native, but shouldn't "this" be a "that" (in "include this file")?

Considering bug 1399031 made it to 57, you may want an uplift.

Thanks! No uplift needed though, the only branches I care about keeping a mingw version building on are esr52 and central.

Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/628bbe2a4815 Require TLS_OUT_OF_INDEXES to be defined before ThreadLocalKeyStoragewill be defined on Windows r=glandium