There's some Windows-specific code that requires some changes to be spec- (or at least clang-) compliant.

Comment on attachment 8997158 [details] Bug 1480457 Address moz.build failures for mingw-clang https://reviewboard.mozilla.org/r/261022/#review268342

Comment on attachment 8997159 [details] Bug 1480457 Add typename specifier to TargetFunction.h https://reviewboard.mozilla.org/r/261024/#review268344 I think that I might have reviewed this one in another bug, so don't be surprised if it is dropped during commit.

Comment on attachment 8997160 [details] Bug 1480457 Resolve a missing 'template' keyword in interceptor/TargetFunction.h https://reviewboard.mozilla.org/r/261026/#review268346

Comment on attachment 8997161 [details] Bug 1480457 Cast FARPROC to void* because clang doesn't do an automatic conversion like msvc https://reviewboard.mozilla.org/r/261028/#review268348 I know I've seen this one previously. This change will conflict with one that was already made in another bug. Please refresh your source tree.

Comment on attachment 8997162 [details] Bug 1480457 Cast a specific type of function pointer to void* for a ctor argument match https://reviewboard.mozilla.org/r/261030/#review268350 Same, I've seen this one already.

Comment on attachment 8997163 [details] Bug 1480457 Resolve an error that requires one to use this-> to find a member variable https://reviewboard.mozilla.org/r/261032/#review268352

Comment on attachment 8997164 [details] Bug 1480457 Don't pass 'false' into a pointer field; pass nullptr https://reviewboard.mozilla.org/r/261034/#review268354

Comment on attachment 8997165 [details] Bug 1480457 Change a ui64 suffix to ULL https://reviewboard.mozilla.org/r/261036/#review268356

Comment on attachment 8997166 [details] Bug 1480457 Cast what is exposed as an unsigned char into the void* it is to point at the Process Heap https://reviewboard.mozilla.org/r/261038/#review268358 r- just because I need to know more about what is going on here. That's odd... Is this a mingw thing? That field is a PVOID in the Windows SDK, which is a typedef for `void*` Can you show me the definition of the PEB structure that you're building with?

Comment on attachment 8997167 [details] Bug 1480457 Correct typedef so it matches an earlier typedef https://reviewboard.mozilla.org/r/261040/#review268360 Needs more info: Where is this earlier typedef? Does it differ depending on whether the build is 32-bit or 64-bit? I'm inclined to think that the right type here should be intptr_t, but I don't know enough about the previous typedef(s).

Giving this a p1 to get it out of the triage list, as there are patches in active development.

I rebased onto central today, and a couple of the patches you thought had been addressed elsewhere had not yet. (Although others had.)

Comment on attachment 8997161 [details] Bug 1480457 Cast FARPROC to void* because clang doesn't do an automatic conversion like msvc https://reviewboard.mozilla.org/r/261028/#review269470 ::: mozglue/misc/interceptor/PatcherDetour.h:166 (Diff revision 2) > return !!this->mVMPolicy; > } > > bool AddHook(FARPROC aTargetFn, intptr_t aHookDest, void** aOrigFunc) > { > - ReadOnlyTargetFunction<MMPolicyT> target(this->ResolveRedirectedAddress(aTargetFn)); > + ReadOnlyTargetFunction<MMPolicyT> target(this->ResolveRedirectedAddress((void*)aTargetFn)); This was fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1479777

Comment on attachment 8997162 [details] Bug 1480457 Cast a specific type of function pointer to void* for a ctor argument match https://reviewboard.mozilla.org/r/261030/#review269472 ::: mozglue/misc/nsWindowsDllInterceptor.h:129 (Diff revision 2) > > bool Set(InterceptorT& aInterceptor, const char* aName, > FuncPtrT aHookDest) > { > LPVOID addHookOk; > - InitOnceContext ctx(this, &aInterceptor, aName, aHookDest, false); > + InitOnceContext ctx(this, &aInterceptor, aName, (void*)aHookDest, false); This was also fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1479777 And confirmed via searchfox.

Comment on attachment 8997166 [details] Bug 1480457 Cast what is exposed as an unsigned char into the void* it is to point at the Process Heap https://reviewboard.mozilla.org/r/261038/#review269476 ::: browser/app/winlauncher/NativeNt.h:554 (Diff revision 2) > inline HANDLE > RtlGetProcessHeap() > { > PTEB teb = ::NtCurrentTeb(); > PPEB peb = teb->ProcessEnvironmentBlock; > - return peb->Reserved4[1]; > + return (void *)peb->Reserved4[1]; Please answer my question from comment 19.

Comment on attachment 8997167 [details] Bug 1480457 Correct typedef so it matches an earlier typedef https://reviewboard.mozilla.org/r/261040/#review269478 ::: memory/replace/logalloc/replay/Replay.cpp:13 (Diff revision 2) > #include "mozmemory_wrap.h" > > #ifdef _WIN32 > #include <windows.h> > #include <io.h> > -typedef int ssize_t; > +typedef long long ssize_t; Please answer my question from comment 20.

