Closed Bug 1336527 Opened 3 years ago Closed 3 years ago

mingw link step fails with undefined reference to RegisterIdlePeriod

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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 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-
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.
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 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+
Flags: needinfo?(amarchesini)
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.
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.
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
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)
Flags: needinfo?(bugs) → needinfo?(amarchesini)
Yes, please. Thanks for this patch, the filed bug on gcc and the comment.
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ca289b780399
Define RegisterIdlePeriod r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca289b780399
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Should we consider uplifting to 53 (and esr52?)?
Flags: needinfo?(tom)
(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 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?
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.