1576263 - modules/libpref/Preferences.cpp:17:10: fatal error: 'ProfilerMarkerPayload.h' file not found

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Jan Beich]

GeckoProfiler is not supported on Tier3 platforms: Linux powerpc/sparc/s390x, DragonFly/FreeBSD/NetBSD/OpenBSD, Solaris. Until bug 1371159 reproducing on Tier1 requires hacking gecko_profiler() in toolkit/moz.configure to return None.

$ c++ --version
FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on LLVM 8.0.1)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ ./mach bootstrap
$ ./mach build
[...]
In file included from objdir/modules/libpref/Unified_cpp_modules_libpref0.cpp:2:
modules/libpref/Preferences.cpp:17:10: fatal error: 'ProfilerMarkerPayload.h' file not found
#include "ProfilerMarkerPayload.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Comment 1

[Will Hawkins]

Is there a compile-time constant/macro that you recommend using to make sure that we don't include this functionality on these platforms? Sorry about the breakage.

Comment 2

[Will Hawkins]

Looks like I can use MOZ_GECKO_PROFILER. Does that jive with your understanding, Jan?

Comment 3

[Jan Beich]

(In reply to Will Hawkins from comment #2)

Looks like I can use MOZ_GECKO_PROFILER. Does that jive with your understanding, Jan?

Yes. Also, after bug 1403868 some macro wrappers can be used unconditionally.

Comment 4

[Will Hawkins]

Attachment: Bug 1576263: Conditionally include profiler functionality only where it's supported.

Comment 5

[Jan Beich]

Not enough to fix --enable-warnings-as-errors yet. Sorry, I can't log in (to review) on Phabricator due to bug 1536716.

In file included from objdir/modules/libpref/Unified_cpp_modules_libpref0.cpp:2:
modules/libpref/Preferences.cpp:4330:13: error: unused function 'PrefValueToString' [-Werror,-Wunused-function]
static void PrefValueToString(const bool* b, nsCString& value) {
            ^
modules/libpref/Preferences.cpp:4333:13: error: unused function 'PrefValueToString' [-Werror,-Wunused-function]
static void PrefValueToString(const int* i, nsCString& value) {
            ^
modules/libpref/Preferences.cpp:4336:13: error: unused function 'PrefValueToString' [-Werror,-Wunused-function]
static void PrefValueToString(const uint32_t* u, nsCString& value) {
            ^
modules/libpref/Preferences.cpp:4339:13: error: unused function 'PrefValueToString' [-Werror,-Wunused-function]
static void PrefValueToString(const float* f, nsCString& value) {
            ^
modules/libpref/Preferences.cpp:4342:13: error: unused function 'PrefValueToString' [-Werror,-Wunused-function]
static void PrefValueToString(const nsACString& s, nsCString& value) {
            ^
5 errors generated.

Comment 6

[Will Hawkins]

Let me know if that updated patch is better :-) Sorry about earlier!

Comment 7

[Jan Beich]

Comment on attachment 9087873 [details]
Bug 1576263: Conditionally include profiler functionality only where it's supported.

Builds fine on FreeBSD.

Comment 8

[Will Hawkins]

Thank you for confirming, Jan!

Comment 9

[Cameron Kaiser [:spectre]]

This also fixes it on Linux on ppc64le.

Comment 10

[Cameron Kaiser [:spectre]]

Can this land? We're broken on beta without it.

Comment 11

[Markus Stange [:mstange]]

I'll press the button.

Comment 12

[Markus Stange [:mstange]]

Oh, never mind, it's not been r+ed yet. I thought njn had already reviewed it in my absence.

Comment 13

[Will Hawkins]

Yeah, it would be great to have Nick sign off.

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1576263 - Conditionally include profiler functionality only where it's supported. r=whawkins,njn

Comment 15

[Markus Stange [:mstange]]

Will, I've attached an alternative patch that only has one #ifdef per lookup function instead of three. Please take a look and let me know what you think of it. This patch is only adding markers when pref_Lookup/pref_SharedLookup succeeds, so it assumes that lookup failures only happen in unexpected error cases and are not relevant for general performance profiling.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c01f03f68def
Conditionally include profiler functionality only where it's supported. r=whawkins,njn

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c01f03f68def

Comment 18

[Jan Beich]

Can you backport to Beta 70 as well, chasing bug 1551313?

Comment 19

[Markus Stange [:mstange]]

I expect this to apply cleanly on 70. I'll request uplift approval.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 9089787 [details]
Bug 1576263 - Conditionally include profiler functionality only where it's supported. r=whawkins,njn

Beta/Release Uplift Approval Request

• User impact if declined: Tier3 platforms have broken builds. No user impact for Firefox on Tier1 platforms.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Extremely low risk. It's pretty much only a build fix. There is one small behavior change but it only affects whether a profiler marker is emitted in a certain error case.
• String changes made/needed:

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9089787 [details]
Bug 1576263 - Conditionally include profiler functionality only where it's supported. r=whawkins,njn

Mostly a build fix; we're in very early beta so I'm fine with this uplift to support Tier 3

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dfafac0c57eb