I'm sorry twice - the bad review reuqest last time was because I pushed from the wrong repo. The comments you requested are added in the commit messages, and the superfluous commits are removed. The second reason I'm sorry is because mozreview messed up my push updating the commits, and it lost your previous r+'s and then assigned one of them to a brand new patch you have not reviewed.

Sorry that I haven't gotten to this yet. I went to look today, but it looks like the reviewboard sunset has now occurred. Could you please resubmit via Phabricator?

This code throws an error in clang on the inner MMPolicy: error: declaration of 'MMPolicy' shadows template parameter Notethat the template parameter is declared earlier at the class definition of ReadOnlyTargetFunction MozReview-Commit-ID: buLE9d22YS

MozReview-Commit-ID: FUA6jMwCV1d Depends on D4571

MozReview-Commit-ID: KhWX2WiejtR Depends on D4572

error: cannot initialize return object of type 'HANDLE' (aka 'void *') with an lvalue of type 'BYTE' (aka 'unsigned char') In the Windows SDK, this is correctly exposed as a void*. In MinGW, it exposed as BYTE Reserved4[104]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-headers/include/winternl.h MozReview-Commit-ID: HjculhCcngl Depends on D4573

error: typedef redefinition with different types ('int' vs 'long long') MinGW defines this as either a __int64 or an int depending on platform. MozReview-Commit-ID: 18KoNZ00nIX Depends on D4574

We need to add clang to our list of mingw compilers in interceptor/moz.build And we need to add dhcpcsvc to toolkit/library's OS Libs to be explicit for mingw MozReview-Commit-ID: Dk2FTzacCUK Depends on D4575

Phabricated!

Comment on attachment 9004947 [details] Bug 1480457 Address moz.build failures for mingw-clang r=aklotz Aaron Klotz [:aklotz] has approved the revision.

Comment on attachment 9004946 [details] Bug 1480457 Correct typedef so it matches an earlier typedef r=aklotz Aaron Klotz [:aklotz] has approved the revision.

Comment on attachment 9004944 [details] Bug 1480457 Change a ui64 suffix to ULL r=aklotz Aaron Klotz [:aklotz] has approved the revision.

Comment on attachment 9004943 [details] Bug 1480457 Don't pass 'false' into a pointer field; pass nullptr r=aklotz Aaron Klotz [:aklotz] has approved the revision.

Comment on attachment 9004942 [details] Bug 1480457 Address template parameter shadowing r=aklotz Aaron Klotz [:aklotz] has approved the revision.

I think.... maybe.... I got the commit updated in phabricator correctly. I did a lot of weird stuff to do it....

(In reply to Tom Ritter [:tjr] from comment #48) > Created attachment 9004945 [details] > Bug 1480457 Cast a PEB entry into void* to point at the Process Heap r=aklotz > > error: cannot initialize return object of type 'HANDLE' (aka 'void *') with > an lvalue of type 'BYTE' (aka 'unsigned char') > > In the Windows SDK, this is correctly exposed as a void*. > In MinGW, it exposed as BYTE Reserved4[104]: > https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64- > headers/include/winternl.h > > MozReview-Commit-ID: HjculhCcngl > > Depends on D4573 It's fixed in mingw now, this patch should no longer be needed.

(In reply to Jacek Caban from comment #58) > (In reply to Tom Ritter [:tjr] from comment #48) > > Created attachment 9004945 [details] > > Bug 1480457 Cast a PEB entry into void* to point at the Process Heap r=aklotz > > > > error: cannot initialize return object of type 'HANDLE' (aka 'void *') with > > an lvalue of type 'BYTE' (aka 'unsigned char') > > > > In the Windows SDK, this is correctly exposed as a void*. > > In MinGW, it exposed as BYTE Reserved4[104]: > > https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64- > > headers/include/winternl.h > > > > MozReview-Commit-ID: HjculhCcngl > > > > Depends on D4573 > > It's fixed in mingw now, this patch should no longer be needed. Confirmed! Thanks!

Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e0d83a0de60 Address template parameter shadowing r=aklotz

Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ddcf3daa268c Don't pass 'false' into a pointer field; pass nullptr r=aklotz

Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/87c2ed42acf0 Change a ui64 suffix to ULL r=aklotz

Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ed585dab6e3c Cast a PEB entry into void* to point at the Process Heap r=aklotz

Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ca61cc8a182 Correct typedef so it matches an earlier typedef r=aklotz

Backout by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79901f0800df Backed out changeset ed585dab6e3c for being obsolete. CLOSED TREE

Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2c02757dfa7 Address moz.build failures for mingw-clang r=aklotz

https://hg.mozilla.org/mozilla-central/rev/0e0d83a0de60 https://hg.mozilla.org/mozilla-central/rev/ddcf3daa268c https://hg.mozilla.org/mozilla-central/rev/87c2ed42acf0 https://hg.mozilla.org/mozilla-central/rev/6ca61cc8a182 https://hg.mozilla.org/mozilla-central/rev/a2c02757dfa7