Note that we want MOZ_CRASH() to continue to be safe -- that is, it should crash in a predictable way regardless of the state of the stack and heap. Invoking printf() doesn't satisfy these requirements, because printf() may touch the heap, allocate memory, and so on. (Possibly depending on the system, inputs, and implementation. But definitely the interface makes no such promise.) So on Linux we'd want the string to be printed to be printed via a system call, and we'd want similar ways on other platforms. My not knowing enough about the latter problem is most of the reason I haven't done this myself yet.

I think we can probably safely write to /dev/stdout by open()'ing it and then write()'ing to it, using syscalls written in inline asm. (We can't write to fd 1 because it might have been redirected, e.g. to /proc/self/mem. /dev/stdout is better than /dev/tty because /dev/stdout can be redirected, which is what we want for our automated testers. We have to write the syscalls ourselves because the libc jump table might be corrupted.) But I wonder if this is over-engineering things. Do we really need to print the message in non-engineering builds? In production, nobody is looking at stdout, and anyway, we can recover the message by looking at the crashing file+line, as reported in breakpad. For development builds (or maybe for builds with breakpad disabled?) it would be nice to print a message to stdout. We can do this on such builds using printf (or by writing to fd 1, or whatever), because we don't ship to users without breakpad.

(In reply to Justin Lebar [:jlebar] from comment #2) > I think we can probably safely write to /dev/stdout by open()'ing it and > then write()'ing to it, using syscalls written in inline asm. Please, no inline asm. > (We can't write to fd 1 because it might have been redirected, e.g. to > /proc/self/mem. /dev/stdout is better than /dev/tty because /dev/stdout can > be redirected, which is what we want for our automated testers. We have to > write the syscalls ourselves because the libc jump table might be corrupted.) Checking glibc for x86-64, i386, and arm indicates that there's no jump table involved. (Admittedly this might depend on the libc, but with a hypothetical libc engineer hat on, I'd want to make syscall(2) as simple as possible, which means not using a hardcoded jump table--syscall(2) should work with other syscalls than those for which the libc has been compiled, for instance) Are you thinking about the kernel's jump table instead? If we can overwrite the kernel's jump table, there are other problems. :) Maybe the situation on Mac is different, but I'd hope not. I vote for syscall(2) over inline asm. No idea about Windows, though.

Unless a) syscall() is defined entirely in a header file, or b) we are statically linked to libc there is an indirect branch involved in calling syscall(), no?

(In reply to Justin Lebar [:jlebar] from comment #2) > I think we can probably safely write to /dev/stdout by open()'ing it and > then write()'ing to it, using syscalls written in inline asm. And that may fail because your process has the maximum number of file descriptors in use.

FYI, RE: syscalls, Breakpad uses: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/third_party/lss/linux_syscall_support.h

(In reply to Justin Lebar [:jlebar] from comment #4) > Unless > > a) syscall() is defined entirely in a header file, or > b) we are statically linked to libc > > there is an indirect branch involved in calling syscall(), no? Ah, yes. For avoidance of doubt, you are concerned about the GOT being corrupted, then? (When I see "jump table", I think "switch", so there may have just been a terminology mixup.) I realize Breakpad has more complicated requirements than this bug, but that header file is why I'd rather not start down the road of inline asm.

> For avoidance of doubt, you are concerned about the GOT being corrupted, then? Yes. I knew I wasn't using the right word... > I realize Breakpad has more complicated requirements than this bug, but that header file > is why I'd rather not start down the road of inline asm. Looks like if we wanted it, we could just use that header. But I agree; that's more complication than I want as well.

jlebar: in https://hg.mozilla.org/try/rev/52854ffca239#l1.128 > 1.128 + * bulds, and it's hard to print to stderr safely when memory might have been s/bulds/builds/

(In reply to Chris Peterson (:cpeterson) from comment #9) > jlebar: in https://hg.mozilla.org/try/rev/52854ffca239#l1.128 > > > 1.128 + * bulds, and it's hard to print to stderr safely when memory might have been > > s/bulds/builds/ Thanks, fixed.

I /think/ I can replace MOZ_NoReturn() with __assume(0). I'll push that to try. That bit is necessary for bug 820686.

Comment on attachment 760680 [details] [diff] [review] Patch, v1: Give MOZ_CRASH() an optional string argument. You might consider also using OutputDebugString when on Windows.

(In reply to Justin Lebar [:jlebar] from comment #12) > I /think/ I can replace MOZ_NoReturn() with __assume(0). I'll push that to > try. Actually, that's scary. If the compiler doesn't understand that TerminateProcess() is noreturn (which it apparently doesn't), then it might optimize TerminateProcess() __assume(0) into __assume(0) which would obviously be wrong. Unless you object to how it is, I think I'll leave it.

Comment on attachment 760680 [details] [diff] [review] Patch, v1: Give MOZ_CRASH() an optional string argument. Review of attachment 760680 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +239,5 @@ > + * > + * The optional explanation-string, if provided, must be a string literal > + * explaining why we're crashing. This argument is intended for use with > + * MOZ_CRASH() calls whose rationale is non-obvious; don't use it if it's > + * obvious why we're crashing. I think we want MOZ_CRASH's explanation/reason/whatever string to be mandatory, actually. I'd expect people to look at output-spew basically immediately when a debug build crashes, and seeing a description of what went wrong seems like it can only help orient people as to what went wrong, even if they do go on to look at the surrounding code/context. But MOZ_CRASH(), somewhat surprisingly, seems to have sprung up like weeds in the tree at this point, so making it optional for now seems okay. Maybe describe it as always taking an argument, and then put a little note at the end of this comment saying the no-argument version is deprecated?

Having looked through a couple hundred MOZ_NOT_REACHED() calls (with mandatory explanation strings), I'm not a fan of requiring an explanation string. In fact, in converting MOZ_NOT_REACHED to MOZ_CRASH, I removed a bunch of bad explanation strings. Most the explanations in the tree were not at all helpful. Many of them were along the lines of "no" or "not reached". Many of the rest were redundant once you opened the file (which is to say, they cluttered the code). The first thing you're going to do when you hit a MOZ_CRASH is open the file and look at the crashing line, regardless of whether or not there's an explanation string. So I think it's pretty good how it is.

Maybe you can revisit comment 15 after looking at the patches in bug 820686?

Sure, makes sense. Looking now, we'll see how far into them I get.