Closed Bug 1402355 Opened 2 years ago Closed 2 years ago

mozilla/ThreadLocal.h:96:12: error: ‘TLS_OUT_OF_INDEXES’ was not declared in this scope

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file)

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>`...
Attachment #8911207 - Flags: review-
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")?
Attachment #8911207 - Flags: review?(mh+mozilla) → review+
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.
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/628bbe2a4815
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.