983957 - toolkit/components/telemetry/Telemetry.cpp: undefined reference to `mozilla::IOInterposeObserver::IsMainThread()' (non-SPS)

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Jan Beich]

On platforms without SPS profiler the build is broken because xpcom doesn't provide IOInterposeObserver.

../components/telemetry/Telemetry.o: In function `(anonymous namespace)::TelemetryIOInterposeObserver::Observe(mozilla::IOInterposeObserver::Observation&)':
toolkit/components/telemetry/Telemetry.cpp:(.text._ZN12_GLOBAL__N_128TelemetryIOInterposeObserver7ObserveERN7mozilla19IOInterposeObserver11ObservationE+0x1b): undefined reference to `mozilla::IOInterposeObserver::IsMainThread()'
/usr/local/bin/ld: libxul.so: hidden symbol `_ZN7mozilla19IOInterposeObserver12IsMainThreadEv' isn't defined

Comment 1

[Jan Beich]

Attachment: fallback to NS_IsMainThread()

Simple approach than trying to ifdef out TelemetryIOInterposeObserver.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8391671 [details] [diff] [review]
fallback to NS_IsMainThread()

Review of attachment 8391671 [details] [diff] [review]:
-----------------------------------------------------------------

The patch makes sense, but we should actually be going further than that. The entire IOInterposer infrastructure is not present in a non-SPS build, so in that case TelemetryIOInterposeObserver becomes just a bunch of dead code. I think that we should #ifdef out all of TelemetryIOInterposeObserver and anything else in there that refrences it.

Comment 3

[Jan Beich]

Attachment: Build IOInterposer and NSPRInterposer on non-SPS platforms.

I disagree. Why expose the header if it cannot be used? Either compilation should fail due to missing header or non-SPS stub in there is not enough.

OTOH, IOInterposer.cpp has no profiler or stack-unwinding references.

Here's an alternative fix for my issue. Do I need to do the same for PoisonIOInterposer? It a stub on Linux even with SPS.

Comment 4

[Jan Beich]

Attachment: Build PoisonIOInterposer on non-SPS.

With this as well the only XPCOM places using SPS ifdef are ThreadStackHelper and profiler cleanup in ShutdownXPCOM.

Comment 5

[Jan Beich]

Attachment: Remove redundant ifdef for PoisonIOInterposer.

Linux already has the following stub

  inline void InitPoisonIOInterposer(){}

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jan Beich from comment #3)
> I disagree. Why expose the header if it cannot be used? Either compilation
> should fail due to missing header or non-SPS stub in there is not enough.
I think that the developer who wrote the first cut of those changes felt that it was cleaner to put the stubs in the header instead of having to #ifdef every single call to a function declared by that header.

> 
> OTOH, IOInterposer.cpp has no profiler or stack-unwinding references.

We didn't hide it behind MOZ_ENABLE_PROFILER_SPS because of any code dependency on SPS; we hid it behind that define because we felt that there was significant overlap between build configurations that would want profiling and build configurations that would want IOInterposer.

(A) We should be able to build Gecko without IOInterposer;

(B) Enabling SPS in a build config should implicitly enable IOInterposer;

(C) OTOH, while it is perfectly reasonable to want to enable IOInterposer without SPS, I don't think that we felt that there was a strong enough need for that use case to implement a new, separate configure option. For my team's purposes, having them both tied to MOZ_ENABLE_PROFILER_SPS is fine. If you feel differently, then by all means feel free to implement a new build config option.

Comment 7

[Jonas Finnemann Jensen (:jonasfj)]

> I think that the developer who wrote the first cut of those changes felt that it was cleaner to put
> the stubs in the header instead of having to #ifdef every single call to a function declared by
> that header.
You are correct, that is exactly what the developer **felt** :)
For functions like `Mozilla(Un|)RegisterDebug(File|FD)` stub implementations makes a lot of sense.

But as the IOInterposer.h have gotten more complicated, I believe arguments can be made for and
against stubs. Not defining the symbols is more explicit and perhaps easier to debug.
OTOH, things like `IOInterposeObserver::Observation` could have a stub implementation, but this
is less important as the places using it are all #ifdef'ed (or not compiled without SPS).

> (C) OTOH, while it is perfectly reasonable to want to enable IOInterposer without SPS, I don't think
> that we felt that there was a strong enough need for that use case to implement a new, separate
> configure option.
I'm not sure there was deep discussion on this. But having had so many problems where we broke
case (A) building gecko without SPS. I would strongly advice against adding another build
configuration option.

Note, that when we break non-SPS builds it is not caught by automated testing. And doing an
extra automated build to test this on every push, would be expensive.


On topic:
Clearly, the solution is to either stub `IOInterposeObserver::IsMainThread` (make an inline that
calls `NS_IsMainThread`) or start #ifdef'ing things out. It might be more explicit and easier to
maintain considering that a proper stub for `IOInterposeObserver::Observation` could be complicated.
But I would consider using stubs for simple things like `Mozilla(Un|)RegisterDebug(File|FD)`.

