829327 - Add back the MOZ_NORETURN annotation on mozalloc_abort

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Daniel Holbert [:dholbert]]

One of the changes in bug 824224 was to remove the MOZ_NORETURN annotation on mozalloc_abort().

This triggers this build warning, w/ GCC 4.7.2 on Ubuntu 12.10:
> mozalloc_abort.cpp: In function ‘void abort()’:
> mozalloc_abort.cpp:40:1: warning: ‘noreturn’ function does return [enabled by default]

...because a system-header declares "abort() as noreturn, and we're providing our own abort() implementation, in which we call some other method (mozalloc_abort), and as far as the compiler knows, mozalloc_abort() *will* return, so abort()looks like it can also return.

We can fix this by adding back the MOZ_NORETURN annotation on mozalloc_abort. Per bug 824224 comment 48, this change might decrease the quality of our crash-stacks, but we don't know that for sure, so I propose we do it. (and then, if it does break crash-stacks, we can revert it and maybe just disable this warning in this directory)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch leaves the directory warning-free, too (at least on Linux; I'll verify on other platforms), so the patch marks it as FAIL_ON_WARNINGS = 1 to keep it that way. :)

Comment 2

[Mike Hommey [:glandium]]

I'm pretty sure this helped the crash stacks to be better on arm, so i would at least make it conditional.

Comment 3

[Daniel Holbert [:dholbert]]

Sounds good to me. What's the recommended way to check for that?

Comment 4

[Daniel Holbert [:dholbert]]

Looks like some existing code uses #if !defined(__arm__) -- I'll add that.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1 again (forgot to qref, sorry)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2: now with #if !defined(__arm__)

(sorry, forgot to qref to pick up the !defined(__arm__) change)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v3: now w/out FAIL_ON_WARNINGS so that Android builds can still warn about this

...and of course, our Android builds spam the same "warning: ‘noreturn’ function does return", so we can't mark this FAIL_ON_WARNINGS if we're exempting arm builds from the fix.

So: Here's the patch again, w/out the FAIL_ON_WARNINGS annotation.

Comment 8

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/796d19acafbb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/796d19acafbb