Add back the MOZ_NORETURN annotation on mozalloc_abort

RESOLVED FIXED in mozilla21

Status

()

Core
mozglue
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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)
Blocks: 187528
Created attachment 700713 [details] [diff] [review]
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. :)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #700713 - Flags: review?(jones.chris.g)
I'm pretty sure this helped the crash stacks to be better on arm, so i would at least make it conditional.
Sounds good to me. What's the recommended way to check for that?
Looks like some existing code uses #if !defined(__arm__) -- I'll add that.
Created attachment 700733 [details] [diff] [review]
fix v1 again (forgot to qref, sorry)
Attachment #700713 - Attachment is obsolete: true
Attachment #700713 - Flags: review?(jones.chris.g)
Created attachment 700754 [details] [diff] [review]
fix v2: now with #if !defined(__arm__)

(sorry, forgot to qref to pick up the !defined(__arm__) change)
Attachment #700733 - Attachment is obsolete: true
Attachment #700733 - Attachment description: fix v2: now with #if !defined(__arm__) → fix v1 again (forgot to qref, sorry)
Attachment #700754 - Flags: review?(jones.chris.g)
Created attachment 700786 [details] [diff] [review]
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.
Attachment #700754 - Attachment is obsolete: true
Attachment #700754 - Flags: review?(jones.chris.g)
Attachment #700786 - Flags: review?(jones.chris.g)
Attachment #700786 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/796d19acafbb
Depends on: 824224
https://hg.mozilla.org/mozilla-central/rev/796d19acafbb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.