1021290 - More Atomics.h build fixes for clang-cl

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: More Atomics.h build fixes for clang-cl

This is basically more of bug 932454

Comment 1

[Nathan Froyd [:froydnj]]

Comment on attachment 8435286 [details] [diff] [review]
More Atomics.h build fixes for clang-cl

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

r=me with the change below.

::: mfbt/Atomics.h
@@ +885,4 @@
>  struct IntrinsicIncDec : public IntrinsicAddSub<T>
>  {
>    typedef typename IntrinsicAddSub<T>::ValueType ValueType;
> +  typedef typename IntrinsicBase<T>::Primitives Primitives;

I don't think this is needed, since we never use Primitives in this class, right?

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/be43c6389387 for (at least) Windows build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=41158492&tree=Mozilla-Inbound

Comment 3

[(no longer active)]

Attachment: More Atomics.h build fixes for clang-cl. r=froydnj

This is basically more of bug 932454

Comment 4

[(no longer active)]

Comment on attachment 8436988 [details] [diff] [review]
More Atomics.h build fixes for clang-cl. r=froydnj

This builds on MSVC10.  The gfx changes are needed because on MSVC10, you seem to need the full definition of T for Atomic<T> because we end up requiring to compute its size somehow.  That is a bug in that compiler which has been fixed in MSVC11.  Other than that, I also had to handle the partial specialization of AtomicIntrinsics.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8436988 [details] [diff] [review]
More Atomics.h build fixes for clang-cl. r=froydnj

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

r=me with the AtomicIntrinsics change explained.

::: mfbt/Atomics.h
@@ +916,5 @@
>  struct AtomicIntrinsics<T*, Order> : public IntrinsicMemoryOps<T*, Order>,
>                                       public IntrinsicIncDec<T*>
>  {
>    typedef typename IntrinsicMemoryOps<T*, Order>::ValueType ValueType;
> +  typedef typename IntrinsicBase<T*>::Primitives Primitives;

You say "I had to handle this", but where does AtomicIntrinsics<T*>::Primitives get used?  It doesn't get used in this class, and it doesn't get used further down...so where is this relevant?

Comment 6

[(no longer active)]

Talked to Nathan on IRC and explained that I don't really understand why any of my parts are needed, except for working around MSVC10 compiler bugs.  Will add a comment to that affect when landing.

https://tbpl.mozilla.org/?tree=Try&rev=f1afceb2266f

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa494104700e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa494104700e

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4)
> This builds on MSVC10.  The gfx changes are needed because on MSVC10, you
> seem to need the full definition of T for Atomic<T> because we end up
> requiring to compute its size somehow.

In AsyncPanZoomController.h we use Atomic<T*>, not Atomic<T>. I assume that also requires the full definition of T?

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #4)
> > This builds on MSVC10.  The gfx changes are needed because on MSVC10, you
> > seem to need the full definition of T for Atomic<T> because we end up
> > requiring to compute its size somehow.
> 
> In AsyncPanZoomController.h we use Atomic<T*>, not Atomic<T>. I assume that
> also requires the full definition of T?

Unfortunately yes.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #10)
> (In reply to comment #9)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #4)
> > > This builds on MSVC10.  The gfx changes are needed because on MSVC10, you
> > > seem to need the full definition of T for Atomic<T> because we end up
> > > requiring to compute its size somehow.
> > 
> > In AsyncPanZoomController.h we use Atomic<T*>, not Atomic<T>. I assume that
> > also requires the full definition of T?
> 
> Unfortunately yes.

If somebody knows appropriate template tricks to not require this, I would be in favor of fixing this.  I think it's surprising behavior.  (But then again, non-MSVC has required this for a long time now and nobody has complained too loudly, so....)