1151829 - dist/include/GeckoProfiler.h:56:31: fatal error: ProfilerBacktrace.h: No such file or directory (non-SPS)

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jan Beich]

On platforms lacking SPS profiler (Linux/PPC, BSDs, Solaris) the build is broken because only GeckoProfiler.h is exported under tools/profiler/.

In file included from uriloader/prefetch/OfflineCacheUpdateChild.cpp:6:
In file included from uriloader/prefetch/OfflineCacheUpdateChild.h:9:
In file included from ipc/ipdl/_ipdlheaders/mozilla/docshell/POfflineCacheUpdateChild.h:14:
In file included from dist/include/mozilla/ipc/MessageChannel.h:15:
In file included from dist/include/mozilla/Monitor.h:10:
In file included from dist/include/mozilla/CondVar.h:16:
dist/include/GeckoProfiler.h:56:10: fatal error: 'ProfilerBacktrace.h' file
      not found
#include "ProfilerBacktrace.h"
         ^

1 error generated.

Comment 1

[Landry Breuil (:gaston)]

A new one ? oh come on :(

Comment 2

[Randall Barker [:rbarker]]

I guess this was caused by my recent patch for bug 1093934. The problem I hit was that the Windows build failed because for mozilla::UniquePtr<ProfilerBacktrace>, ProfilerBacktrace was only forward declared. Adding ProfilerBacktrace.h was the only way to get the Windows build on try to pass.

Comment 3

[Jan Beich]

Why not extend bug 1145988 to include MOZILLA_XPCOMRT_API case and backout GeckoProfiler.h change of #includes?

Comment 4

[Jan Beich]

Attachment: fix, v0

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a265a6a88281

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84ebfdd3c94c

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8fead3b81ebf due to Windows build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=8675979&repo=mozilla-inbound

Comment 7

[Daniel Holbert [:dholbert]]

Per comment 0, the original issue here is coming from GeckoProfiler.h being #included by CondVar.h. (regardless of whether profiling is enabled in our build)

Perhaps CondVar.h's profiler stuff should be wrapped in "#ifdef MOZ_ENABLE_PROFILER_SPS"?

Comment 8

[Daniel Holbert [:dholbert]]

(Then we wouldn't have to remove the ProfilerBacktrace.h include from GeckoProfiler.h, and that means we wouldn't trigger the error in comment 6.)

Comment 9

[Jan Beich]

Attachment: fix, v1

How am I supposed to test the fix if comment 4 didn't catch Windows bustage? Trying again in hope --enable-debug build makes a difference:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be40e86118c7

MSVC seems to be picky about |Maybe| unused class. UniquePtr<> non-SPS regression while exposed by bug 1093934 actually comes from bug 1129249. Let's define GeckoProfilerTracingRAII only with GeckoProfilerImpl.h which already includes ProfilerBacktrace.h.

Carrying over r=rbarker from v0. Changes added in v1 need a reviewer from bug 1145988.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Per comment 0, the original issue here is coming from GeckoProfiler.h being
> #included by CondVar.h. (regardless of whether profiling is enabled in our
> build)

GeckoProfiler isn't tied to --enable-profiling. And namespace pollution in SPS case under tools/profiler/ isn't something new. So, non-SPS riding on standalone XPCOM testing is actually an improvement given how often it breaks (about every month for the past 2 years).

> Perhaps CondVar.h's profiler stuff should be wrapped in "#ifdef MOZ_ENABLE_PROFILER_SPS"?

Stubs within GeckoProfiler.h are exactly to avoid such a strawman fix. CondVar.h isn't the only header using the profiler.

