Closed
Bug 1336527
Opened 7 years ago
Closed 7 years ago
mingw link step fails with undefined reference to RegisterIdlePeriod
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: tjr, Assigned: tjr)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jcristau
:
approval-mozilla-esr52+
|
Details |
29:58.23 INPUT("StaticXULComponentsEnd/StaticXULComponentsEnd.o") 29:58.23 29:58.23 ../../dom/workers/Unified_cpp_dom_workers2.o:Unified_cpp_dom_workers2.cpp:(.rdata$_ZTVN7mozilla3dom7workers12WorkerThreadE[__ZTVN7mozilla3dom7workers12WorkerThreadE]+0x40): undefined reference to `nsThread::RegisterIdlePeriod(already_AddRefed<nsIIdlePeriod>)@4' 29:58.23 collect2: error: ld returned 1 exit status 29:58.41 /home/worker/workspace/mingw-w64-build/firefox/config/rules.mk:800: recipe for target 'xul.dll' failed 29:58.41 make[5]: *** [xul.dll] Error 1 29:58.43 /home/worker/workspace/mingw-w64-build/firefox/config/recurse.mk:71: recipe for target 'toolkit/library/target' failed 29:58.43 make[4]: *** [toolkit/library/target] Error 2
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Okay, try builds passed, baku, it seems like you do stuff with DOM Workers, could you review or help me find someone to review?
Flags: needinfo?(amarchesini)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8833418 [details] Bug 1336527 Define RegisterIdlePeriod https://reviewboard.mozilla.org/r/109646/#review111012 WorkerThread inherits nsThread. This patch doesn't add nothing useful. We have many nsThread implementations, why only WorkerThread fails? try to run mach clobber and compile everything from scratch.
Attachment #8833418 -
Flags: review-
Assignee | ||
Comment 5•7 years ago
|
||
I can confirm this happens from a clean build, and Jacek and Geog got the same error. I can actually only find one other class that inherits nsThread, which is LazyIdleThread, inherited via nsIThread. LazyIdleThread defines RegisterIdlePeriod explicitly, so it is only WorkerThread that does not.
Comment 6•7 years ago
|
||
But why RegisterIdlePeriod needs to be defined explicitly, but not for example IdleDispatch. LazyThread is different. It inherits nsIThread, not nsThread. I wouldn't object taking the patch (assuming it is fixed to use Gecko coding style, so { to its own line), but would be great to understand why the patch is needed.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8833418 [details] Bug 1336527 Define RegisterIdlePeriod https://reviewboard.mozilla.org/r/109646/#review112376 Write a comment explaining why this is needed for mingw. Better if we understand why this is needed. ::: dom/workers/WorkerThread.h:80 (Diff revision 2) > already_AddRefed<WorkerRunnable> aWorkerRunnable); > > uint32_t > RecursionDepth(const WorkerThreadFriendKey& aKey) const; > > + NS_IMETHOD RegisterIdlePeriod(already_AddRefed<nsIIdlePeriod> aIdlePeriod) override { NS_IMETHOD RegisterIdlePeriod(already_AddRefed<nsIIdlePeriod> aIdlePeriod) override { return nsThread::RegisterIdlePeriod(already_AddRefed<nsIIdlePeriod>(aIdlePeriod.take())); }
Attachment #8833418 -
Flags: review- → review+
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
I'm still working to understand the why; although I've updated the patch to accordance to style guidelines. On a linux build: $ nm dom/workers/Unified_cpp_dom_workers2.o | grep RegisterIdlePeriod U _ZN8nsThread18RegisterIdlePeriodE16already_AddRefedI13nsIIdlePeriodE $ nm xpcom/threads/Unified_cpp_xpcom_threads0.o | grep RegisterIdlePeriod 00000000000076a2 T _ZN7mozilla14LazyIdleThread18RegisterIdlePeriodE16already_AddRefedI13nsIIdlePeriodE $ nm xpcom/threads/Unified_cpp_xpcom_threads1.o | grep RegisterIdlePeriod 00000000000042c2 T _ZN8nsThread18RegisterIdlePeriodE16already_AddRefedI13nsIIdlePeriodE $ nm dom/workers/Unified_cpp_dom_workers2.o | grep IdleDispatch U _ZN8nsThread12IdleDispatchE16already_AddRefedI11nsIRunnableE $ nm xpcom/threads/Unified_cpp_xpcom_threads0.o | grep IdleDispatch 000000000000768e T _ZN7mozilla14LazyIdleThread12IdleDispatchE16already_AddRefedI11nsIRunnableE $ nm xpcom/threads/Unified_cpp_xpcom_threads1.o | grep IdleDispatch 000000000000439e T _ZN8nsThread12IdleDispatchE16already_AddRefedI11nsIRunnableE On a mingw build. ../../dom/workers/Unified_cpp_dom_workers2.o:Unified_cpp_dom_workers2.cpp:(.rdata$_ZTVN7mozilla3dom7workers12WorkerThreadE[__ZTVN7mozilla3dom7workers12WorkerThreadE]+0x40): undefined reference to `nsThread::RegisterIdlePeriod(already_AddRefed<nsIIdlePeriod>)@4' $ nm dom/workers/Unified_cpp_dom_workers2.o | grep RegisterIdlePeriod U __ZN8nsThread18RegisterIdlePeriodE16already_AddRefedI13nsIIdlePeriodE@4 $ nm xpcom/threads/Unified_cpp_xpcom_threads1.o | grep RegisterIdlePeriod (nothing) $ nm xpcom/threads/Unified_cpp_xpcom_threads0.o | grep RegisterIdlePeriod 00003680 T __ZN7mozilla14LazyIdleThread18RegisterIdlePeriodE16already_AddRefedI13nsIIdlePeriodE@8 0000ada4 T __ZN8nsThread18RegisterIdlePeriodE16already_AddRefedI13nsIIdlePeriodE@8 $ nm dom/workers/Unified_cpp_dom_workers2.o | grep IdleDispatch U __ZN8nsThread12IdleDispatchE16already_AddRefedI11nsIRunnableE@8 $ nm xpcom/threads/Unified_cpp_xpcom_threads0.o | grep IdleDispatch 00003674 T __ZN7mozilla14LazyIdleThread12IdleDispatchE16already_AddRefedI11nsIRunnableE@8 0000ae0e T __ZN8nsThread12IdleDispatchE16already_AddRefedI11nsIRunnableE@8 $ nm xpcom/threads/Unified_cpp_xpcom_threads1.o | grep IdleDispatch (nothing) The fact that nsThread winds up in 0.o on mingw instead of 1.o on Linux seems weird, but at least it's there. The problem seems to be the @4 vs @8 suffix. Which I'm not familiar with and is difficult to search, but I'll keep investigating.
Comment 12•7 years ago
|
||
FWIW, @4 and @8 are part of mangled stdcall name. It usually means arguments size. I don't know why it's broken in this case.
Comment 13•7 years ago
|
||
I reproduced the problem with a simple test case proving that it's a gcc bug and filed a bug upstream: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79582
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Okay with the root cause figured out, I've updated the commit with formatting and comments. Can I tag this as checkin-needed ?
Flags: needinfo?(bugs)
Updated•7 years ago
|
Flags: needinfo?(bugs) → needinfo?(amarchesini)
Comment 16•7 years ago
|
||
Yes, please. Thanks for this patch, the filed bug on gcc and the comment.
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 17•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ca289b780399 Define RegisterIdlePeriod r=baku
Keywords: checkin-needed
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca289b780399
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #19) > Should we consider uplifting to 53 (and esr52?)? If we uplifted to -esr52 Tor would have to carry one less patch, which is advantageous. There's no reason to uplift to -53 other than to keep it in sync with 52.
Flags: needinfo?(tom)
Comment 21•7 years ago
|
||
Comment on attachment 8833418 [details] Bug 1336527 Define RegisterIdlePeriod [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: It fixes a build issue with mingw-w64. User impact if declined: The Tor Browser (or other projects using mingw-w64) would need to ship one more patch to get Firefox cross-compiled. Fix Landed on Version: 54 Risk to taking this patch (and alternatives if risky): none as far as I can see String or UUID changes made by this patch: non See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8833418 -
Flags: approval-mozilla-esr52?
Updated•7 years ago
|
Comment 22•7 years ago
|
||
Comment on attachment 8833418 [details] Bug 1336527 Define RegisterIdlePeriod work around gcc bug on mingw, esr52+
Attachment #8833418 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 23•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/b9f53baeabb3
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•