Closed Bug 983957 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Telemetry, defect)

x86_64
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 6 obsolete files)

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
Blocks: 972577
Depends on: 902587
Attached patch fallback to NS_IsMainThread() (obsolete) — Splinter Review
Simple approach than trying to ifdef out TelemetryIOInterposeObserver.
Attachment #8391671 - Flags: review?(aklotz)
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.
Attachment #8391671 - Flags: review?(aklotz) → review-
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.
Attachment #8391671 - Attachment is obsolete: true
Attachment #8391686 - Flags: review?(aklotz)
With this as well the only XPCOM places using SPS ifdef are ThreadStackHelper and profiler cleanup in ShutdownXPCOM.
Attachment #8391693 - Flags: review?(aklotz)
Linux already has the following stub

  inline void InitPoisonIOInterposer(){}
Attachment #8391698 - Flags: review?(aklotz)
(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.
> 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)`.
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....
Attached patch remove SPS ifdefs (obsolete) — Splinter Review
rebased after bug 973353 and combined
Attachment #8391686 - Attachment is obsolete: true
Attachment #8391693 - Attachment is obsolete: true
Attachment #8391698 - Attachment is obsolete: true
Attachment #8391686 - Flags: review?(aklotz)
Attachment #8391693 - Flags: review?(aklotz)
Attachment #8391698 - Flags: review?(aklotz)
Attachment #8392044 - Flags: review?(aklotz)
Attached patch decouple from SPS ifdef (obsolete) — Splinter Review
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.
Attachment #8392045 - Flags: review?(aklotz)
(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 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.
Attachment #8392045 - Attachment is obsolete: true
Attachment #8392045 - Flags: review?(aklotz)
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.
Attachment #8392044 - Flags: review?(aklotz) → review-
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
Now probably affects aurora since the merge was done..
Nvm, looks like the push before mine disabled the profiler for some reason.
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.
Attachment #8392044 - Attachment is obsolete: true
Attachment #8392567 - Flags: review+
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
Attachment #8392567 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/440ab7452b1e
Assignee: nobody → jbeich
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Attachment #8392567 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.