Closed Bug 855010 Opened 9 years ago Closed 9 years ago

Build bustage from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8), in breakpad header included by tools/profiler (which is FAIL_ON_WARNINGS)

Categories

(Core :: Gecko Profiler, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

GCC 4.8 adds -Wunused-local-typedefs to -Wall (and bug 853874 track adding it for other GCC versions, too).

Waldo fixed a bunch of instances of this warning in bug 851237. There's only one remaining case where it breaks our --enable-warnings-as-errors builds: in toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h, which has some typedefs-that-are-really-static-assertions called 'type_must_be_complete'.  They trigger these build warnings:
{
distcc[26356] ERROR: compile /scratch/work/builds/.ccache/tmp/local_debu.tmp.indigo.26353.ii on localhost failed
In file included from /mozilla/tools/profiler/local_debug_info_symbolizer.cc:12:0:
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h: In destructor 'google_breakpad::scoped_ptr<T>::~scoped_ptr()':
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h:74:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h: In member function 'void google_breakpad::scoped_ptr<T>::reset(T*)':
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h:79:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
In file included from /mozilla/tools/profiler/local_debug_info_symbolizer.cc:12:0:
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h: In destructor 'google_breakpad::scoped_array<T>::~scoped_array()':
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h:163:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h: In member function 'void google_breakpad::scoped_array<T>::reset(T*)':
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h:168:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h: In destructor 'google_breakpad::scoped_ptr_malloc<T, FreeProc>::~scoped_ptr_malloc()':
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h:257:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h: In member function 'void google_breakpad::scoped_ptr_malloc<T, FreeProc>::reset(T*)':
/mozilla/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h:262:18: error: typedef 'type_must_be_complete' locally defined but not used [-Werror=unused-local-typedefs]
     typedef char type_must_be_complete[sizeof(T)];
                  ^
}

This header is included by tools/profiler/local_debug_info_symbolizer.cc, and tools/profiler is FAIL_ON_WARNINGS (from bug 836150) -- so that's how these build-warnings in the header end up being treated as errors.

Filing this bug on addressing these.  One way we could do this would be to just use nsAutoPtr instead of scoped_ptr in this instance, I think (since from a superficial glance, they seem to be doing the same thing).
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Filing this bug on addressing these.  One way we could do this would be to
> just use nsAutoPtr instead of scoped_ptr in this instance, I think (since
> from a superficial glance, they seem to be doing the same thing).

I think this is cleanest way to fix this. Moving this to "profiler" component, accordingly, since that's where that code-change will happen. Patch coming up.
Component: Breakpad Integration → Gecko Profiler
Product: Toolkit → Core
Version: unspecified → Trunk
Attached patch fixSplinter Review
One might argue that we *should* be using google_breakpad::scoped_ptr here, since this code is in "namespace google_breakpad"

However, in response to that, I'd say:
 (a) google_breakpad::scoped_ptr doesn't have any breakpad-specific magic about it -- it actually appears to just be the Boost scoped_ptr which has been copypasted into the breakpad code[1].

 (b) the scoped_ptr / nsAutoPtr functionality is pretty simple, so it really doesn't matter much which one we use. So, we might as well use the one that doesn't spam build warnings.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/scoped_ptr.h

(This patch includes a comment saying that we might want to switch back to scoped_ptr after its build warning issues are resolved.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #729728 - Flags: review?(bgirard)
Attachment #729728 - Flags: review?(bgirard) → review+
Try run (builds only), as a sanity-check: https://tbpl.mozilla.org/?tree=Try&rev=26baff114b69
That should be fine, local_debug_info_symbolizer only used scoped_ptr because it's written like Breakpad code, but it's actually profiler code.
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/d32ba4b1d746
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.