https://dxr.mozilla.org/mozilla-central/search?q=GeckoProfiler.h+ext%3Ah

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (Then we wouldn't have to remove the ProfilerBacktrace.h include from
> GeckoProfiler.h, and that means we wouldn't trigger the error in comment 6.)

ProfilerBacktrace.h is never exported in non-SPS case. If we're going to use SPS headers then conditionals like the following don't make sense:

  #if !defined(MOZ_ENABLE_PROFILER_SPS) || defined(MOZILLA_XPCOMRT_API)

or

  #if defined(MOZ_ENABLE_PROFILER_SPS) && !defined(MOZILLA_XPCOMRT_API)

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Jan Beich from comment #9)
> Created attachment 8590554 [details] [diff] [review]
> fix, v1
> 
> How am I supposed to test the fix if comment 4 didn't catch Windows bustage?

That is indeed strange. Maybe we just needed a clobber build here, for some reason? (That's one key difference between m-i vs. Try -- Try is always clobber-build, m-i is rarely clobber-build.) I don't see why we would, just spitballing...

RE the updated patch: I'm happy to sign off on moving the Maybe<> variables inside the #ifdefs, if it helps things here.

I'm uncomfortable with dropping the #includes, though. (Particularly the GuardObjects one; I don't see that include causing any trouble here, and we should include it since we're using stuff from it.  I'm curious about the ProfilerBacktrace.h one, too.)

Maybe this would all be cleaner if GeckoProfilerTracingRAII moved into its own header, which could only be included by files that use it?  Then GeckoProfiler.h could legitimately drop these #includes.

Comment 11

[Jan Beich]

Attachment: fix, v2

Carrying over r=rbarker from v0. Changes added in this version need
the same reviewer from bug 1145988.

(In reply to Daniel Holbert [:dholbert] from comment #10)
> I'm uncomfortable with dropping the #includes, though. (Particularly the
> GuardObjects one; I don't see that include causing any trouble here, and we
> should include it since we're using stuff from it.

Bug 1145988 already added mozilla/GuardObjects.h. Do you want a dup?

> Maybe this would all be cleaner if GeckoProfilerTracingRAII moved into its own
> header, which could only be included by files that use it?  Then
> GeckoProfiler.h could legitimately drop these #includes.

Why not GeckoProfilerImpl.h? It may make SPS header bootlegging slightly
worse given the number of places GeckoProfiler.h is included in but not
worse than before bug 1145988.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5b3b7a7b96

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8590593 [details] [diff] [review]
fix, v2

(In reply to Jan Beich from comment #11)
> Bug 1145988 already added mozilla/GuardObjects.h. Do you want a dup?

I'm not quite sure what you're asking here, but it doesn't matter anymore, because:
 - I now see that there were actually two GuardObjects.h #includes there, and you were simply removing the second one. (not removing the include entirely)
 - It doesn't matter anymore, now that the guard-object-using code is moving to a different file.

> > Maybe this would all be cleaner if GeckoProfilerTracingRAII moved into its own
> > header
[...] 
> Why not GeckoProfilerImpl.h?

Seems reasonable to me!

One nit, though:

>+++ b/tools/profiler/GeckoProfilerImpl.h
>+class MOZ_STACK_CLASS GeckoProfilerTracingRAII {
>+public:
[...]
>+};
>+
> namespace mozilla {
> 
> class MOZ_STACK_CLASS SamplerStackFrameRAII {
> public:

GeckoProfilerTracingRAII should go inside 'namespace mozilla' here, for consistency.

This namespacing shouldn't break any of the usages, because this class is only used in RestyleTracker.cpp, inside of a 'namespace mozilla { ... }' scope.

Also: This GeckoProfiler.h --> GeckoProfilerImpl.h code-moving should get sign-off from someone who actually works on profiler stuff. Tagging mstange.

Comment 13

[Jan Beich]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> GeckoProfilerTracingRAII should go inside 'namespace mozilla' here, for
> consistency.

What about consistency with other GeckoProfiler*RAII in GeckoProfiler.h?
GeckoProfiler* classes already have hints of their origin as part of the names
just like mozilla_sampler_* functions. Besides, 'namespace mozilla' doesn't
work as a generic way to denote SPS stuff e.g., see ProfilerBacktrace.h
on which the moved GeckoProfilerTracingRAII relies.

Comment 14

[Daniel Holbert [:dholbert]]

I can't speak to GeckoProfiler coding conventions, or why GeckoProfiler has some namespaced and non-namespaced stuff.   But if it's moving to GeckoProfilerImpl.h, it seems like it should match the local style in that file.  Also, general mozilla style is to use "namespace mozilla"[1], so when there's ambiguity, better to err on the side of that.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Comment 15

[Jan Beich]

Attachment: fix, v2.1

Moved GeckoProfilerTracingRAII under 'namespace mozilla' per comment 14.
If :mstange disagrees one can easily swap obsolete flag.

Carrying over r=rbarker from v0 and r=dholbert from v2.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6871ab383630

Comment 16

[Jan Beich]

Oops, unrelated --enable-debug bustage in the previous Try build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2321f13d5c2

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8591303 [details] [diff] [review]
fix, v2.1

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

This seems fine. I don't know the history of why some things are namespaced and some not.

Comment 18

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8f93de22cb

Comment 19

[Carsten Book [:Tomcat]]

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