856566 - Fix crashreporter compilation and profiler on mingw.

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Jacek Caban]

Attachment: fix

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.

Comment 1

[Jacek Caban]

Attachment: Upstream changes (for the reference)

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Jacek Caban]

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.

Comment 4

[Jacek Caban]

Attachment: fix v1.1

Comment 5

[Jacek Caban]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Philip Chee]

My build keeps falling over at:

c:/t1/hg/comm-central/mozilla/toolkit/crashreporter/client/crashreporter_win.cpp(1227) : error C2065: 'nullptr' : undeclared identifier

Comment 8

[Philip Chee]

Ms2ger says to add:
#include "mozilla/NullPtr.h"

And this seems to work for me. Patch coming up.

Comment 9

[Philip Chee]

Attachment: Fix for: error C2065: 'nullptr' : undeclared identifier

This patch allows me to build successfully with VS2008SP1 / VC9

Comment 10

[Philip Chee]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09ff3bd0beb8

Comment 12

[Jacek Caban]

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

Comment 13

[Jacek Caban]

Attachment: patch.diff

Comment 14

[(not currently active) Ted Mielczarek]

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?

Comment 15

[Jacek Caban]

Attachment: fix

Comment 16

[Jacek Caban]

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.

Comment 17

[Jacek Caban]

Attachment: brakpad.diff

This is the patch applied to upstream. There was only one conflict.

Comment 18

[(not currently active) Ted Mielczarek]

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.

Comment 20

[Jacek Caban]

Thanks. Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8e6288be38

Comment 21

[(not currently active) Ted Mielczarek]

Landed upstream:
https://code.google.com/p/google-breakpad/source/detail?r=1399

Comment 22

[(not currently active) Ted Mielczarek]

(If you could close the breakpad.appspot.com issue that'd be helpful.)

Comment 23

[Jacek Caban]

I closed it. Thanks.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e8e6288be38