dist/include/GeckoProfiler.h:239:5: error: use of undeclared identifier 'MOZ_GUARD_OBJECT_NOTIFIER_INIT'

RESOLVED FIXED in Firefox 39

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Jan Beich, Assigned: Jan Beich)

Tracking

Trunk
mozilla40
All
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 unaffected, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.71 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
On platforms lacking SPS profiler (e.g. Linux/PPC, BSDs, Solaris) the build is broken likely due to header bootlegging. Injecting #error into mfbt/GuardObjects.h may explain how the header ends up being included there with SPS.

  In file included from netwerk/base/Unified_cpp_netwerk_base2.cpp:2:
  In file included from netwerk/base/nsInputStreamPump.cpp:8:
  In file included from netwerk/base/nsInputStreamPump.h:14:
  In file included from dist/include/mozilla/ReentrantMonitor.h:13:
  dist/include/GeckoProfiler.h:235:28: error: expected ')'
                             MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
                             ^
  dist/include/GeckoProfiler.h:233:27: note: to match this '('
    GeckoProfilerTracingRAII(const char* aCategory, const char* aInfo,
                            ^
  dist/include/GeckoProfiler.h:248:3: error: unknown type name
        'MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER'
    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
    ^
  dist/include/GeckoProfiler.h:249:3: error: expected member name or ';' after
        declaration specifiers
    const char* mCategory;
    ^
  dist/include/GeckoProfiler.h:236:7: error: member initializer 'mCategory' does
        not name a non-static data member or base class
      : mCategory(aCategory)
        ^~~~~~~~~~~~~~~~~~~~
  dist/include/GeckoProfiler.h:239:5: error: use of undeclared identifier
        'MOZ_GUARD_OBJECT_NOTIFIER_INIT'
      MOZ_GUARD_OBJECT_NOTIFIER_INIT;
      ^
  dist/include/GeckoProfiler.h:240:22: error: use of undeclared identifier
        'mCategory'; did you mean 'aCategory'?
      profiler_tracing(mCategory, mInfo, aBacktrace.release(), TRACING_INTERVA...
                       ^~~~~~~~~
                       aCategory
  dist/include/GeckoProfiler.h:233:40: note: 'aCategory' declared here
    GeckoProfilerTracingRAII(const char* aCategory, const char* aInfo,
                                         ^
  dist/include/GeckoProfiler.h:244:22: error: use of undeclared identifier
        'mCategory'
      profiler_tracing(mCategory, mInfo, TRACING_INTERVAL_END);
                       ^
  7 errors generated.

and only GeckoProfiler.h under tools/profiler/ is exported without SPS

  In file included from layout/generic/Unified_cpp_layout_generic0.cpp:47:
  In file included from layout/generic/StickyScrollContainer.cpp:16:
  layout/generic/../base/RestyleTracker.h:19:10: fatal error:
        'ProfilerBacktrace.h' file not found
  #include "ProfilerBacktrace.h"
           ^
  1 error generated.

but the header is required to avoid the following

  dist/include/mozilla/UniquePtr.h:488:19: error: invalid application of
        'sizeof' to an incomplete type 'ProfilerBacktrace'
      static_assert(sizeof(T) > 0, "T must be complete");
                    ^~~~~~~~~
  dist/include/mozilla/UniquePtr.h:308:7: note: in instantiation of member
        function 'mozilla::DefaultDelete<ProfilerBacktrace>::operator()' requested
        here
        getDeleter()(old);
        ^
  layout/generic/../base/RestyleTracker.h:399:22: note:
        in instantiation of member function 'mozilla::UniquePtr<ProfilerBacktrace,
        mozilla::DefaultDelete<ProfilerBacktrace> >::reset' requested here
        rd->mBacktrace.reset(profiler_get_backtrace());
                       ^
  dist/include/GeckoProfiler.h:101:7: note: forward declaration of
        'ProfilerBacktrace'
  class ProfilerBacktrace;
        ^
  1 error generated.
(Assignee)

Comment 1

3 years ago
Created attachment 8581152 [details] [diff] [review]
fix, v0

Not sure how much to comment out or provide a stub for |class ProfilerBacktrace|.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc359be5655
Attachment #8581152 - Flags: feedback?
(Assignee)

Updated

3 years ago
Attachment #8581152 - Flags: feedback? → feedback?(chung)
Comment on attachment 8581152 [details] [diff] [review]
fix, v0

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

Looks good to me.
Attachment #8581152 - Flags: feedback?(chung) → feedback+
Duplicate of this bug: 1149065
Comment on attachment 8581152 [details] [diff] [review]
fix, v0

Adding a different reviewer so that this hopefully lands..
Attachment #8581152 - Flags: review?(dholbert)
Comment on attachment 8581152 [details] [diff] [review]
fix, v0

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

::: layout/base/RestyleTracker.h
@@ +19,5 @@
>  #include "GeckoProfiler.h"
>  
> +#ifdef MOZ_ENABLE_PROFILER_SPS
> +#include "ProfilerBacktrace.h"
> +#endif

Can this just be a forward-declaration of "class ProfilerBacktrace"?

(I think it can? If so, I wouldn't bother with ifdefs around the forward-decl; and the #include can probably move to RestyleTracker.cpp, if we even need it there (we probably do).)

@@ +402,1 @@
>        rd->mBacktrace.reset(profiler_get_backtrace());

The #ifdef/#endif should move out one curly-brace level -- it should be outside the profiler_feature_active() call.
Comment on attachment 8581152 [details] [diff] [review]
fix, v0

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (I think it can? If so, I wouldn't bother with ifdefs around the
> forward-decl; and the #include can probably move to RestyleTracker.cpp, if
> we even need it there (we probably do).)

Locally, I can build with just the forward-decl -- no need to move the #include to RestyleTracker.cpp -- which makes sense because I think we just pass & receive ProfilerBacktrace pointers to & from profiler functions. We never actually do anything directly with the ProfilerBacktrace.

I'm willing to believe that I'm just getting saved by unified builds and/or some other attribute of my build environment, though.

r+ with both parts of comment 5 addressed (the first part ideally with just a forward-dec; if you need the #include, r=me still stands, but I'm curious where it ends up being necessary.)
Attachment #8581152 - Flags: review?(dholbert) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8586863 [details] [diff] [review]
fix, v1

(In reply to Daniel Holbert [:dholbert] from comment #5)
> > +#ifdef MOZ_ENABLE_PROFILER_SPS
> > +#include "ProfilerBacktrace.h"
> > +#endif
> 
> Can this just be a forward-declaration of "class ProfilerBacktrace"?

Let me quote the last error in comment 0:

  dist/include/mozilla/UniquePtr.h:488:19: error: invalid application of
        'sizeof' to an incomplete type 'ProfilerBacktrace'
      static_assert(sizeof(T) > 0, "T must be complete");
                    ^~~~~~~~~
  dist/include/mozilla/UniquePtr.h:308:7: note: in instantiation of member
        function 'mozilla::DefaultDelete<ProfilerBacktrace>::operator()' requested
        here
        getDeleter()(old);
        ^
  layout/generic/../base/RestyleTracker.h:399:22: note:
        in instantiation of member function 'mozilla::UniquePtr<ProfilerBacktrace,
        mozilla::DefaultDelete<ProfilerBacktrace> >::reset' requested here
        rd->mBacktrace.reset(profiler_get_backtrace());
                       ^
  dist/include/GeckoProfiler.h:101:7: note: forward declaration of
        'ProfilerBacktrace'
  class ProfilerBacktrace;
        ^
  1 error generated.

> (I think it can? If so, I wouldn't bother with ifdefs around the
> forward-decl; and the #include can probably move to RestyleTracker.cpp, if
> we even need it there (we probably do).)

In comment 1 by stub I meant to make smth like the following generic
and put under |#ifndef MOZ_ENABLE_PROFILER_SPS| in GeckoProfiler.h.

  #ifdef MOZ_ENABLE_PROFILER_SPS
      UniquePtr<ProfilerBacktrace> mBacktrace;
  #else
      UniquePtr<NULL> mBacktrace;
  #endif

> @@ +402,1 @@
> >        rd->mBacktrace.reset(profiler_get_backtrace());
> 
> The #ifdef/#endif should move out one curly-brace level -- it should be
> outside the profiler_feature_active() call.

Done in v1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db5d66a9e65

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Locally, I can build with just the forward-decl -- no need to move the
> #include to RestyleTracker.cpp -- which makes sense because I think we just
> pass & receive ProfilerBacktrace pointers to & from profiler functions. We
> never actually do anything directly with the ProfilerBacktrace.

Please, do a proper non-SPS build by modifying configure.in.
SPS bootlegs ProfilerBacktrace.h via GeckoProfiler.h.

Here's an example:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e47266fc83a3

  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_generic0.o] Error 1
  gmake[5]: *** Waiting for unfinished jobs....
  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_generic1.o] Error 1
  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_generic2.o] Error 1
  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_tables0.o] Error 1
Attachment #8581152 - Attachment is obsolete: true
Attachment #8586863 - Flags: review?(dholbert)
Comment on attachment 8586863 [details] [diff] [review]
fix, v1

Gotcha. I hadn't realized UniquePtr<Foo> requires Foo to be defined. (nsRefPtr / nsAutoPtr do not have that requirement, IIRC)

Thanks for catching & fixing the bustage!

r=me
Attachment #8586863 - Flags: review?(dholbert) → review+
m-i's closed, setting checkin-needed...
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4de1ed6e851d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 12

3 years ago
Comment on attachment 8586863 [details] [diff] [review]
fix, v1

Approval Request Comment
[Feature/regressing bug #]: bug 1129249 regression
[User impact if declined]: broken build on non-SPS platforms
[Describe test coverage new/current, TreeHerder]: m-c, m-i, Try in comment 7
[Risks and why]: Low: due to typos either broken build or new code from bug 1129249 being accidentally disabled for SPS platforms
[String/UUID change made/needed]: None
Attachment #8586863 - Flags: approval-mozilla-aurora?
status-firefox38: --- → unaffected
status-firefox39: --- → affected
Comment on attachment 8586863 [details] [diff] [review]
fix, v1

Approving this for 39 since it's a recent regression and looks like a low-risk fix.
Attachment #8586863 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
Blocks: 1151829
(Assignee)

Updated

3 years ago
Assignee: nobody → jbeich
You need to log in before you can comment on or make changes to this bug.