Improve nsDebugImpl.cpp::Abort() implementations

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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?
Version: unspecified → Trunk

Comment 1

8 years ago
(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.
(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.
Depends on: 564158
Created attachment 443925 [details] [diff] [review]
Use __debugbreak() to abort on windows
Assignee: nobody → jones.chris.g
Attachment #443925 - Flags: review?(ehsan)

Updated

8 years ago
Attachment #443925 - Flags: review?(ehsan) → review+

Comment 4

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

r=me.
http://hg.mozilla.org/mozilla-central/rev/1e9af0857103
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 6

8 years ago
Created attachment 446487 [details] [diff] [review]
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.
Attachment #446487 - Flags: review?

Updated

8 years ago
Attachment #446487 - Flags: review? → review?(jones.chris.g)

Comment 7

8 years ago
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?
Attachment #446487 - Flags: review?(jones.chris.g) → review-

Comment 8

8 years ago
Created attachment 446704 [details] [diff] [review]
 Use DebugBreak instead of __debugbreak on GCC.

Sure, thanks for review, the patch is attached.
Attachment #446704 - Flags: review?(ehsan)

Comment 9

8 years ago
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.
Attachment #446704 - Flags: review?(ehsan) → review+

Comment 10

8 years ago
Created attachment 446766 [details] [diff] [review]
Use DebugBreak instead of __debugbreak on GCC.

Fixed as requested, thanks.
Attachment #446487 - Attachment is obsolete: true

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: checkin: comment 10
(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
Keywords: checkin-needed
Whiteboard: checkin: comment 10
You need to log in before you can comment on or make changes to this bug.