Comment 8

[Landry Breuil (:gaston)]

Either way, can we get a proper fix commited ? builds are broken on non-SPS platforms and that might hide other failures piling on top....

Comment 9

[Jan Beich]

Attachment: remove SPS ifdefs

rebased after bug 973353 and combined

Comment 10

[Jan Beich]

Attachment: decouple from SPS ifdef

ifdef'ing out IOInterposer in Telemetry.cpp et al. and removing the stub may come after. This patch only adds the option and switches from SPS ifdef.

Sorry, my C++ level is too low to make the following work

  static inline bool IsMainThread()            { return NS_IsMainThread(); }

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #7)
> > (C) OTOH, while it is perfectly reasonable to want to enable IOInterposer without SPS, I don't think
> > that we felt that there was a strong enough need for that use case to implement a new, separate
> > configure option.
> I'm not sure there was deep discussion on this. But having had so many
> problems where we broke
> case (A) building gecko without SPS. I would strongly advice against adding
> another build
> configuration option.

SPS is not the only source of problems for Tier3 platforms e.g., --disable-ion is broken again (bug 983513). Adding (A) or more non-TBPL baggage for them would only increase maintenance burden.

I'll try to do (C) once and forget about IOInterposer existence.

Comment 11

[Jonas Finnemann Jensen (:jonasfj)]

(In reply to Jan Beich from comment #10)
> SPS is not the only source of problems for Tier3 platforms e.g.,
> --disable-ion is broken again (bug 983513). Adding (A) or more non-TBPL
> baggage for them would only increase maintenance burden.
Agree, build configurations not covered by TBPL will break constantly.

> I'll try to do (C) once and forget about IOInterposer existence.
Maybe I misunderstand you, but is there a **very** good use-case for this?

My argument was that we should keep SPS and IOInterposer under the same flag (MOZ_ENABLE_PROFILER_SPS).
As not doing this, adds another build configuration that isn't covered on TBPL, and hence, likely to
break whenever somebody is working on something related.

Note, when I worked on IOInterposer I did test without MOZ_ENABLE_PROFILER_SPS, adding yet another
configuration would make this kind of manual testing even harder. And to be honest I did managed to
briefly break non-SPS builds with a last minute change, despite manual testing.

Comment 12

[Jan Beich]

Comment on attachment 8392045 [details] [diff] [review]
decouple from SPS ifdef

Nevermind. The idea was to let IOInteposer owners maintain ifdefs for it in order to reduce work for non-SPS platform maintainers. It turns out this is neither desired nor following up to get rid of stubs is trivial.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8392044 [details] [diff] [review]
remove SPS ifdefs

Review of attachment 8392044 [details] [diff] [review]:
-----------------------------------------------------------------

I'm okay with this patch for the most part. I'd like to see one more revised version before granting review.

::: xpcom/build/IOInterposer.h
@@ +262,5 @@
>  {
>  public:
>    IOInterposerInit()
>    {
>      IOInterposer::Init();

Can you please wrap the call to IOInterposer::Init() with an #if defined(MOZ_ENABLE_PROFILER_SPS)? I think that this is one place where we should have it.

::: xpcom/build/moz.build
@@ +27,5 @@
>  if CONFIG['OS_ARCH'] == 'WINNT':
>      EXPORTS += ['nsWindowsDllInterceptor.h']
>      EXPORTS.mozilla += ['perfprobe.h']
>      SOURCES += ['perfprobe.cpp']
> +    SOURCES += [

Nit: Now there are two statements appending entries to SOURCES. Can you please combine these two into a single statement?

@@ +54,5 @@
>      'Services.cpp',
>  ]
>  
> +SOURCES += [
> +    'IOInterposer.cpp',

This file should be able to be moved to UNIFIED_SOURCES now that it is compiled unconditionally.

Comment 14

[Andrew Halberstadt [:ahal]]

Is whether the profiler gets enabled something that's set per branch? I'm seeing this on Ash:
https://tbpl.mozilla.org/?tree=Ash&rev=f5bbca24993c

Comment 15

[Landry Breuil (:gaston)]

Now probably affects aurora since the merge was done..

Comment 16

[Andrew Halberstadt [:ahal]]

Nvm, looks like the push before mine disabled the profiler for some reason.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: remove SPS ifdefs

Given that these nits are trivial, I'm just going to fix them and r+.

I've already built locally without --enable-profiling and posted https://tbpl.mozilla.org/?tree=Try&rev=8b589f1f0f7e to try with --enable-profiling.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

https://tbpl.mozilla.org/?tree=Try&rev=8b589f1f0f7e

https://hg.mozilla.org/integration/mozilla-inbound/rev/440ab7452b1e

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8392567 [details] [diff] [review]
remove SPS ifdefs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 972577
User impact if declined: Build failures on non-SPS platforms.
Testing completed (on m-c, etc.): built on try with SPS on, locally with SPS off
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/440ab7452b1e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af2b86deaf5d