Closed Bug 1272203 Opened 4 years ago Closed 4 years ago

Add mozilla::NotNull to MFBT

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(4 files, 3 obsolete files)

I have a draft implementation of mozilla::NotNull. It's similar to gsl::not_null, but with one major and one minor difference, which I will explain shortly.
This patch implements mozilla::NotNull, which is similar but not identicial to
gsl::not_null.

The current draft(?) implementation of gsl::not_null is at
https://github.com/Microsoft/GSL/blob/master/include/gsl.h.

The main difference is that not_null allows implicit conversion from T to
not_null<T>. In contrast, NotNull only allows explicit conversion from T to
NotNull<T> via WrapNotNull().

The rationale for this is that when converting from a less-constrained type to
a more constrained type, implicit conversion is undesirable. For example, if I
changed a function f from this:

  f(int* aPtr);

to this:

  f(gsl::not_null<int*> aPtr);

no call sites would have to be modified. But if I changed it to this:

  f(mozilla::NotNull<int*> aPtr);

call sites *would* need to be modified. This is a good thing! It forces the
author to audit the call sites for non-nullness, and encourages them to
back-propagate NotNull throughout the code.

The other difference between not_null and NotNull is that not_null disables
pointer arithmetic, which means it cannot be used with array pointers. I have
not implemented this restriction for NotNull because it seems arbitrary and
unnecessary.
Attachment #8751588 - Flags: review?(nfroyd)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8751589 - Flags: review?(nfroyd)
This might not look compelling in isolation, but this use of NotNull would have
prevented the null dereference crash in bug 1268721.
Attachment #8751590 - Flags: review?(nfroyd)
This is a nice example of using NotNull for a single non-null pointer that gets
passed around lots of different functions.
Attachment #8751591 - Flags: review?(nfroyd)
Clang and gcc have `nonnull` and `returns_nonnull` attributes that may be interesting:

http://clang.llvm.org/docs/AttributeReference.html#id102
(In reply to Chris Peterson [:cpeterson] from comment #5)
> Clang and gcc have `nonnull` and `returns_nonnull` attributes that may be
> interesting:
> 
> http://clang.llvm.org/docs/AttributeReference.html#id102

Thank you for the info.

These attributes appear to be very weak. E.g. if you have a _Nonnull parameter, clang will complain if you pass a literal |nullptr| to it, but won't complain if you pass a variable that is null. Also, they don't work with smart pointers like RefPtr. AFAICT NotNull<> is a much more robust mechanism.
(In reply to :Ms2ger from comment #7)
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/bindings/
> BindingDeclarations.h?rev=c3f5e6079284#376
> https://mxr.mozilla.org/mozilla-central/source/xpcom/base/OwningNonNull.
> h?rev=c3f5e6079284

These are useful, but much less general than mozilla::NotNull.

- dom::binding::NonNull doesn't work with smart pointers, and it allows vanilla assignment from a base pointer.

- mozilla::OwningNonNull is a specialized version of RefPtr.
Comment on attachment 8751588 [details] [diff] [review]
(part 1) - Add mozilla::NotNull to MFBT

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

::: mfbt/NotNull.h
@@ +147,5 @@
> +NotNull<T>
> +WrapNotNull(const T aBasePtr)
> +{
> +  NotNull<T> notNull(aBasePtr);
> +  MOZ_RELEASE_ASSERT(aBasePtr);

I'm not a fan of imposing overhead, even this minimal overhead, on people to use this.  Just assert normally, and if people screw up, it's on their heads.  Expect developers to be intelligent.

@@ +153,5 @@
> +}
> +
> +// Compare two NotNulls.
> +template <typename T, typename U>
> +inline bool operator==(const NotNull<T>& aLhs, const NotNull<U>& aRhs)

inline bool should be on its own line for all of these.

@@ +165,5 @@
> +}
> +
> +// Compare a NotNull to a base pointer.
> +template <typename T, typename U>
> +inline bool operator==(const NotNull<T>& aLhs, const U& aRhs)

Seems like you should also

template <typename T>
bool
operator==(const NotNull<T>& aLhs, decltype(nullptr)) = delete;

and vice versa to prevent those from compiling (because the answer's always vacuously true).
> ::: mfbt/NotNull.h
> @@ +147,5 @@
> > +NotNull<T>
> > +WrapNotNull(const T aBasePtr)
> > +{
> > +  NotNull<T> notNull(aBasePtr);
> > +  MOZ_RELEASE_ASSERT(aBasePtr);
> 
> I'm not a fan of imposing overhead, even this minimal overhead, on people to
> use this.  Just assert normally, and if people screw up, it's on their
> heads.  Expect developers to be intelligent.

Two points:
- The compiler should be able to optimize away a lot of these assertions, because WrapNotNull() is inline and many WrapNotNull() calls will occur shortly after a null check.
- Several decades of experience has shown that "expect developers to be intelligent" is not a reliable strategy.
Duplicate of this bug: 1273594
Will this implicitly convert from MakeUnique<T>? i.e.

Will we be able to write:

NotNull<UniquePtr<int>> k = MakeUnique<int>(5)?

or do we have to write:

NotNull<UniquePtr<int>> k = WrapNotNull(MakeUnique<int>(5))?

The first is option is preferable to me.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
I see that NotNull<UniquePtr<T>> doesn't work. I suppose the question could still stand for regular new.
(In reply to Nicholas Nethercote [:njn] from comment #10)
> - The compiler should be able to optimize away a lot of these assertions

But not all, and IMO this type *should* be the basis for optimization.

> - Several decades of experience has shown that "expect developers to be
> intelligent" is not a reliable strategy.

No strategy is reliable.  But the compiler/language already adopts the strategy of not nullchecking everything.  Given that developers must go out of their way to use WrapNotNull, clearly demanding not-null meaning, this isolated demand for rigor isn't justified.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> I suppose the question could still stand for regular new.

The current patch requires the use of MakeNotNull(). See comment 1 for an explanation why.
Attachment #8751589 - Flags: review?(nfroyd) → review+
Comment on attachment 8751590 [details] [diff] [review]
(part 3) - Use NotNull in nsContentUtils::GetSurfaceData()

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

...or, at least, it would have forced people to think harder about whether something could have been non-null.  But maybe not.
Attachment #8751590 - Flags: review?(nfroyd) → review+
Attachment #8751591 - Flags: review?(nfroyd) → review+
Comment on attachment 8751588 [details] [diff] [review]
(part 1) - Add mozilla::NotNull to MFBT

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

I did not look too closely at the tests.  f+ for getting some interface issues sorted out, but I'm inclined to accept this once we do.

::: mfbt/NotNull.h
@@ +64,5 @@
> +
> +#include "mozilla/Assertions.h"
> +
> +template <typename T> class nsAutoPtr;
> +template <typename T> class RefPtr;

What do we have these here for?

@@ +97,5 @@
> +//
> +// NotNull currently doesn't work with UniquePtr because UniquePtr<T> is not
> +// copy-constructible, NotNull<T> is not move-constructible, so
> +// NotNull<UniquePtr<T>> is neither copy- nor move-constructible. See
> +// https://github.com/Microsoft/GSL/issues/89 for some more discussion.

Perhaps for now we should:

static_assert(IsPointer<T>::value, "only usable with raw pointer types!");

until we get enough template wizardry to support smart pointers efficiently (see also discussion on |get()| and |operator()| below)?

@@ +108,5 @@
> +  T mBasePtr;
> +
> +  // This constructor is only used by WrapNotNull().
> +  template <typename U>
> +  explicit NotNull(U aBasePtr) : mBasePtr(aBasePtr) {}

I'm wondering if we want a U&& constructor here, so that one could say, e.g. in part 3:

RefPtr<DataSourceSurface> maybeNullSurface = surface->GetDataSurface();
if (!dataSurface) {
  ...
}

NotNull<RefPtr<DataSourceSurface>> dataSurface = WrapNotNull(Move(maybeNullSurface));

@@ +120,5 @@
> +  MOZ_IMPLICIT NotNull(const NotNull<U>& aOther) : mBasePtr(aOther.get()) {}
> +
> +  // Default copy construction and assignment.
> +  NotNull(const NotNull<T>&) = default;
> +  NotNull<T>& operator=(const NotNull<T>&) = default;

Can you explicitly add |= delete| for move construction/assignment?  And FWIW, |NotNull| works just fine as a type name inside the class, no need to NotNull<T>.

@@ +127,5 @@
> +  // Nb: This has no effect when T is a raw pointer -- the implicit conversion
> +  // below applies in boolean context, alas.
> +  explicit operator bool() const = delete;
> +
> +  // Explicit conversion to a base pointer. Use only to resolve ambiguity or to

Saying "base pointer" when the code returns a reference seems confusing; I know what you mean, but I'm unsure of how to say it better.

@@ +130,5 @@
> +
> +  // Explicit conversion to a base pointer. Use only to resolve ambiguity or to
> +  // get a castable pointer.
> +  const T& get() const { return mBasePtr; }
> +  T& get() { return mBasePtr; }

I'm not excited about this or the non-const operator() below, since this opens up the ability to modify the internals of the class and thereby break invariants.  Presumably this functionality is there for smart pointer types, so that .get() on NonNull<RefPtr<T>> doesn't do extra reference counting?  Not a fan.

@@ +134,5 @@
> +  T& get() { return mBasePtr; }
> +
> +  // Implicit conversion to a base pointer. Preferable to get().
> +  operator const T&() const { return get(); }
> +  operator T&() { return get(); }

This is partly here so that passing NotNull<T> to a function that takes T* just works without any extra effort?  I think we'd eventually want to implement the suggestion from the above-referenced github issue that .get() on a not_null<smart_ptr<T>> returns smart_ptr::get().

@@ +147,5 @@
> +NotNull<T>
> +WrapNotNull(const T aBasePtr)
> +{
> +  NotNull<T> notNull(aBasePtr);
> +  MOZ_RELEASE_ASSERT(aBasePtr);

I sympathize with Waldo's position here.  But I'm inclined to keep this in, because being able to pass a null pointer to WrapNotNull breaks the whole point of NotNull existing.  If NotNull gets used, and we get crashes about NotNull things being null because we don't have this assert, then we're no better off than we were before.  But if NotNull gets used and we keep the assert, then we get a crash at the point when things started to go sideways, not when they're already off the road and sliding through the forest somewhere.  And that's much more useful.  (Though I'm not sure how many of our null-pointer-or-close-to-it crashes are not immediately obvious what's going wrong...but then again, I know I've seen "well, just add a null pointer check" patches go by, and it seems like we could do better than that with something like this assert.)

There is a small runtime cost and a less-small space cost (extra code, strings for filenames, assert messages, etc.).  It's not obvious that all the uses of WrapNotNull will result in runtime checks, either; the compiler can probably optimize away some of them.  It certainly can in some of Nick's later patches.  For the small number of uses we're going to have initially, I don't think that's awful.  Perhaps we can use MOZ_DIAGNOSTIC_ASSERT as a trial instead?
Attachment #8751588 - Flags: review?(nfroyd) → feedback+
> > +template <typename T> class nsAutoPtr;
> > +template <typename T> class RefPtr;
> 
> What do we have these here for?

Cruft from an earlier version, no longer needed. I'll remove.


> @@ +97,5 @@
> > +//
> > +// NotNull currently doesn't work with UniquePtr because UniquePtr<T> is not
> > +// copy-constructible, NotNull<T> is not move-constructible, so
> > +// NotNull<UniquePtr<T>> is neither copy- nor move-constructible. See
> > +// https://github.com/Microsoft/GSL/issues/89 for some more discussion.
> 
> Perhaps for now we should:
> 
> static_assert(IsPointer<T>::value, "only usable with raw pointer types!");
> 
> until we get enough template wizardry to support smart pointers efficiently
> (see also discussion on |get()| and |operator()| below)?

If NotNull doesn't work with RefPtr and nsCOMPtr that takes away a big chunk of the motivation for using it. So I've left that unchanged for now.


> > +  template <typename U>
> > +  explicit NotNull(U aBasePtr) : mBasePtr(aBasePtr) {}
> 
> I'm wondering if we want a U&& constructor here, so that one could say, e.g.
> in part 3:
> 
> RefPtr<DataSourceSurface> maybeNullSurface = surface->GetDataSurface();
> if (!dataSurface) {
>   ...
> }
> 
> NotNull<RefPtr<DataSourceSurface>> dataSurface =
> WrapNotNull(Move(maybeNullSurface));

I haven't done that for now.


> @@ +120,5 @@
> > +  MOZ_IMPLICIT NotNull(const NotNull<U>& aOther) : mBasePtr(aOther.get()) {}
> > +
> > +  // Default copy construction and assignment.
> > +  NotNull(const NotNull<T>&) = default;
> > +  NotNull<T>& operator=(const NotNull<T>&) = default;
> 
> Can you explicitly add |= delete| for move construction/assignment?

I tried adding those, but I got this error about the constructor:

> In file included from Unified_cpp_widget0.cpp:101:
> /home/njn/moz/mi9/widget/PuppetWidget.cpp:993:36: error: copying parameter of type 'mozilla::NotNull<mozilla::gfx::DataSourceSurface *>' invokes deleted constructor
>     nsContentUtils::GetSurfaceData(WrapNotNull(dataSurface), &length, &stride);
>                                    ^~~~~~~~~~~~~~~~~~~~~~~~
> ../dist/include/mozilla/NotNull.h:124:3: note: 'NotNull' has been explicitly marked deleted here
>   NotNull(NotNull<T>&&) = delete;
>   ^

and these errors about the assignment operator:

> In file included from Unified_cpp_xpcom_threads0.cpp:119:
> /home/njn/moz/mi9/xpcom/threads/nsThread.cpp:1181:13: error: overload resolution selected deleted operator '='
>     mEvents = WrapNotNull(mEvents->mNext);
>     ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../dist/include/mozilla/NotNull.h:125:15: note: candidate function has been explicitly deleted
>   NotNull<T>& operator=(NotNull<T>&&) = delete;
>               ^
> ../../dist/include/mozilla/NotNull.h:121:15: note: candidate function
>   NotNull<T>& operator=(const NotNull<T>&) = default;
>               ^
> 1 error generated.

I don't understand these errors. I've left that part unchanged for now.


> And FWIW, |NotNull| works just fine as a type name inside the class, no need to
> NotNull<T>.

I've been using NotNull<T> because sometimes there is NotNull<U>, which does require the template parameter, and I like the consistency. I've left it unchanged for now, but I can remove if you feel strongly about it.


> @@ +127,5 @@
> > +  // Nb: This has no effect when T is a raw pointer -- the implicit conversion
> > +  // below applies in boolean context, alas.
> > +  explicit operator bool() const = delete;
> > +
> > +  // Explicit conversion to a base pointer. Use only to resolve ambiguity or to
> 
> Saying "base pointer" when the code returns a reference seems confusing; I
> know what you mean, but I'm unsure of how to say it better.

The comment at the top of the class defines "base pointer" as a raw or smart pointer, so I don't know how to improve this.


> > +  // Explicit conversion to a base pointer. Use only to resolve ambiguity or to
> > +  // get a castable pointer.
> > +  const T& get() const { return mBasePtr; }
> > +  T& get() { return mBasePtr; }
> 
> I'm not excited about this or the non-const operator() below, since this
> opens up the ability to modify the internals of the class and thereby break
> invariants.  Presumably this functionality is there for smart pointer types,
> so that .get() on NonNull<RefPtr<T>> doesn't do extra reference counting? 
> Not a fan.

I was able to remove the non-const get() and operator() and operator->. I also changed operator*'s return type to const.


> I think we'd eventually want to implement the suggestion from the above-referenced github issue that .get() on a
> not_null<smart_ptr<T>> returns smart_ptr::get().

I don't know how to implement that. Any tips?


> Perhaps we can use MOZ_DIAGNOSTIC_ASSERT as a trial instead?

That would be better than MOZ_ASSERT, but I still prefer MOZ_RELEASE_ASSERT. A few weeks of crash report triage is a bracing reminder of the value of disabled checks. Let's be more Rust-like where we can.
Updated version.
Attachment #8755711 - Flags: feedback?(nfroyd)
Attachment #8751588 - Attachment is obsolete: true
Comment on attachment 8755711 [details] [diff] [review]
(part 1) - Add mozilla::NotNull to MFBT

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

Forgive the drive by...

::: mfbt/NotNull.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_Ptr_h

This file is called NotNull.h and is included as "mozilla/NotNull.h", so I believe the include guard should be "mozilla_NotNull_h" instead?
(In reply to Nicholas Nethercote [:njn] from comment #18)
> > @@ +120,5 @@
> > > +  MOZ_IMPLICIT NotNull(const NotNull<U>& aOther) : mBasePtr(aOther.get()) {}
> > > +
> > > +  // Default copy construction and assignment.
> > > +  NotNull(const NotNull<T>&) = default;
> > > +  NotNull<T>& operator=(const NotNull<T>&) = default;
> > 
> > Can you explicitly add |= delete| for move construction/assignment?
> 
> I tried adding those, but I got this error about the constructor:
> 
> > In file included from Unified_cpp_widget0.cpp:101:
> > /home/njn/moz/mi9/widget/PuppetWidget.cpp:993:36: error: copying parameter of type 'mozilla::NotNull<mozilla::gfx::DataSourceSurface *>' invokes deleted constructor
> >     nsContentUtils::GetSurfaceData(WrapNotNull(dataSurface), &length, &stride);
> >                                    ^~~~~~~~~~~~~~~~~~~~~~~~
> > ../dist/include/mozilla/NotNull.h:124:3: note: 'NotNull' has been explicitly marked deleted here
> >   NotNull(NotNull<T>&&) = delete;
> >   ^

|WrapNotNull(dataSurface)| is an rvalue, so overload resolution, which does not take into account the fact that a candidate is deleted, chooses the move constructor.

What is the motivation for deleting the move constructor / assignment operator?
(In reply to Botond Ballo [:botond] from comment #21)
> |WrapNotNull(dataSurface)| is an rvalue, so overload resolution, which does
> not take into account the fact that a candidate is deleted, chooses the move
> constructor.
> 
> What is the motivation for deleting the move constructor / assignment
> operator?

I was asking for it to be deleted, but I should have asked for |= default| if that was reasonable, or for it to be explicitly defined.  My fault for wasting time there.
(In reply to :Cykesiopka from comment #20)
> >
> > +#ifndef mozilla_Ptr_h
> 
> This file is called NotNull.h and is included as "mozilla/NotNull.h", so I
> believe the include guard should be "mozilla_NotNull_h" instead?

Good catch. Thank you.


> I was asking for it to be deleted, but I should have asked for |= default|
> if that was reasonable, or for it to be explicitly defined.

I'll add default versions.
Minor fixes.
Attachment #8756167 - Flags: review?(nfroyd)
Attachment #8755711 - Attachment is obsolete: true
Attachment #8755711 - Flags: feedback?(nfroyd)
Comment on attachment 8756167 [details] [diff] [review]
(part 1) - Add mozilla::NotNull to MFBT

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

OK, let's try this.  A couple of comments below.

::: mfbt/NotNull.h
@@ +119,5 @@
> +  // Default copy/move construction and assignment.
> +  NotNull(const NotNull<T>&) = default;
> +  NotNull<T>& operator=(const NotNull<T>&) = default;
> +  NotNull(NotNull<T>&&) = default;
> +  NotNull<T>& operator=(NotNull<T>&&) = default;

This bit contradicts the comment above the class which claims that NotNull<T> is not move-constructable.  I think that means the comment needs to be modified.

@@ +129,5 @@
> +  // get a castable pointer.
> +  const T& get() const { return mBasePtr; }
> +
> +  // Implicit conversion to a base pointer. Preferable to get().
> +  operator const T&() const { return get(); }

On second thought, returning |const T&| here instead of |T&| is going to cause issues with code like:

extern void f(T*);

NotNull<T*> x = ...;
f(x);

You could secretly view that as encouragement to convert f to use NotNull parameters, but sometimes that might not be feasible (f might be a third-party library function, for instance, but you still want to track NotNull-ness in your own application code).  Can you file a followup bug on having:

  // If T is a smart pointer type:
  const T& get() const;
  operator const T&() const;

  // If T is a raw pointer type:
  T get();
  operator T*();

and I can try to attempt template wizardry?  In the meantime, let's remove the |const| qualifiers from the return type and the method.

@@ +145,5 @@
> +  MOZ_RELEASE_ASSERT(aBasePtr);
> +  return notNull;
> +}
> +
> +// Compare two NotNulls.

Were you able to implement Waldo's suggestion of |= delete|'ing comparisons to nullptr?  I  don't know overload and template instantiation rules well enough to know whether a comparison to nullptr will fail, because you don't have an appropriate overload defined, or whether the (NotNull&, const U&) (with U = some pointer type) would get instantiated.  I *think* the first?
Attachment #8756167 - Flags: review?(nfroyd) → review+
> This bit contradicts the comment above the class which claims that
> NotNull<T> is not move-constructable.  I think that means the comment needs
> to be modified.

Ok, will do.


> On second thought, returning |const T&| here instead of |T&| is going to
> cause issues with code like:
> 
> extern void f(T*);
> 
> NotNull<T*> x = ...;
> f(x);
> 
> You could secretly view that as encouragement to convert f to use NotNull
> parameters, but sometimes that might not be feasible (f might be a
> third-party library function, for instance, but you still want to track
> NotNull-ness in your own application code).

I don't think there's a problem here. My unit test has lots of calls like that and they work ok.

> Can you file a followup bug on having:
> 
>   // If T is a smart pointer type:
>   const T& get() const;
>   operator const T&() const;
> 
>   // If T is a raw pointer type:
>   T get();
>   operator T*();
> 
> and I can try to attempt template wizardry?

I can do it if you still think it's necessary.


> Were you able to implement Waldo's suggestion of |= delete|'ing comparisons
> to nullptr?

I've done this. Explicitly delete'ing them was necessary to prevent such comparisons from compiling.
Updated version, carrying over r+. Time for a try push.
Attachment #8756167 - Attachment is obsolete: true
Attachment #8757075 - Flags: review+
I got a compile error I don't understand on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb85997225c

The error is this:

> In file included from /builds/slave/try-lx-00000000000000000000000/build/src/dom/base/nsContentUtils.h:35:0,
>                  from /builds/slave/try-lx-00000000000000000000000/build/src/widget/PuppetWidget.cpp:22,
>                  from /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/widget/Unified_cpp_widget0.cpp:101:
> /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/dist/include/mozilla/NotNull.h: In instantiation of 'class mozilla::NotNull<RefPtr<mozilla::gfx::DataSourceSurface> >':
> /builds/slave/try-lx-00000000000000000000000/build/src/widget/PuppetWidget.cpp:992:52:   required from here
> /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/dist/include/mozilla/NotNull.h:137:29: error: 'const' qualifiers cannot be applied to 'mozilla::gfx::DataSourceSurface&'
>    const decltype(*mBasePtr) operator*() const { return *mBasePtr; }

The error only occurred on GCC on Try. I couldn't reproduce it locally, even when I installed GCC-4.8.5.

I ended up removing the |const| from the return type of |operator*| which fixed it. I'm still not sure if this is some kind of bug in whatever version of GCC we use on Try.
FWIW:

While not the source for Ubuntu's 4.8.5, this tells Ubuntu's GCC is far from pristine, and differences may well be coming from there: http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/wily/gcc-4.8/wily/files/head:/debian/patches/

In contrast, our GCC 4.8.5 is built with this script:
https://dxr.mozilla.org/mozilla-central/rev/b0096c5c727749ad3e79cbdf20d2e96bd179c213/build/unix/build-gcc/build-gcc.sh
which applies exactly one patch:
https://dxr.mozilla.org/mozilla-central/source/build/unix/build-gcc/PR64905.patch
for this issue:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64905
Blocks: 1289662
You need to log in before you can comment on or make changes to this bug.