558928 - Improve nsDebugImpl.cpp::Abort() implementations

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

According to Ehsan:

>diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
>+#if defined(_WIN32)
>+#  if !defined(WINCE)
>+    //This should exit us
>+    raise(SIGABRT);
>+#  endif

Would it make sense here to call RaiseException, at least for Windows CE?

>+#endif
>+
>+    // Still haven't aborted?  Try dereferencing null.
>+    // (Written this way to lessen the likelihood of it being optimized away.)
>+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
>+    // this is crazy

In order to make sure that we'll cause a crash, there is a better technique.

  __debugbreak();

This is guaranteed to work, and won't get optimized away.


According to http://msdn.microsoft.com/en-us/library/f408b4et.aspx, __debugbreak() is a synonym for |asm { int 3 }|.  There's also a :DebugBreak() function already used in nsDebugImpl.cpp that seems to do approximately the same thing.  Is one preferable over the other?  Is it guaranteed that these will trigger breakpad?

Comment 1

[(no longer active)]

(In reply to comment #0)
> According to http://msdn.microsoft.com/en-us/library/f408b4et.aspx,
> __debugbreak() is a synonym for |asm { int 3 }|.  There's also a :DebugBreak()
> function already used in nsDebugImpl.cpp that seems to do approximately the
> same thing.  Is one preferable over the other?  Is it guaranteed that these
> will trigger breakpad?

__debugbreak() is only equivalent to |asm { int 3 }| on 32-bit systems, as 64-bit systems do not support that.  It is also a compiler intrinsic.  DebugBreak is a Windows API.  I _think_ on 64-bit systems, __debugbreak() turns out to a call to DebugBreak.  Using either of them directly is fine, but using |asm { int 3}| is not because it won't work on platforms other than x86.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #0)
> Is it guaranteed that these
> will trigger breakpad?

Oops, here's the answer (http://msdn.microsoft.com/en-us/library/ms679297%28VS.85%29.aspx):

  If the process is not being debugged, the function uses the search logic of a standard exception handler. In most cases, this causes the calling process to terminate because of an unhandled breakpoint exception.

Sounds good to me.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Use __debugbreak() to abort on windows

Comment 4

[(no longer active)]

Comment on attachment 443925 [details] [diff] [review]
Use __debugbreak() to abort on windows

r=me.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/1e9af0857103

Comment 6

[Jacek Caban]

Attachment: Use DebugBreak instead of __debugbreak.

This batch broke mingw comilation due to nonportable __debugbreak use. Although it's possible to hack mingw to handle it, I think we'd better avoid such nonportable tricks. According to:
http://msdn.microsoft.com/en-us/library/26td21ds.aspx
all we loose with this patch is compiler optimization, which is nothing to worry about in this case.

Comment 7

[(no longer active)]

Comment on attachment 446487 [details] [diff] [review]
Use DebugBreak instead of __debugbreak.

Could you please add an ifdef on _MSC_VER so that we continue to use __debugbreak for MSVC and we fall back to DebugBreak on other compilers?

Comment 8

[Jacek Caban]

Attachment: Use DebugBreak instead of __debugbreak on GCC.

Sure, thanks for review, the patch is attached.

Comment 9

[(no longer active)]

Comment on attachment 446704 [details] [diff] [review]
 Use DebugBreak instead of __debugbreak on GCC.

>-#if defined(XP_WIN)
>+#if defined(_MSC_VER)
> #  include <intrin.h>           // for __debugbreak()
>+#elif defined(XP_WIN)

Could you add a comment which makes the logic more apparent here?  Something like:

#if defined(_MSC_VER)     // MSVC
...
#elif defined(XP_WIN)     // mingw

r=me with that fixed.

Comment 10

[Jacek Caban]

Attachment: Use DebugBreak instead of __debugbreak on GCC.

Fixed as requested, thanks.

Comment 11

[Justin Wood (:Callek)]

(In reply to comment #10)
> Created an attachment (id=446766) [details]
> Use DebugBreak instead of __debugbreak on GCC.

Pushed as: http://hg.mozilla.org/mozilla-central/rev/4a076422aa26