Closed Bug 856566 Opened 7 years ago Closed 5 years ago

Fix crashreporter compilation and profiler on mingw.

Categories

(Toolkit :: Crash Reporting, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(4 files, 3 obsolete files)

Attached patch fix (obsolete) — Splinter Review
I started looking at profiler compile failure and while I was at this, I fixed compilation of whole crashreporter. Upstream changes are already submited for review:

http://breakpad.appspot.com/548002/

This bug is for m-c changes. The attached patch includes:

- Change to configure.in to allow cross compiling crash reporter for Windows
- Annonymous namespace and invalid cast fixes
- Using std::max instead of max macro
- Workaround for lack of wchar_t-based functions in *fstream classes. Filenames are converted to current codepage from UTF-8, which AFAIK requires conversion to unicode on Windows first.
- Support for DummyEntryPoint entry point on LD.
- Support for mingw in PlatformMacros.h
- Some include lowercasing.
Attachment #731811 - Flags: review?(ted)
Comment on attachment 731811 [details] [diff] [review]
fix

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

::: configure.in
@@ +6039,1 @@
>      AC_MSG_ERROR([Breakpad tools do not support compiling on $HOST_OS_ARCH while targeting $OS_ARCH.  Use --disable-crashreporter.])

I guess this makes sense, because we don't actually build any host tools for the Win32 build.

::: toolkit/crashreporter/LoadLibraryRemote.cpp
@@ -21,5 @@
>  #endif
>  
>  #include "nsWindowsHelpers.h"
>  
> -namespace {

What's the reason for removing this?

::: toolkit/crashreporter/client/crashreporter_win.cpp
@@ +1428,5 @@
>    ifstream* file = new ifstream(_wfopen(UTF8ToWide(filename).c_str(), L"r"));
> +#else   // GCC
> +  char name[MAX_PATH];
> +  WideCharToMultiByte(CP_ACP, 0, UTF8ToWide(filename).c_str(), -1, name,
> +                      sizeof(name), NULL, NULL);

You can be a little fancier here and use a std::vector<char> to hold the name, and call WideCharToMultiByte twice to get the size the first time.

Something like this:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/common/windows/string_utils.cc#47

Maybe just define a UTF8ToMBCS function like we have UTF8ToWide above?

::: toolkit/crashreporter/injector/injector.cpp
@@ +13,1 @@
>                              DWORD reason,

Fix the indentation here.
Attachment #731811 - Flags: review?(ted)
Thanks for the review.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> ::: toolkit/crashreporter/LoadLibraryRemote.cpp
> @@ -21,5 @@
> >  #endif
> >  
> >  #include "nsWindowsHelpers.h"
> >  
> > -namespace {
> 
> What's the reason for removing this?

I get following errors otherwise:

In file included from ../../dist/include/nsWindowsHelpers.h:9:0,
                 from /home/jacek/mozilla/mozilla-central/toolkit/crashreporter/LoadLibraryRemote.cpp:23:
../../dist/include/nsAutoRef.h:435:26: error:   from definition of ‘template<class T> class nsAutoRefTraits’ [-fpermissive]
/home/jacek/mozilla/mozilla-central/toolkit/crashreporter/LoadLibraryRemote.cpp:34:24: error: definition of ‘static const unsigned char* nsAutoRefTraits<const unsigned char*>::Void()’ is not in namespace enclosing ‘nsAutoRefTraits<const unsigned char*>’ [-fpermissive]
/home/jacek/mozilla/mozilla-central/toolkit/crashreporter/LoadLibraryRemote.cpp:39:35: error: definition of ‘static void nsAutoRefTraits<const unsigned char*>::Release(nsAutoRefTraits<const unsigned char*>::RawRef)’ is not in namespace enclosing ‘nsAutoRefTraits<const unsigned char*>’ [-fpermissive]

My understanding is that unnamed/annonymous namespace is not considered the same thing as global namespace. I may get better understanding if you'd like.

> ::: toolkit/crashreporter/client/crashreporter_win.cpp
> @@ +1428,5 @@
> >    ifstream* file = new ifstream(_wfopen(UTF8ToWide(filename).c_str(), L"r"));
> > +#else   // GCC
> > +  char name[MAX_PATH];
> > +  WideCharToMultiByte(CP_ACP, 0, UTF8ToWide(filename).c_str(), -1, name,
> > +                      sizeof(name), NULL, NULL);
> 
> You can be a little fancier here and use a std::vector<char> to hold the
> name, and call WideCharToMultiByte twice to get the size the first time.
> 
> Something like this:
> http://code.google.com/p/google-breakpad/source/browse/trunk/src/common/
> windows/string_utils.cc#47
> 
> Maybe just define a UTF8ToMBCS function like we have UTF8ToWide above?

I used a bit different approach. I created a new, more generic function WideToMBCP (which takes a code page to convert to) based on WideToUTF8 and made WideToUTF8 a thin wrapper. This way there is no mingw-only helper and it's good enough for *fstream purposes to look nicer. If this gets r+, I will change the Breakpad patch to use similar approach.

> ::: toolkit/crashreporter/injector/injector.cpp
> @@ +13,1 @@
> >                              DWORD reason,
> 
> Fix the indentation here.

Fixed.
Attached patch fix v1.1Splinter Review
Attachment #732889 - Attachment is patch: true
Attachment #732889 - Flags: review?(ted)
Attachment #731811 - Attachment is obsolete: true
Attachment #732889 - Flags: review?(ted) → review+
Thanks, pushed to m-i:

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

Let's leave this bug open to track required upstream changes.
Whiteboard: [leave open]
My build keeps falling over at:

c:/t1/hg/comm-central/mozilla/toolkit/crashreporter/client/crashreporter_win.cpp(1227) : error C2065: 'nullptr' : undeclared identifier
Ms2ger says to add:
#include "mozilla/NullPtr.h"

And this seems to work for me. Patch coming up.
This patch allows me to build successfully with VS2008SP1 / VC9
Attachment #734220 - Flags: review?(ted)
Attachment #734220 - Flags: review?(ted) → review+
Comment on attachment 734220 [details] [diff] [review]
Fix for: error C2065: 'nullptr' : undeclared identifier

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/09ff3bd0beb8
Ted, could we please revisit this bug? This is a patch that sits in my tree (with occasional changes) for about 1.5 year now. My attempt to fix upstream failed waiting in the queue [1]. Meantime, someone else wrote a more complete mingw port [2], but it also seems to be mostly ignored by upstream. Also, breakpad used in Mozilla wasn't updated for over 1.5 years, so my local patch no longer aplies upstream.

In this situation, could we commit my patch to Mozilla. I'm fine with the fact that it will break if it's updated to upstream at some point. I will rework it then.

[1] https://breakpad.appspot.com/548002/
[2] https://github.com/jon-turney/google-breakpad
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #731812 - Attachment is obsolete: true
Attachment #8500329 - Flags: review?(ted)
Comment on attachment 8500329 [details] [diff] [review]
patch.diff

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

I'm sorry this has sat around so long. I have a hard time keeping track of Breakpad patches, Rietveld is terrible for that. This patch is generally fine, there are just a few minor things that I'd like to see fixed. Once you've fixed those I'll land it upstream for you as well.

::: toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/minidump_generator.cc
@@ +176,4 @@
>  
> +  AVRF_HANDLE_OPERATION* data_iter = reinterpret_cast<AVRF_HANDLE_OPERATION*>(stream_data + 1);
> +  for(i = operations_.begin(); i != i_end; i++)
> +      *data_iter++ = *i;

This is sort of unfortunate. I think all stdext::checked_array_iterator does here is silence a warning in MSVC, since this array is guaranteed to be the same length as operations_.

This code violates some Google style guidelines, though. First, you need braces around the body of the for. Second, split the assignment and the increment into two lines (or maybe put the increment up with the other for loop increment). Also, nitpicky, but you want a space after 'for'.

::: toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc
@@ +822,5 @@
>    string id_no_dash;
>    for (int i = 0; identifier_str[i] != '\0'; ++i)
>      if (identifier_str[i] != '-')
>        id_no_dash += identifier_str[i];
> +  // Add an extra "0" by the end.  PDB files on Windows.have an 'age'

This looks like an accidental change.

::: toolkit/crashreporter/google-breakpad/src/common/windows/http_upload.cc
@@ +336,1 @@
>    ifstream file(_wfopen(filename.c_str(), L"rb"));

I think you can just remove this branch at this point and make it:
#ifdef _MSC_VER
  file.open(...)
#else  // GCC
// what you have below

::: toolkit/crashreporter/google-breakpad/src/google_breakpad/common/minidump_cpu_amd64.h
@@ +27,5 @@
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */
>  
>  /* minidump_format.h: A cross-platform reimplementation of minidump-related
> + * portions of dbghelp.h from the Windows Platform SDK.

These comment changes seem unnecessary. I assume you generated these changes with a script? Can you omit them just to make the patch smaller?
Attachment #8500329 - Flags: review?(ted)
Attached patch fixSplinter Review
Attachment #8500329 - Attachment is obsolete: true
Attachment #8514361 - Flags: review?(ted)
Thanks for the review.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> Comment on attachment 8500329 [details] [diff] [review]
> patch.diff
> 
> Review of attachment 8500329 [details] [diff] [review]:
> toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/
> minidump_generator.cc
> @@ +176,4 @@
> >  
> > +  AVRF_HANDLE_OPERATION* data_iter = reinterpret_cast<AVRF_HANDLE_OPERATION*>(stream_data + 1);
> > +  for(i = operations_.begin(); i != i_end; i++)
> > +      *data_iter++ = *i;
> 
> This is sort of unfortunate. I think all stdext::checked_array_iterator does
> here is silence a warning in MSVC, since this array is guaranteed to be the
> same length as operations_.

Yeah, I could use std::copy without checked_array_iterator on GCC, but that would introduce #ifdef. That's why the new patch does. If you like the previous fix better, I will revert to that and adress style issues.

> This code violates some Google style guidelines, though. First, you need
> braces around the body of the for. Second, split the assignment and the
> increment into two lines (or maybe put the increment up with the other for
> loop increment). Also, nitpicky, but you want a space after 'for'.
> 
> ::: toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc
> @@ +822,5 @@
> >    string id_no_dash;
> >    for (int i = 0; identifier_str[i] != '\0'; ++i)
> >      if (identifier_str[i] != '-')
> >        id_no_dash += identifier_str[i];
> > +  // Add an extra "0" by the end.  PDB files on Windows.have an 'age'
> 
> This looks like an accidental change.

Yes, sorry about that.

> ::: toolkit/crashreporter/google-breakpad/src/common/windows/http_upload.cc
> @@ +336,1 @@
> >    ifstream file(_wfopen(filename.c_str(), L"rb"));
> 
> I think you can just remove this branch at this point and make it:
> #ifdef _MSC_VER
>   file.open(...)
> #else  // GCC
> // what you have below

OK, done.

> :::
> toolkit/crashreporter/google-breakpad/src/google_breakpad/common/
> minidump_cpu_amd64.h
> @@ +27,5 @@
> >   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> >   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */
> >  
> >  /* minidump_format.h: A cross-platform reimplementation of minidump-related
> > + * portions of dbghelp.h from the Windows Platform SDK.
> 
> These comment changes seem unnecessary. I assume you generated these changes
> with a script? Can you omit them just to make the patch smaller?

Yes, that's what global sed did and I thought that it's always nice to have fewer mentions of not lowercased names to not propagate that... The new version doesn't have those parts.
Attached patch brakpad.diffSplinter Review
This is the patch applied to upstream. There was only one conflict.
Comment on attachment 8514361 [details] [diff] [review]
fix

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

::: toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/minidump_generator.cc
@@ +180,5 @@
>                  reinterpret_cast<AVRF_HANDLE_OPERATION*>(stream_data + 1),
> +                operations_.size())
> +#else
> +            reinterpret_cast<AVRF_HANDLE_OPERATION*>(stream_data + 1)
> +#endif

Thanks. I think this is the best we can do here.
Attachment #8514361 - Flags: review?(ted) → review+
Duplicate of this bug: 1059725
Thanks. Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8e6288be38
Whiteboard: [leave open]
(If you could close the breakpad.appspot.com issue that'd be helpful.)
I closed it. Thanks.
https://hg.mozilla.org/mozilla-central/rev/7e8e6288be38
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.