Remove just enough NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW uses to stop segfaulting for GCC 6 -O2 builds

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This is split out from bug 1232696; see that for further details.

It appears that NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW interacts badly
with GCC 6's new Lifetime-DSE optimization, in that the optimisation
in some cases removes the zeroing, resulting in many/all of the class
fields being undefined.

Removing NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW will be some work, but
fortunately it appears that only a small subset of them trigger this
behaviour, at least for GCC 6 at -O2.  This bug fixes those.  Bug 1232696
tracks the removal of the rest.
(Assignee)

Comment 1

2 years ago
Created attachment 8803784 [details] [diff] [review]
bug1232696-2-minimal.cset

Minimal fix.  Gives explicit initialisation for the classes
nsContentSink and nsXMLContentSink.
(Assignee)

Updated

2 years ago
Attachment #8803784 - Flags: review?(nfroyd)
Comment on attachment 8803784 [details] [diff] [review]
bug1232696-2-minimal.cset

Review of attachment 8803784 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentSink.h
@@ -267,5 @@
>  
> -private:
> -  // People shouldn't be allocating this class directly.  All subclasses should
> -  // be allocated using a zeroing operator new.
> -  void* operator new(size_t sz) CPP_THROW_NEW;  // Not to be implemented

Removing this makes me a little nervous, but I guess since this class already has a protected destructor and pure virtual methods, none of the subclasses are going to be instantiating it directly anyway.
Attachment #8803784 - Flags: review?(nfroyd) → review+
Component: Build Config → DOM
(Assignee)

Comment 4

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Removing this makes me a little nervous, [..]

Yes, fair comment, and it made me nervous too.  What I can say is
that, with the patch applied, I ran all of mochitests on valgrind
and didn't see any (new) complaints of uninitialisedness.  So that
makes me fairly confident that it is OK.

Comment 5

2 years ago
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/990964802af8
Remove just enough NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW uses to stop segfaulting for GCC 6 -O2 builds.  r=froydnj.

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/990964802af8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

2 years ago
Blocks: 1316555
Duplicate of this bug: 1331363
You need to log in before you can comment on or make changes to this bug.