1325771 - Spidermonkey build support for Linux m68k broken

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, defect, P5)

Description

[John Paul Adrian Glaubitz]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161201031821

Steps to reproduce:

Trying to build Firefox on Linux/m68k currently fails with [1]:

Adding configure options from /<<PKGBUILDDIR>>/debian/firefox.mozconfig
  --enable-application=browser
  --with-app-name=firefox
  --enable-release
  --prefix=/usr
  --enable-default-toolkit=cairo-gtk3
  --with-system-zlib
  --with-system-bz2
  --disable-gconf
  --enable-readline
  --enable-system-hunspell
  --disable-strip
  --disable-install-strip
  --enable-startup-notification
  --enable-system-ffi
  --with-system-libevent
  --with-system-nspr
  --with-system-nss
  --enable-system-sqlite
  --with-system-libvpx
  --disable-updater
  --enable-pie
checking for a shell... /bin/sh
checking for host system type... 
ERROR: Unknown CPU type: m68k
debian/rules:215: recipe for target 'stamps/configure-browser' failed
make[1]: *** [stamps/configure-browser] Error 1
make[1]: Leaving directory '/<<PKGBUILDDIR>>'
debian/rules:388: recipe for target 'build-arch' failed
make: *** [build-arch] Error 2
dpkg-buildpackage: error: debian/rules build-arch gave error exit status 2

This is because m68k is not known to the mozbuild build system. The attached patches fix the issue and also fix additional issues on m68k with the code base.

> [1] https://buildd.debian.org/status/fetch.php?pkg=firefox&arch=m68k&ver=50.1.0-1&stamp=1481682239

Comment 1

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1325771-Add-m68k-as-target-architecture-to-mozbu.patch

Comment 2

[John Paul Adrian Glaubitz]

Attachment: 0002-Bug-1325771-xpcom-Fix-syntax-error-in-PrepareAndDisp.patch

Comment 3

[John Paul Adrian Glaubitz]

Attachment: 0003-Bug-1325771-xpcom-Fix-type-of-result-in-NS_InvokeByI.patch

Comment 4

[John Paul Adrian Glaubitz]

Attachment: 0004-Bug-1325771-mfbt-tests-Define-RETURN_INSTR-for-m68k-.patch

Comment 5

[John Paul Adrian Glaubitz]

Attachment: 0005-Bug-1325771-mfbt-double-conversion-Update-for-m68k-f.patch

Comment 6

[John Paul Adrian Glaubitz]

Attachment: 0006-Bug-1325771-media-webrtc-Add-platform-defines-for-m6.patch

Comment 7

[John Paul Adrian Glaubitz]

Attachment: 0007-Bug-1325771-ipc-chromium-Add-platform-defines-for-m6.patch

Comment 8

[John Paul Adrian Glaubitz]

Attachment: 0008-Bug-1325771-js-jit-Make-sure-JitCode-has-at-least-4-.patch

Comment 9

[John Paul Adrian Glaubitz]

Attachment: 0009-Bug-1325771-js-vm-Make-sure-AccessorShape-and-Shape-.patch

Comment 10

[John Paul Adrian Glaubitz]

Attachment: 0010-Bug-1325771-layout-style-Make-sure-nsCSSValue-has-at.patch

Comment 11

[John Paul Adrian Glaubitz]

Attachment: 0011-Bug-1325771-mfbt-tests-Account-for-alignment-when-te.patch

Comment 12

[John Paul Adrian Glaubitz]

Attachment: 0011-Bug-1325771-mfbt-tests-Handle-platforms-with-less-st.patch

Updated 0011-Bug-1325771-mfbt-tests-Account-for-alignment-when-te.patch with better and more elaborate commit message as requested by Michael Karcher (author of that patch).

Comment 13

[John Paul Adrian Glaubitz]

Attachment: 0011-Bug-1325771-mfbt-tests-Handle-platforms-with-less-st.patch

Comment 14

[John Paul Adrian Glaubitz]

Attachment: 0011-Bug-1325771-mfbt-tests-Handle-targets-with-less-stri.patch

Fixed spelling in commit message.

Comment 15

[John Paul Adrian Glaubitz]

I just noticed, we're still lacking atomic operations for m68k (likewise alpha and hppa). Currently investigating on a possible patch.

Comment 16

[Martin Husemann]

Comment on attachment 8821741 [details] [diff] [review]
0003-Bug-1325771-xpcom-Fix-type-of-result-in-NS_InvokeByI.patch

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

One of these days we should cleanup and share all xpcom files on all systems using the same abi, renaming it i.e. to xptcinvoke_elf_m68k.cpp

Comment 17

[Martin Husemann]

Comment on attachment 8821747 [details] [diff] [review]
0008-Bug-1325771-js-jit-Make-sure-JitCode-has-at-least-4-.patch

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

isn't there a moz_allign() or something macro already to express this required alignment?

Comment 18

[John Paul Adrian Glaubitz]

Attachment: 0012-Bug-1325771-js-jit-Use-PowerPC-atomic-operations-on-.patch

Comment 19

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #15)
> I just noticed, we're still lacking atomic operations for m68k (likewise
> alpha and hppa). Currently investigating on a possible patch.

Just attached a patch which makes use of the atomic operations of PowerPC on m68k which seems to work fine.

Comment 20

[John Paul Adrian Glaubitz]

Just confirmed: With all patches applied, 'make' and 'make install' are executed without any issues.

Comment 21

[John Paul Adrian Glaubitz]

Attachment: 0012-Bug-1325771-js-jit-Use-PowerPC-atomic-operations-on-.patch

Improved the patch a bit to be more readable and also to avoid merge conflicts in case we want to use the PPC atomic operations on other architectures like hppa or alpha as well.

Comment 22

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - Add m68k as target architecture to mozbuild.

Review commit: https://reviewboard.mozilla.org/r/103750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103750/

Comment 23

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - xpcom: Fix syntax error in PrepareAndDispatch on Linux/m68k.

Review commit: https://reviewboard.mozilla.org/r/103752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103752/

Comment 24

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - xpcom: Fix type of result in NS_InvokeByIndex on Linux/m68k.

Review commit: https://reviewboard.mozilla.org/r/103754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103754/

Comment 25

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea.

Review commit: https://reviewboard.mozilla.org/r/103756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103756/

Comment 26

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - mfbt:double-conversion: Update for m68k from upstream.

Review commit: https://reviewboard.mozilla.org/r/103758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103758/

Comment 27

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - media:webrtc: Add platform defines for m68k.

Review commit: https://reviewboard.mozilla.org/r/103760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103760/

Comment 28

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - ipc:chromium: Add platform defines for m68k.

Review commit: https://reviewboard.mozilla.org/r/103762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103762/

Comment 29

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - js:jit: Make sure JitCode has at least 4 bytes alignment.

Review commit: https://reviewboard.mozilla.org/r/103764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103764/

Comment 30

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - js:vm: Make sure AccessorShape and Shape have at least 4 bytes alignment.

Review commit: https://reviewboard.mozilla.org/r/103766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103766/

Comment 31

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - layout:style: Make sure nsCSSValue has at least 4 bytes alignment.

Review commit: https://reviewboard.mozilla.org/r/103768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103768/

Comment 32

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - mfbt:tests: Handle targets with less strict alignment in TestPair.

Previously, the tests assumed that the alignment of int and long equals
their size. This commit fixes the tests for targets like m68k that have
sizeof(int) == 4 and alignof(int) == 2. A static helper function sizemax
was introduced as the offset of the second element in Pair<int,long>
might be either determined by its alignment requirement or the size of
the preceding int element and we use the helper function to pick the
larger of the two values.

Review commit: https://reviewboard.mozilla.org/r/103770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103770/

Comment 33

[Mike Hommey [:glandium]]

Attachment: Bug 1325771 - js:jit: Use PowerPC atomic operations on m68k.

Review commit: https://reviewboard.mozilla.org/r/103772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103772/

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 8825659 [details]
Bug 1325771 - Add m68k as target architecture to mozbuild.

https://reviewboard.mozilla.org/r/103750/#review104464

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8825662 [details]
Bug 1325771 - mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea.

https://reviewboard.mozilla.org/r/103756/#review104468

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 8825663 [details]
Bug 1325771 - mfbt:double-conversion: Update for m68k from upstream.

https://reviewboard.mozilla.org/r/103758/#review104474

Comment 37

[Mike Hommey [:glandium]]

The double-conversion one was fixed upstream close to three years ago, apparently. Maybe we should consider an update?

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8825668 [details]
Bug 1325771 - layout:style: Make sure nsCSSValue has at least 4 bytes alignment.

https://reviewboard.mozilla.org/r/103768/#review104476

::: layout/style/nsCSSValue.h:1030
(Diff revision 1)
>      mozilla::css::FontFamilyListRefCnt* MOZ_OWNING_REF mFontFamilyList;
>      mozilla::css::ComplexColorValue* MOZ_OWNING_REF mComplexColor;
>    } mValue;
> -};
>  
> +} __attribute__ ((aligned(4))); /* ensure alignment is at least 4 bytes */

Would be good to mention the reason for the alignment, since it's not obvious, e.g. "ensure alignment is at least 4 bytes, for m68k".

Is there any reason not to use MOZ_ALIGNED_DECL?  That would save having to add the #ifndef block at the top.

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8825665 [details]
Bug 1325771 - ipc:chromium: Add platform defines for m68k.

https://reviewboard.mozilla.org/r/103762/#review104478

Comment 40

[Mike Hommey [:glandium]]

(In reply to Cameron McCormack (:heycam) from comment #38)
> Comment on attachment 8825668 [details]
> Bug 1325771 - layout:style: Make sure nsCSSValue has at least 4 bytes
> alignment.
> 
> https://reviewboard.mozilla.org/r/103768/#review104476
> 
> ::: layout/style/nsCSSValue.h:1030
> (Diff revision 1)
> >      mozilla::css::FontFamilyListRefCnt* MOZ_OWNING_REF mFontFamilyList;
> >      mozilla::css::ComplexColorValue* MOZ_OWNING_REF mComplexColor;
> >    } mValue;
> > -};
> >  
> > +} __attribute__ ((aligned(4))); /* ensure alignment is at least 4 bytes */
> 
> Would be good to mention the reason for the alignment, since it's not
> obvious, e.g. "ensure alignment is at least 4 bytes, for m68k".

The more interesting question might be, why isn't the compiler aligning that to at least 4 bytes already?
 
> Is there any reason not to use MOZ_ALIGNED_DECL?  That would save having to
> add the #ifndef block at the top.

Actually, I'd expect the patch as is to break the build on MSVC.

Comment 41

[John Paul Adrian Glaubitz]

(In reply to Cameron McCormack (:heycam) from comment #38)
> Would be good to mention the reason for the alignment, since it's not
> obvious, e.g. "ensure alignment is at least 4 bytes, for m68k".

The default alignment on m68k is 2 bytes, not 4. That's why we have to enforce 4 bytes alignment.

> Is there any reason not to use MOZ_ALIGNED_DECL?  That would save having to
> add the #ifndef block at the top.

I didn't know MOZ_ALIGNED_DECL exists :).

Comment 42

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #40)
> The more interesting question might be, why isn't the compiler aligning that
> to at least 4 bytes already?

Because the default alignment on m68k is 2 bytes.

> > Is there any reason not to use MOZ_ALIGNED_DECL?  That would save having to
> > add the #ifndef block at the top.
> 
> Actually, I'd expect the patch as is to break the build on MSVC.

No, it won't. The attribute macro is a no-op on other compilers than gcc hence the macro definition at the top. I have also seen this mechanism in at least some of the imported third-party code.

Comment 43

[Mike Hommey [:glandium]]

(In reply to John Paul Adrian Glaubitz from comment #42)
> (In reply to Mike Hommey [:glandium] from comment #40)
> > The more interesting question might be, why isn't the compiler aligning that
> > to at least 4 bytes already?
> 
> Because the default alignment on m68k is 2 bytes.

That doesn't say why that would be a problem for that particular class, and not all the other classes in the codebase. Nor why that particular class would be aligned at 2 bytes, considering its content.

Comment 44

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #43)
> That doesn't say why that would be a problem for that particular class, and
> not all the other classes in the codebase. Nor why that particular class
> would be aligned at 2 bytes, considering its content.

The sum of the size of the individual members of the class just don't add up to a multiple of 4 on m68k, so the 2-byte alignment kicks in.

Comment 45

[Mike Hommey [:glandium]]

Sorry, but that doesn't make sense to me. Or context is severely lacking. If the alignment of pointers is 32-bits, the class should have a 32-bits alignment. If it's 16-bits, then what's the problem? What is expecting those objects to be at 32-bits aligned addresses?

Comment 46

[Jan de Mooij [:jandem]]

I (also) don't understand why we need this attribute on JitCode and (Accessor)Shape, but not on any other GC thing like BaseShape, JSObject or ObjectGroup...

Comment 47

[Jan de Mooij [:jandem]]

Oh, is it because of the uint8_t members? It would be nice to define this attribute in some MFBT header (with appropriate comments) so we don't have to do this #ifndef..#define..#endif everywhere.

Comment 48

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #45)
> Sorry, but that doesn't make sense to me. Or context is severely lacking. If
> the alignment of pointers is 32-bits, the class should have a 32-bits

Pointers are also 16-bit aligned on m68k.

> alignment. If it's 16-bits, then what's the problem? What is expecting those
> objects to be at 32-bits aligned addresses?

There are a number of static asserts which test the size of these classes in the Mozilla codebase, e.g.:

js/src/jit/Ion.cpp:JS_STATIC_ASSERT(sizeof(JitCode) % gc::CellSize == 0);

Comment 49

[John Paul Adrian Glaubitz]

(In reply to Jan de Mooij [:jandem] from comment #46)
> I (also) don't understand why we need this attribute on JitCode and
> (Accessor)Shape, but not on any other GC thing like BaseShape, JSObject or
> ObjectGroup...

Because the other objects just happen to have a size dividable by 4 on m68k.

Again, the basic alignment on m68k is 2 bytes, not 4 bytes, despite being a 32-bit architecture. The Mozilla codebase effectively assumes the alignment is always 4 bytes and the various JS_STATIC_ASSERT depend on that with their size checks.

The classes in question don't have a size a multiple of 4 on most other 32-bit architectures because of the 4-byte alignment which lets the compiler automatically add padding. If it wasn't for the 4-byte alignment, these classes wouldn't necessarily have a size of a multiple of 4 and the JS_STATIC_ASSERT size checks would fail.

Comment 50

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #49)
> The classes in question don't have a size a multiple of 4 on most other
> 32-bit architectures because of the 4-byte alignment

I meant to say:

> The classes in question do* have

Sorry.

So, anyway. The reason the JS_STATIC_ASSERTs work on other targets is because the minimum alignment is 4 and therefore any object's size is at least dividable by 4. If the sum of the size of members within an object just happen to add up to a size which is a multiple of 4, the enforced alignment is therefore not necessary because we were just lucky to have the proper size.

Comment 51

[Jan de Mooij [:jandem]]

OK, I understand. Is it possible to add the attribute to js::Cell and have it work for all derived classes (Shape, JitCode, etc)?

Comment 52

[Nathan Froyd [:froydnj]]

Comment on attachment 8825660 [details]
Bug 1325771 - xpcom: Fix syntax error in PrepareAndDispatch on Linux/m68k.

https://reviewboard.mozilla.org/r/103752/#review104540

Comment 53

[Nathan Froyd [:froydnj]]

Comment on attachment 8825661 [details]
Bug 1325771 - xpcom: Fix type of result in NS_InvokeByIndex on Linux/m68k.

https://reviewboard.mozilla.org/r/103754/#review104544

Comment 54

[John Paul Adrian Glaubitz]

(In reply to Jan de Mooij [:jandem] from comment #51)
> OK, I understand. Is it possible to add the attribute to js::Cell and have
> it work for all derived classes (Shape, JitCode, etc)?

Very good question. I'm not 100% confident, but you could be indeed right on that. I will do some research and report back. We might be able to reduce the number of patches then.

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8825664 [details]
Bug 1325771 - media:webrtc: Add platform defines for m68k.

https://reviewboard.mozilla.org/r/103760/#review104586

Comment 56

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #54)
> (In reply to Jan de Mooij [:jandem] from comment #51)
> > OK, I understand. Is it possible to add the attribute to js::Cell and have
> > it work for all derived classes (Shape, JitCode, etc)?
> 
> Very good question. I'm not 100% confident, but you could be indeed right on
> that. I will do some research and report back. We might be able to reduce
> the number of patches then.

Ok, according to a friend of mine who's also a GCC upstream developer inheriting the alignment attribute should indeed work. I'm testing an updated patch now which enforces the alignment for Cell instead of Shape, JitCode and AccessorShape.

Some of the other patches I posted here also need to be rebased anyway because of the recent patches merged to fix Firefox on Linux/sparc64.

Comment 57

[John Paul Adrian Glaubitz]

(In reply to John Paul Adrian Glaubitz from comment #41)
> (In reply to Cameron McCormack (:heycam) from comment #38)
> > Would be good to mention the reason for the alignment, since it's not
> > obvious, e.g. "ensure alignment is at least 4 bytes, for m68k".
> 
> The default alignment on m68k is 2 bytes, not 4. That's why we have to
> enforce 4 bytes alignment.
> 
> > Is there any reason not to use MOZ_ALIGNED_DECL?  That would save having to
> > add the #ifndef block at the top.
> 
> I didn't know MOZ_ALIGNED_DECL exists :).

I have looked into this and it seems that MOZ_ALIGNED_DECL isn't usable in this case. This macro is designed for instantiating aligned objects, but not for declaring them. Therefore, MOZ_ALIGNED_DECL(struct Cell, 4); doesn't work, unfortunately.

Comment 58

[John Paul Adrian Glaubitz]

Attachment: 0010-Bug-1325771-js-jit-Use-PowerPC-atomic-operations-on-.patch

Updated to have the m68k-specific part below "#elif defined(JS_CODEGEN_NONE)" (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1325495#c11)

Comment 59

[John Paul Adrian Glaubitz]

Attachment: 0011-Bug-1325771-js-src-Make-sure-Cell-has-at-least-4-byt.patch

Updated according to: https://bugzilla.mozilla.org/show_bug.cgi?id=1325771#c51

Comment 60

[John Paul Adrian Glaubitz]

Attachment: 0012-Bug-1325771-js-src-Make-sure-shadow-Shape-has-at-lea.patch

New patch. Since shadow::Shape is not derived from Cell, it cannot inherit the alignment from that class unfortunately.

Comment 61

[Mike Hommey [:glandium]]

(In reply to John Paul Adrian Glaubitz from comment #57)
> (In reply to John Paul Adrian Glaubitz from comment #41)
> > (In reply to Cameron McCormack (:heycam) from comment #38)
> > > Would be good to mention the reason for the alignment, since it's not
> > > obvious, e.g. "ensure alignment is at least 4 bytes, for m68k".
> > 
> > The default alignment on m68k is 2 bytes, not 4. That's why we have to
> > enforce 4 bytes alignment.
> > 
> > > Is there any reason not to use MOZ_ALIGNED_DECL?  That would save having to
> > > add the #ifndef block at the top.
> > 
> > I didn't know MOZ_ALIGNED_DECL exists :).
> 
> I have looked into this and it seems that MOZ_ALIGNED_DECL isn't usable in
> this case. This macro is designed for instantiating aligned objects, but not
> for declaring them. Therefore, MOZ_ALIGNED_DECL(struct Cell, 4); doesn't
> work, unfortunately.

MOZ_ALIGNED_DECL(struct Cell {
  ...
}, 4);

would work. I'm not sure it's much better, though.

Considering how MOZ_ALIGNED_DECL is not very used, it might be worth inverting its arguments, that would make it somehow better.

Comment 62

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #61)
> MOZ_ALIGNED_DECL(struct Cell {
>   ...
> }, 4);
> 
> would work. I'm not sure it's much better, though.

Ech, I just thought I had verified that this doesn't work (I tried with gcc in an m68k chroot). I must have had a syntax error in that, you're right.

> Considering how MOZ_ALIGNED_DECL is not very used, it might be worth
> inverting its arguments, that would make it somehow better.

Indeed. It looks like it's being used in js/src/jit/x86/MacroAssembler-x86.cpp only. Can't find any other references with "git grep MOZ_ALIGNED_DECL".

I'm open to suggestions in any case.

Comment 63

[Jan de Mooij [:jandem]]

Comment on attachment 8825670 [details]
Bug 1325771 - js:jit: Use PowerPC atomic operations on m68k.

https://reviewboard.mozilla.org/r/103772/#review105468

I r+'d the second version.

Comment 64

[Jan de Mooij [:jandem]]

(In reply to Mike Hommey [:glandium] from comment #40)
> Actually, I'd expect the patch as is to break the build on MSVC.

Any thoughts on this, before I review the other patches?

Comment 65

[John Paul Adrian Glaubitz]

(In reply to Jan de Mooij [:jandem] from comment #64)
> (In reply to Mike Hommey [:glandium] from comment #40)
> > Actually, I'd expect the patch as is to break the build on MSVC.
> 
> Any thoughts on this, before I review the other patches?

I can test this on MSVC2015 and Windows 10.

Comment 66

[Mike Hommey [:glandium]]

(In reply to Jan de Mooij [:jandem] from comment #64)
> (In reply to Mike Hommey [:glandium] from comment #40)
> > Actually, I'd expect the patch as is to break the build on MSVC.
> 
> Any thoughts on this, before I review the other patches?

I had misread the __attribute__ define thing, but I still think MOZ_ALIGNED_DECL would be better, albeit, after reordering its arguments.

Comment 67

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #66)
> I had misread the __attribute__ define thing, but I still think
> MOZ_ALIGNED_DECL would be better, albeit, after reordering its arguments.

Ok, thanks. I will rewrite the patches in question within the next days and report back.

Comment 68

[Jeff Walden [:Waldo]]

Comment on attachment 8825669 [details]
Bug 1325771 - mfbt:tests: Handle targets with less strict alignment in TestPair.

https://reviewboard.mozilla.org/r/103770/#review107226

I'm not particularly satisfied with the forms either of those size expressions take.  Partly because all these particulars are ABI-specific and apparently don't fully accord with intuition.  And as a consequence, I can't make sense of exactly what/how the expressions correspond to the size as a consequence.  I'd prefer, somehow, if we separated out the special-case from the intuitive case -- something like

    (sizeof(int) == 4 && alignof(int) == 2 && sizeof(long) == 4 && alignof(long) == 4)
    ? ...
    : 2 * sizeof(long)

so that someone can work through the logic of how the size is computed.  But I've been staring at this too long, so go with what you have if you can't somehow clean it up this way.

Comment 69

[Jeff Walden [:Waldo]]

Comment on attachment 8825669 [details]
Bug 1325771 - mfbt:tests: Handle targets with less strict alignment in TestPair.

https://reviewboard.mozilla.org/r/103770/#review107238

Comment 70

[John Paul Adrian Glaubitz]

Thanks for the review so far and the very helpful comments.

Some of the patches need to be rebased now which I have already done in my local branch. Once I have modified the alignment patches to use MOZ_ALIGNED_DECL() with reordered parameters, I will provide updated patches.

Comment 71

[Jeff Walden [:Waldo]]

(In reply to Mike Hommey [:glandium] from comment #37)
> The double-conversion one was fixed upstream close to three years ago,
> apparently. Maybe we should consider an update?

Yeah, fair.  Filed bug 1332797 on this, will address shortly.

Comment 72

[Jan de Mooij [:jandem]]

Comment on attachment 8825666 [details]
Bug 1325771 - js:jit: Make sure JitCode has at least 4 bytes alignment.

https://reviewboard.mozilla.org/r/103764/#review107444

Clearing review while waiting for updated patches.

Comment 73

[Jan de Mooij [:jandem]]

Comment on attachment 8825667 [details]
Bug 1325771 - js:vm: Make sure AccessorShape and Shape have at least 4 bytes alignment.

https://reviewboard.mozilla.org/r/103766/#review107446

Clearing review while waiting for updated patches.

Comment 74

[John Paul Adrian Glaubitz]

(In reply to Jan de Mooij [:jandem] from comment #72)
> Clearing review while waiting for updated patches.

Thanks. I have already updated and rebased my patches, dropped the Double-Conversion part but I'm having issues now with MOZ_ALIGNED_DECL with the reordered parameters.

I first reordered the parameter order as glandium suggested:

> https://github.com/glaubitz/gecko-dev/commit/d47f7c4948ad20c2e12eee0cf585cb55593e2dfa

Then I updated my alignment patches to use MOZ_ALIGNED_DECL:

> https://github.com/glaubitz/gecko-dev/commit/904f962881f078b58a62de94030cca0d2e13c4b6
> https://github.com/glaubitz/gecko-dev/commit/4691974d92468e6ef79fe4ba7098a49f37c6134f
> https://github.com/glaubitz/gecko-dev/commit/447ddb38057b20466d05b45673ba8f6b4fd82d5a

Problem is that this doesn't work. The compiler bails out with:

/build/firefox-I68ZeU/firefox-50.1.0/build-browser/dist/include/nsCSSValue.h:801:2: error: macro "MOZ_ALIGNED_DECL" passed 5 arguments, but takes just 2
 });
  ^

I tried a small sample code with a class with just two members and the approach with MOZ_ALIGNED_DECL with the reordered parameters worked there. It doesn't work with the actual Firefox code and I can't see at the moment what's wrong, but I assume the preprocessor is confused by something.

Comment 75

[John Paul Adrian Glaubitz]

Ok, so I have been testing this quite thoroughly and honestly can't make MOZ_ALIGNED_DECL() get to work.

My previous approach with __attribute((aligned(4))); works, but using MOZ_ALIGNED_DECL() with such a big type definitions seems to confuse the pre-processor.

Comment 76

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1325771-Add-m68k-as-target-architecture-to-mozbu.patch

Comment 77

[John Paul Adrian Glaubitz]

Attachment: 0002-Bug-1325771-xpcom-Fix-syntax-error-in-PrepareAndDisp.patch

Comment 78

[John Paul Adrian Glaubitz]

Attachment: 0003-Bug-1325771-xpcom-Fix-type-of-result-in-NS_InvokeByI.patch

Comment 79

[John Paul Adrian Glaubitz]

Attachment: 0004-Bug-1325771-mfbt-tests-Define-RETURN_INSTR-for-m68k-.patch

Comment 80

[John Paul Adrian Glaubitz]

Attachment: 0005-Bug-1325771-media-webrtc-Add-platform-defines-for-m6.patch

Comment 81

[John Paul Adrian Glaubitz]

Attachment: 0006-Bug-1325771-ipc-chromium-Add-platform-defines-for-m6.patch

Comment 82

[John Paul Adrian Glaubitz]

Attachment: 0007-Bug-1325771-mfbt-tests-Handle-targets-with-less-stri.patch

Comment 83

[John Paul Adrian Glaubitz]

Attachment: 0008-Bug-1325771-js-jit-Use-PowerPC-atomic-operations-on-.patch

Comment 84

[John Paul Adrian Glaubitz]

Attachment: 0009-Bug-1325771-mfbt-Reorder-parameters-for-MOZ_ALIGNED_.patch

Comment 85

[John Paul Adrian Glaubitz]

Attachment: 0010-Bug-1325771-js-src-Make-sure-Cell-has-at-least-4-byt.patch

Comment 86

[John Paul Adrian Glaubitz]

Attachment: 0011-Bug-1325771-js-src-Make-sure-shadow-Shape-has-at-lea.patch

Comment 87

[John Paul Adrian Glaubitz]

Attachment: 0012-Bug-1325771-layout-style-Make-sure-nsCSSValue-has-at.patch

Comment 88

[John Paul Adrian Glaubitz]

Attachment: 0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch

Comment 89

[John Paul Adrian Glaubitz]

Ok, Michael Karcher finally found a solution. Using __VA_ARGS__ for MOZ_ALIGNED_DECL() helps resolving the issue. __VA_ARGS__ is a C++11-supported feature.

Comment 90

[Nathan Froyd [:froydnj]]

Please stop asking for review on patches that have already been reviewed and contain no changes.

Comment 91

[John Paul Adrian Glaubitz]

(In reply to Nathan Froyd [:froydnj] from comment #90)
> Please stop asking for review on patches that have already been reviewed and
> contain no changes.

I just rebased the patches to make sure they still apply cleanly.

Comment 92

[Jan de Mooij [:jandem]]

Comment on attachment 8831556 [details] [diff] [review]
0007-Bug-1325771-mfbt-tests-Handle-targets-with-less-stri.patch

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

Waldo reviewed exactly this patch, right? I compared it to https://reviewboard.mozilla.org/r/103770/diff/1#index_header

Clearing my review, but you can probably land this with r=jwalden then.

Comment 93

[Nathan Froyd [:froydnj]]

(In reply to John Paul Adrian Glaubitz from comment #91)
> (In reply to Nathan Froyd [:froydnj] from comment #90)
> > Please stop asking for review on patches that have already been reviewed and
> > contain no changes.
> 
> I just rebased the patches to make sure they still apply cleanly.

I think a good rule of thumb is that if you didn't have to perform manual fixups on the patch when you rebased, you don't need to ask for review on the patch again.

Comment 94

[John Paul Adrian Glaubitz]

(In reply to Nathan Froyd [:froydnj] from comment #93)
> I think a good rule of thumb is that if you didn't have to perform manual
> fixups on the patch when you rebased, you don't need to ask for review on
> the patch again.

I agree. I'll remember this next time :).

Comment 95

[Jeff Walden [:Waldo]]

Bug 1332797 landed, so double-conversion code shouldn't have any m68k-relevant issues any more.  (Assuming it sticks, which I believe it should, because it had some tryservering.)

The patch-review, if it's the same as the previous patch I reviewed here (maybe as noted in comment 93?), shouldn't need any additional looking.  But I'm out of time now to verify their identicality.

Comment 96

[John Paul Adrian Glaubitz]

> The patch-review, if it's the same as the previous patch I reviewed here (maybe as noted in comment 93?), shouldn't need any additional looking.  But I'm out of time now to verify their identicality.

Yes, they're identical, see:

old: https://reviewboard.mozilla.org/r/103770/diff/1#index_header
new: https://bugzilla.mozilla.org/attachment.cgi?id=8831556&action=edit

So, I think acking should be fine.

Comment 97

[Jeff Walden [:Waldo]]

Comment on attachment 8831556 [details] [diff] [review]
0007-Bug-1325771-mfbt-tests-Handle-targets-with-less-stri.patch

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

...okay.  I'm confused why I'm being asked to review again, but I can certainly just copy-paste my comments on it from last time.

I'm not particularly satisfied with the forms either of those size expressions take.  Partly because all these particulars are ABI-specific and apparently don't fully accord with intuition.  And as a consequence, I can't make sense of exactly what/how the expressions correspond to the size as a consequence.  I'd prefer, somehow, if we separated out the special-case from the intuitive case -- something like

    (sizeof(int) == 4 && alignof(int) == 2 && sizeof(long) == 4 && alignof(long) == 4)
    ? ...
    : 2 * sizeof(long)

so that someone can work through the logic of how the size is computed.  But I've been staring at this too long, so go with what you have if you can't somehow clean it up this way.

Comment 98

[Mike Hommey [:glandium]]

Comment on attachment 8831550 [details] [diff] [review]
0001-Bug-1325771-Add-m68k-as-target-architecture-to-mozbu.patch

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

This already had a r+

Comment 99

[Mike Hommey [:glandium]]

Comment on attachment 8831553 [details] [diff] [review]
0004-Bug-1325771-mfbt-tests-Define-RETURN_INSTR-for-m68k-.patch

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

This already had a r+

Comment 100

[Mike Hommey [:glandium]]

Comment on attachment 8831555 [details] [diff] [review]
0006-Bug-1325771-ipc-chromium-Add-platform-defines-for-m6.patch

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

This already had a r+

Comment 101

[Mike Hommey [:glandium]]

Comment on attachment 8831562 [details] [diff] [review]
0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch

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

This should be patch 9.5 (after patch 9, before patch 10), or folded in patch 9. You should also add a few words in the commit message about why. It's better if people don't have to read the comments in this bug to figure that out in the future.

This will also need to run through try because MSVC has some issues with __VA_ARGS__. I don't think they'd been hit with this change and patch 12, but better safe than sorry.

Comment 102

[Jeff Walden [:Waldo]]

Comment on attachment 8831562 [details] [diff] [review]
0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch

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

Um, actually, this is wrong.  MOZ_ALIGNED_DECL is intended for use in declaring an aligned *variable*.  It is not intended for use in declaring that a type should have particular alignment.

Instead, you should use C++11 alignas.  And additionally you very likely need to tag the type as MOZ_NON_PARAM and potentially fix any places where it's passed as an argument by value, to instead pass by reference or pointer.

Comment 103

[Mike Hommey [:glandium]]

BTW, considering rust is now mandatory, until there is a rust compiler backend for m68k, this bug is not going to do anything useful.

Comment 104

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #103)
> BTW, considering rust is now mandatory, until there is a rust compiler
> backend for m68k, this bug is not going to do anything useful.

So, it's not going to get merged now?

Comment 105

[Jeff Walden [:Waldo]]

I think we would accept patches, but you have to recognize that merely landing the patches in this bug won't let you build.  And having a Rust m68k backend is (I presume) much more work than the patches here.  Without serious effort on the latter front, reviewers will probably naturally assign the patchwork here lower priority, because it won't be usable until that big project occurs.

Comment 106

[John Paul Adrian Glaubitz]

I consider this hostile behavior towards the community and downstream. As a Debian Developer, I will make use of the means we have (CTTE, GR) to try prevent these incompatible versions of Firefox from entering Debian.

We have had this fight with Mozilla and the trademark issue in the past before, so we will not silently accept this decision either.

Comment 107

[Jan de Mooij [:jandem]]

(In reply to John Paul Adrian Glaubitz from comment #106)
> I consider this hostile behavior towards the community and downstream.

I understand your frustration. As a developer it's nice to see our code run on all these different platforms, and I appreciate the heroic efforts from you and others to keep all these ports working.

But please try to put yourself in our shoes for a minute. Rust offers us some game changing benefits (in terms of security, productivity, performance) that we really need in the very competitive browser world. We have been working towards this for years so it's also not something that should come as a huge surprise. Should we keep these improvements from 99.999% of our users because there exist some platforms without a Rust backend? What would you do?

Comment 108

[Jeff Walden [:Waldo]]

I ask this as delicately as I can, but what precise action here do you consider hostile behavior?  I truly don't understand 

My understanding from the last time I looked at Debian (it's been awhile, I'm more a Fedora person, myself -- but to each his own), is that it really really really wants to support a bunch of different architectures and hardware combinations.

Mozilla and Firefox, on the other hand, don't attempt to function so broadly.  We have a "tier" system into which platforms/architectures are categorized.  Windows (32/64 Intel-compatible for now, conceivably ARM eventually), OS X (64 Intel, and maybe 32 but it's dying), and Linux (32/64 Intel) are tier 1.  We do not break these; breakage is fixed or backed out in short order.  Below tier 1 we have 2/3/etc. that we, to varying degrees, try not to break but make no guarantees about.  We review patches for this functionality, and we try to stub out functionality in them so more-informed people can do complete fixes, and we'll land patches that are properly reviewed for non-tier 1 platforms, but it's not a strong focus.

This is simply different fundamental ideas about compatibility.  You/Debian have made a perfectly reasonable choice to try to support as many things as you can.  We/Mozilla/Firefox have made a different, equally perfectly reasonable choice to focus our efforts on a few platforms, while lending what hand we can to those who want to work on more.  It's just different priorities and philosophies.  It doesn't seem like hostility to me.  What am I missing?

Comment 109

[Mike Hommey [:glandium]]

With my Debian hat on, I'll add that it's Debian's problem if it doesn't have Rust packages on architectures other than x86, x86-64 and arm64 (and in fact, it doesn't even have cargo on arm64). Clang/LLVM are available on all Debian release architectures, and more. Rust has tier-3 support for most if not all Debian release architectures, and the Rust community has been very proactive about that. So, really, you can't blame Mozilla or the Rust community for the fact that Debian can't build Firefox >= 54 on architectures other than x86 and x86-64.

(In reply to John Paul Adrian Glaubitz from comment #106)
> As a Debian Developer, I will make use of the means we have (CTTE, GR) to try
> prevent these incompatible versions of Firefox from entering Debian.

Talk about hostility...

Are you also going to backport security fixes on the older version of Firefox you imply Debian should stick with?

BTW, Debian doesn't ship anything else than the ESR versions, and ESR52, which doesn't require Rust, is going to be supported until at least mid 2018. That leaves more than a year for Debian to make Rust work on all the architectures it cares about. Oh, and FYI, IIRC, the Debian security team prefers Firefox updates wth Rust, even if it means on x86 and x86-64 only, than the alternative.

Now, this is *very* offtopic in this bug, please stop this discussion here.

Comment 110

[Martin Husemann]

Just a side note: support for all big endian platforms is gone with making Skia the only supported software rendering backend (see #1323303). Building without skia is already broken in firefox 51.

Comment 111

[Mike Hommey [:glandium]]

(In reply to Martin Husemann from comment #110)
> Building without skia is already broken in firefox 51.

That was fixed in bug 1319374 (for 52). But yes, 53 makes skia mandatory. But nothing stops anyone from going to skia upstream and fixing big-endian support in skia.

But again, offtopic.

Comment 112

[Martin Husemann]

Thanks for the pointer, very helpful!

Comment 113

[John Paul Adrian Glaubitz]

(In reply to Jan de Mooij [:jandem] from comment #107)
> But please try to put yourself in our shoes for a minute. Rust offers us
> some game changing benefits (in terms of security, productivity,
> performance) that we really need in the very competitive browser world. We
> have been working towards this for years so it's also not something that
> should come as a huge surprise. Should we keep these improvements from
> 99.999% of our users because there exist some platforms without a Rust
> backend? What would you do?

Well, Mozilla's problem is that you are claiming to be a community project, yet you alienate contributors by letting them review and fix their patches just to discard everything in the end. To be honest, I feel played by you guys.

I also do not think that it's a particularly good idea to port Firefox to Rust when the language isn't really a 1.0 release (unlike what the actual version number claims). You still have lots of unstable inferfaces, missing portability and you often read that lots of project require certain versions of Rust to compile. I don't consider this production ready.

Comment 114

[John Paul Adrian Glaubitz]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #108)
> I ask this as delicately as I can, but what precise action here do you
> consider hostile behavior?  I truly don't understand 

You are turning a project which is supposed to be a community project into one which is solely profit-oriented. You break portability, you throw away XUL and you kill things like Thunderbird.

How do you expect external people still to be willing to contribute to Firefox? You are basically focusing on Windows/x86 only. I'm already seeing the other operating system ports getting killed. I wouldn't be surprised because you want to be competitive (with a 7% market share ...).

> My understanding from the last time I looked at Debian (it's been awhile,
> I'm more a Fedora person, myself -- but to each his own), is that it really
> really really wants to support a bunch of different architectures and
> hardware combinations.

Which is a very good thing that you will realize very quickly when you fire up your favorite search engine and search for things like "Intel AMT", "Intel Binary Blobs" and "Intel Hardware Backdoors".

There are lots of people in the community who work on projects like RISC-V which will bring people free and open source hardware with actual open source CPUs. The Intel vendor-lockin you are practicing here is undermining these efforts.

> Mozilla and Firefox, on the other hand, don't attempt to function so
> broadly.

Yet you claim to be a community project.

> We have a "tier" system into which platforms/architectures are
> categorized.  Windows (32/64 Intel-compatible for now, conceivably ARM
> eventually)

With ARM still not having a stable port of Rust. And, no, building and testing a compiler on QEMU is not acceptable at all for software that is supposed to be used in production. You are claiming that you want to use Rust to make the code more robust and more reliable, yet you are relying on unreliable environments like QEMU to test your code.

> , OS X (64 Intel, and maybe 32 but it's dying), and Linux (32/64
> Intel) are tier 1.  We do not break these; breakage is fixed or backed out
> in short order.

You probably said that about the other ports as well.

> Below tier 1 we have 2/3/etc. that we, to varying degrees,
> try not to break but make no guarantees about.  We review patches for this
> functionality, and we try to stub out functionality in them so more-informed
> people can do complete fixes, and we'll land patches that are properly
> reviewed for non-tier 1 platforms, but it's not a strong focus.

Rust is currently really only fully working on x86 only. Claiming that anything else will actually work in a stable manner is very dishonest, to say the least.

> This is simply different fundamental ideas about compatibility.  You/Debian
> have made a perfectly reasonable choice to try to support as many things as
> you can.  We/Mozilla/Firefox have made a different, equally perfectly
> reasonable choice to focus our efforts on a few platforms, while lending
> what hand we can to those who want to work on more.  It's just different
> priorities and philosophies.  It doesn't seem like hostility to me.  What am
> I missing?

You are missing the point that people don't use Firefox because they like your branding or your image so much. They use Firefox because it's (currently) a very versatile (XUL Addons) browser with a broad range of portability to various architectures and operating systems. You are killing both in the hope that you will be able to compete with a multi-billion dollar search engine company that also happens to make a browser.

Comment 115

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #109)
> With my Debian hat on, I'll add that it's Debian's problem if it doesn't
> have Rust packages on architectures other than x86, x86-64 and arm64 (and in
> fact, it doesn't even have cargo on arm64).

No, it's not Debian's problem. It's Mozilla's problem because Rust's codebase is far from being production grade that it actually works on anything beyond x86. Don't blame us if your upstream project isn't ready for production yet.

I find it unacceptable that you - being a DD yourself - put the blame on the Rust maintainers saying that they are responsible for Rust not being as portable as it should be. Ximin has sent tons of PRs through github and yet rustc is still not building on anything but i386, amd64 and arm64.

> Clang/LLVM are available on all
> Debian release architectures, and more.

LLVM only ever has 100% test coverage on x86 targets. From Sylvestre's post on the ML, he says, he's not even running the testsuite on anything but i386 and amd64. I have seen too many compiler bugs that I would actually trust a compiler that doesn't pass its own testsuite.

> Rust has tier-3 support for most if
> not all Debian release architectures, and the Rust community has been very
> proactive about that. So, really, you can't blame Mozilla or the Rust
> community for the fact that Debian can't build Firefox >= 54 on
> architectures other than x86 and x86-64.

Yes, we can. And I find it really dishonest from you that you are trying to shift the blame onto Debian here.

> (In reply to John Paul Adrian Glaubitz from comment #106)
> > As a Debian Developer, I will make use of the means we have (CTTE, GR) to try
> > prevent these incompatible versions of Firefox from entering Debian.
> 
> Talk about hostility...

I didn't start this. You did. You are steering away from the community. You are blaming downstream for your upstream code being incomplete and broken.

> Are you also going to backport security fixes on the older version of
> Firefox you imply Debian should stick with?

We have done this in the past and we have fought Mozilla in the past over the trademark issue.

> BTW, Debian doesn't ship anything else than the ESR versions, and ESR52,
> which doesn't require Rust, is going to be supported until at least mid
> 2018. That leaves more than a year for Debian to make Rust work on all the
> architectures it cares about. Oh, and FYI, IIRC, the Debian security team
> prefers Firefox updates wth Rust, even if it means on x86 and x86-64 only,
> than the alternative.

Debian is a very democratic project and no single entity gets to make such decisions and you are supposed to know that.

Comment 116

[Mike Hommey [:glandium]]

(In reply to John Paul Adrian Glaubitz from comment #115)
> > Are you also going to backport security fixes on the older version of
> > Firefox you imply Debian should stick with?
> 
> We have done this in the past and we have fought Mozilla in the past over
> the trademark issue.

Where "We" was me, and a) it was not sustainable (and it's even less so now), b) many security issues weren't fixed.
 
> > BTW, Debian doesn't ship anything else than the ESR versions, and ESR52,
> > which doesn't require Rust, is going to be supported until at least mid
> > 2018. That leaves more than a year for Debian to make Rust work on all the
> > architectures it cares about. Oh, and FYI, IIRC, the Debian security team
> > prefers Firefox updates wth Rust, even if it means on x86 and x86-64 only,
> > than the alternative.
> 
> Debian is a very democratic project and no single entity gets to make such
> decisions and you are supposed to know that.

Debian is more a do-ocracy than a democracy. There is only one democratic process (GRs), and they are barely involved in package maintenance.

Comment 117

[Jan de Mooij [:jandem]]

(In reply to John Paul Adrian Glaubitz from comment #113)
> yet you alienate contributors by letting them review and fix their patches
> just to discard everything in the end. To be honest, I feel played by you
> guys.

Requiring Rust is not something that was decided last week, as you seem to suggest. People have been working towards this for years. I mentioned this 3 weeks ago (bug 1329194 comment 18) and there have been bugs + mailing list threads on this going back months if not years.

Discussing this here is offtopic so this is my last comment.

Comment 118

[Nathan Froyd [:froydnj]]

I went to go land these patches this morning (they have minimal impact, and getting them landed can at least start the process for them to be uplifted for ESR52), and saw that part 13 has been r-.  John, can you please fix according to Jeff's review in comment 102?

Comment 119

[Mike Hommey [:glandium]]

(In reply to John Paul Adrian Glaubitz from comment #115)
> (In reply to Mike Hommey [:glandium] from comment #109)
> > With my Debian hat on, I'll add that it's Debian's problem if it doesn't
> > have Rust packages on architectures other than x86, x86-64 and arm64 (and in
> > fact, it doesn't even have cargo on arm64).
> 
> No, it's not Debian's problem. It's Mozilla's problem because Rust's
> codebase is far from being production grade that it actually works on
> anything beyond x86. Don't blame us if your upstream project isn't ready for
> production yet.

Fedora doesn't agree with you.
https://koji.fedoraproject.org/koji/buildinfo?buildID=829097

Comment 120

[Jeff Walden [:Waldo]]

(In reply to John Paul Adrian Glaubitz from comment #114)
> You are turning a project which is supposed to be a community project into
> one which is solely profit-oriented. You break portability, you throw away
> XUL and you kill things like Thunderbird.

It seems to me, that you literally equate "community project" with "supports every platform/project/feature while guaranteeing stability".

But time and resources are finite.  Pursuing one goal requires (to a degree) deferring others.  Prioritizing support for relatively-unused platforms, versus focusing on other core competencies, is a value judgment on which reasonable free/open source projects can differ.  Even if those less-used platforms are relatively worse for it, or the Intel/ARM duopoly are relatively better for it.

I'm sure I'm not going to change your mind on the merits of that debate, but it would be nice if you recognized that it *was* a debate.  Either way, like jandem, I think I'm going to have to call it quits on further discussion.  I'm still more than happy to review updated patches for anything where my input's helpful, but I'm afraid I'm out of time for philosophical discussion.

Comment 121

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #119)
> Fedora doesn't agree with you.
> https://koji.fedoraproject.org/koji/buildinfo?buildID=829097

error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
thread 'rustc' panicked at 'assertion failed: `(left == right)` (left: `Indirect`, right: `Direct`)', src/librustc_trans/abi.rs:100
note: Run with `RUST_BACKTRACE=1` for a backtrace.
make[1]: *** [Makefile:4: all] Error 101

> https://kojipkgs.fedoraproject.org//packages/rust/1.14.0/2.fc26/data/logs/ppc64/build.log

OK

Comment 122

[John Paul Adrian Glaubitz]

(In reply to Nathan Froyd [:froydnj] from comment #118)
> I went to go land these patches this morning (they have minimal impact, and
> getting them landed can at least start the process for them to be uplifted
> for ESR52), and saw that part 13 has been r-.  John, can you please fix
> according to Jeff's review in comment 102?

I'm not sure what to do here. When using the alignment attribute of gcc, it doesn't really matter whether the object is a type or a variable, in both cases __attribute__ is applied to the object.

Comment 123

[John Paul Adrian Glaubitz]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #120)
> It seems to me, that you literally equate "community project" with "supports
> every platform/project/feature while guaranteeing stability".

Something like a compiler should have good portability, yes.

> I'm sure I'm not going to change your mind on the merits of that debate, but
> it would be nice if you recognized that it *was* a debate.  Either way, like
> jandem, I think I'm going to have to call it quits on further discussion. 
> I'm still more than happy to review updated patches for anything where my
> input's helpful, but I'm afraid I'm out of time for philosophical discussion.

I'm not the person who started the discussion here. I wanted to get my last set of fixes in. Then Mike questioned my work and started this discussion in Comment 103.

Comment 124

[John Paul Adrian Glaubitz]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #102)
> Comment on attachment 8831562 [details] [diff] [review]
> 0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch
> 
> Review of attachment 8831562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Um, actually, this is wrong.  MOZ_ALIGNED_DECL is intended for use in
> declaring an aligned *variable*.  It is not intended for use in declaring
> that a type should have particular alignment.

But the syntax is the same, independent whether it's a variable or a type.

> Instead, you should use C++11 alignas.  And additionally you very likely
> need to tag the type as MOZ_NON_PARAM and potentially fix any places where
> it's passed as an argument by value, to instead pass by reference or pointer.

If I understand the documentation correctly [1], alignas(4) would result in an error on 64-bit architectures:

> If the strictest (largest) alignas on a declaration is weaker than the alignment it would have
> without any alignas specifiers (that is, weaker than its natural alignment or weaker than alignas
> on another declaration of the same object or type), the program is ill-formed:

The natural alignment on 64-bit targets would be 8 bits (if the struct/class contains a pointer, for example) and declaring the objects with alignas(4) would therefore result in an error, wouldn't it? So I'm not sure whether alignas is actually usable here.

> [1] http://en.cppreference.com/w/cpp/language/alignas

Comment 125

[Sylvestre Ledru [:Sylvestre]]

> I consider this hostile behavior towards the community and downstream. As a
> Debian Developer, I will make use of the means we have (CTTE, GR) to try
> prevent these incompatible versions of Firefox from entering Debian.
> 
> We have had this fight with Mozilla and the trademark issue in the past
> before, so we will not silently accept this decision either.
As a Debian developer, please, I ask you to stop using "We" and just use "I".
AFAIK, you just represent yourself now.


> LLVM only ever has 100% test coverage on x86 targets. From Sylvestre's post on the ML, he says, he's not even running the testsuite 
> on anything but i386 and amd64.
Not exactly, I am running all the testsuites on all archs. I won't fail the build if they are test failure. On most the architectures, the failure rate is very low and not concerning.

> Rust is currently really only fully working on x86 only. Claiming that anything else will actually work in a stable manner is very 
> dishonest, to say the least.
arm64 is available in debian and the testsuite fully executes without failure.

Comment 126

[John Paul Adrian Glaubitz]

Hello!

Since we can't seem to come to an agreement with the alignment issues, how about just merging the remaining fixes for now?

I don't want these patches to get lost. I invested some time to get these patches out and it would be great if these efforts had not been in vain.

I can still open a new bug report for the alignment issues.

Thanks,
Adrian

Comment 127

[John Paul Adrian Glaubitz]

Hello!

Is there anything I can do to get this resolved?

Thanks,
Adrian

Comment 128

[Jeff Walden [:Waldo]]

Sorry, missed reading the last few emails here.

(In reply to John Paul Adrian Glaubitz from comment #124)
> But the syntax is the same, independent whether it's a variable or a type.

That the syntax is the same, doesn't mean the macro *semantically* applies to both cases.  (Note: I care about the macro, not whatever compiler-specific gunk it might expand to.)  Consider, in the pre-C++11-guaranteed days, a macro designed to make classes final, when possible:

/**
 * Macro to apply to a class to prevent its being used as a base class,
 * when possible.
 */
#if __cplusplus >= 201103L
#  define MOZ_FINAL_CLASS(cls) \
     cls final
#else
#  define MOZ_FINAL_CLASS(cls) \
     cls
#endif

You could also apply that macro to virtual member functions:

  struct S
  {
     MOZ_FINAL_CLASS(virtual void foo()) { }
  };

The macro would "work" in that situation.  But that doesn't mean it wouldn't be a misuse of the macro, contrary to its intended purpose.  (And the macro name would be pretty wrong as well.)  (No, it's not relevant that for *that* particular pattern you can/could just have MOZ_FINAL and use it in both cases -- this is just a simple, illustrative instance of the scenario.)

> If I understand the documentation correctly [1], alignas(4) would result in
> an error on 64-bit architectures:

Okay, so then just apply multiple alignment attributes:

  class alignas(4) alignas(void*) nsCSSValue {
    ...
  };

As long as the maximum of all of them isn't less than the struct's natural alignment, things should be okay.  (It's kind of too bad smaller alignas than the default aren't simply ignored, rather than being an error.  But oh well.)

Comment 129

[John Paul Adrian Glaubitz]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #128)
> Sorry, missed reading the last few emails here.

No problem.

> That the syntax is the same, doesn't mean the macro *semantically* applies
> to both cases.  (Note: I care about the macro, not whatever
> compiler-specific gunk it might expand to.)  Consider, in the
> pre-C++11-guaranteed days, a macro designed to make classes final, when
> possible:

Ok, thanks for the explanation.

> > If I understand the documentation correctly [1], alignas(4) would result in
> > an error on 64-bit architectures:
> 
> Okay, so then just apply multiple alignment attributes:
> 
>   class alignas(4) alignas(void*) nsCSSValue {
>     ...
>   };
> 
> As long as the maximum of all of them isn't less than the struct's natural
> alignment, things should be okay.  (It's kind of too bad smaller alignas
> than the default aren't simply ignored, rather than being an error.  But oh
> well.)

I will give this a try and verify it works as expected.

Comment 130

[John Paul Adrian Glaubitz]

Ok, I have done some testing with alignas() and it turns out we actually cannot use it. The reason is that alignas() is broken on gcc from the very beginning (since 4.8.x) and specifying multiple alignas() will not work.

What happens is that gcc will always just use the last specified alignas() when multiple alignas() are specified. So, writing "alignas(4) alignas(void*)" will always result in the alignment being alignof(void*) instead of using the larger one of multiple provided alignas.

The relevant gcc bug report is 64236 [1].

> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64236

Comment 131

[Jeff Walden [:Waldo]]

Then do

    alignas(tl::Max<alignof(void*), 4>::value)

instead.

Comment 132

[Michael Karcher]

Preface: Please assume CHAR_BITS == 8 for the following message, so sizeof(int32_t) is assumed to be 4, and alignof(int32_t) is assumed to be at max 4. I don't think Mozilla is targetting platforms with other CHAR_BITS values at the moment, and the message should apply even there "mutatis mutandis".

The alignas(max<...>) suggestion is perfect to work around the problem of gcc mishandling multiple alignas, but alas it does not make alignas work as general replacement for __attribute__((aligned(x))). The following stuff should work in my oppinion (and does in fact work with __attribute__((aligned(x))) as well as __declspec(...) applied at the right place:

a)
    struct ALIGN_4 Point3D_I {
        int32_t x, y, z;
    }
    static_assert(alignof(Point3D) == 12, "superflous padding in Point3D");

b)
    struct ALIGN_4 Point3D_D {
        double x, y, z;
    }

If we use "#define ALIGN_4 alignas(4) alignas(void*)" (or the equivalent suggested by Waldo that actually works with gcc), (a) fails on amd64, because aligning to void* causes an unneeded alignment to 8 bytes. I don't think wasting memory on amd64 because of a patch to help m68k is acceptable to the Mozilla project.

Furthermore (b) is invalid according to the C++11 standard on architectures where double has a stricter alignment requirement than void* (e.g. on x32), because ALIGN_4 will cause four-byte alignment, which conflicts the natural 8-byte-alignment of double. This can be worked around by maxing in double, too (like "#define ALIGN_4 alignas(4) alignas(void*) alignas(double)") with the end result, that the issue of memory wasting is even more exaggregated.

I thought about the issue for some time, and the only fully general and conforming issue that came to my mind is inheritance based:

    class alignas(4) align4 {};

Derive from this class, and you instantly get 4-byte alignment. Memory usage is zero, because it is an empty base class. As the C++ compiler figures out the alignment for the derived class by taking the maximum of the alignment required by the base classes and the members, this will never create unnecessary padding. There is a drawback though, if there are multiple align4 base subobjects in the same class, like this:

    class base : public align4 { int32_t x; };
    class derived1 : public base, public align4 { char y; };
    class derived2 : public derived1, public align4  { char z };

(a situation like this seems pointless, but can arise if the derivation from align4 slowly "crawls upwards", i.e. you first only need alignment on derived2, later you also need alignment on derived1 in general, not thinking (or even knowing) about derived2 and its align4 base class, and doing the same again later for base) In this scenario, derived2 can not be smaller than 12 bytes (although 8 bytes is obviously enough for the actual data members and the 4-Byte alignment requirement). The reason is that the three "align4" base subobjects may not be allocated to the same address and must be allocated on an address divisible by 4, so the three empty subobjects require 12 Bytes padding, which may be overlaid by members. You can avoid that be making sure that no two align4 subobjects have the same type, because then they may be allocated at the same address. It would work like this:

    template<typename T> class alignas(4) align4 {};
    class base : public align4<base> { int32_t x; };
    class derived1 : public base, public align4<derived1> { char y; };
    class derived2 : public derived1, public align4<derived2> {char z; };

Related to this, I also observed (and first considered that an issue with the align4 base class suggestion) that

    class base : public align4 { char x; };
    class derived99 : public base, public align4 { char y; };

causes class99 to have 8 bytes (according to the C++ ABI implemented by g++), although a size of 4 is obviously enough to accomodate all members. The reason for that is not the align4 base class itself, but the fact that base is a POD type ("for the aspect of layout") that carries a four-byte alignment, so it has three tail padding bytes. The C++ ABI forbids reusing the tail padding of POD objects. You observer a similar "issue" without my "align4" base class suggestion in this case:

    static_assert(alignof(int32_t) > 1, "This example needs padding bytes inserted for int32_t alignment");
    class base {
        int32_t a;
        char b;
        // 1 padding byte inserted for alignof(int32_t) == 2 (e.g. m68k)
        // 3 padding bytes inserted for alignof(int32_t) == 4 (e.g. nearly every other platform
    };
    struct derived : public base {
        char c;
    };
    // sizeof(derived) == 8 on m68k (instead of 6)
    // sizeof(derived) == 12 on other platforms (instead of 8)

Comment 133

[John Paul Adrian Glaubitz]

FWIW, we're working on Rust for m68k now. Progress is slow, but we're getting there. I can already compile some files before the compiler bails out.

Comment 134

[Nathan Froyd [:froydnj]]

(In reply to John Paul Adrian Glaubitz from comment #133)
> FWIW, we're working on Rust for m68k now. Progress is slow, but we're
> getting there. I can already compile some files before the compiler bails
> out.

Bug 1477048 removed m68k xptcall files, so putting those back (and fixing them to conform to whatever changes we make in the interim) will also be necessary.

Comment 135

[John Paul Adrian Glaubitz]

Thanks for the heads-up. I'll do that once Rust is ready. But that will still take some months until m68k support has landed in LLVM.

Comment 136

[John Paul Adrian Glaubitz]

Attachment: Bug-1325771-Enable_AtomicOperations-feeling-lucky.h_on_m68k.patch

Comment 137

[John Paul Adrian Glaubitz]

Comment on attachment 8831551 [details] [diff] [review]
0002-Bug-1325771-xpcom-Fix-syntax-error-in-PrepareAndDisp.patch

File no longer part of the source, so this patch is obsolete.

Comment 138

[John Paul Adrian Glaubitz]

Comment on attachment 8831552 [details] [diff] [review]
0003-Bug-1325771-xpcom-Fix-type-of-result-in-NS_InvokeByI.patch

File no longer present, so this patch is obsolete.

Comment 139

[John Paul Adrian Glaubitz]

Attachment: Bug-1325771-Add_m68k_as_target_architecture_to_mozbuild.patch

Comment 140

[John Paul Adrian Glaubitz]

Comment on attachment 8831562 [details] [diff] [review]
0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 141

[John Paul Adrian Glaubitz]

Comment on attachment 8831561 [details] [diff] [review]
0012-Bug-1325771-layout-style-Make-sure-nsCSSValue-has-at.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 142

[John Paul Adrian Glaubitz]

Attachment: Bug_1325771-mfbt_Reorder_parameters_for_MOZ_ALIGNED_DECL.patch

Comment 143

[Jan de Mooij [:jandem]]

Comment on attachment 9146097 [details] [diff] [review]
Bug-1325771-Enable_AtomicOperations-feeling-lucky.h_on_m68k.patch

Forwarding to Lars. I'd also suggest moving your patches to Phabricator.

Comment 144

[John Paul Adrian Glaubitz]

Attachment: Bug_1325771-mfbt_tests_Define_RETURN_INSTR_for_m68k_in_TestPoisonArea.patch

Comment 145

[John Paul Adrian Glaubitz]

Ah, I didn't know there was a Phabricator instance now. Thanks for the heads-up!

Comment 146

[Lars T Hansen [:lth]]

Comment on attachment 9146097 [details] [diff] [review]
Bug-1325771-Enable_AtomicOperations-feeling-lucky.h_on_m68k.patch

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

Sure, why not.

Out of entirely idle curiosity, what m68k hardware do you have that runs Firefox?

Comment 147

[John Paul Adrian Glaubitz]

Comment on attachment 8831560 [details] [diff] [review]
0011-Bug-1325771-js-src-Make-sure-shadow-Shape-has-at-lea.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 148

[John Paul Adrian Glaubitz]

Comment on attachment 8831559 [details] [diff] [review]
0010-Bug-1325771-js-src-Make-sure-Cell-has-at-least-4-byt.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 149

[John Paul Adrian Glaubitz]

Attachment: Bug_1325771-mfbt_tests_Handle_targets_with_less_strict_alignment_in_TestPair.patch

Comment 150

[John Paul Adrian Glaubitz]

Comment on attachment 8831555 [details] [diff] [review]
0006-Bug-1325771-ipc-chromium-Add-platform-defines-for-m6.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 151

[John Paul Adrian Glaubitz]

Comment on attachment 8831554 [details] [diff] [review]
0005-Bug-1325771-media-webrtc-Add-platform-defines-for-m6.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 152

[John Paul Adrian Glaubitz]

Comment on attachment 8831554 [details] [diff] [review]
0005-Bug-1325771-media-webrtc-Add-platform-defines-for-m6.patch

Dropping this for now as it's not required for Spidermonkey.

Comment 153

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - mfbt: Reorder parameters for MOZ_ALIGNED_DECL

Currently, MOZ_ALIGNED_DECL uses the order (_type, _align) for its
parameters. However, this order makes the code less readable when
_type is a larger object like a struct because the value for _align
would be at the end of the struct definition. By swapping the order
of _type and _align, the alignment value will always be next to
the type name, regardless how far the definition of _type extends.

Comment 154

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea

Define RETURN_INSTR for m68k in TestPoisonArea, i.e. the m68k assembly opcodes for "rts ; rts".

Comment 155

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - mfbt:tests: Handle targets with less strict alignment in TestCompactPair

Previously, the tests assumed that the alignment of int and long equals
their size. This commit fixes the tests for targets like m68k that have
sizeof(int) == 4 and alignof(int) == 2. A static helper function sizemax
was introduced as the offset of the second element in Pair<int,long>
might be either determined by its alignment requirement or the size of
the preceding int element and we use the helper function to pick the
larger of the two values.

Comment 156

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - js:jit: Enable AtomicOperations-feeling-lucky.h on m68k

This allows the build on m68k to use the atomic operations provided by GCC.

Comment 157

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - build: Add m68k as target architecture to mozbuild

Adds the basic definitions for m68k to mozbuild, allowing to build Spidermonkey.

Comment 158

[John Paul Adrian Glaubitz]

All added to Phabricator. Thanks!

Once this is merged, I'll follow up with the necessary changes for riscv64.

Comment 159

[John Paul Adrian Glaubitz]

FWIW, we're currently also working on Rust support for m68k. So we even might be able to build the whole of Firefox at some point :).

Comment 160

[Mike Hommey [:glandium]]

Comment on attachment 9146100 [details] [diff] [review]
Bug-1325771-Add_m68k_as_target_architecture_to_mozbuild.patch

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

Please use phabricator

Comment 161

[Randell Jesup [:jesup] (needinfo me)]

(In reply to John Paul Adrian Glaubitz from comment #159)

FWIW, we're currently also working on Rust support for m68k. So we even might be able to build the whole of Firefox at some point :).

I'll have to see how it runs on my 68040 Amiga A4000T.... of course 16MB ram is a problem. ;-)

Comment 162

[John Paul Adrian Glaubitz]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #161)

(In reply to John Paul Adrian Glaubitz from comment #159)

FWIW, we're currently also working on Rust support for m68k. So we even might be able to build the whole of Firefox at some point :).

I'll have to see how it runs on my 68040 Amiga A4000T.... of course 16MB ram is a problem. ;-)

There are modern accelerators that will provide a lot more power such as the Apollo Vampire and the Amiga allows RAM upgrades with cards like the BigRAMPlus up to 1 GB of RAM (or maybe more with the Vampire).

In case you're interested, the m68k LLVM backend is currently work in progress, see: https://www.bountysource.com/issues/90829856-llvm-complete-the-m68000-backend-so-it-can-be-merged-upstream

Comment 163

[John Paul Adrian Glaubitz]

This is unfortunately still blocked by the last patch review by jwalden:

https://phabricator.services.mozilla.com/D74030

Any suggestion what I can to move this forward?

I would like to get these changes merged so I can follow up with the improvements for riscv64 and other architectures.

Comment 164

[John Paul Adrian Glaubitz]

Ok, it has been accepted now. Let's see when my changes get merged :).

Comment 165

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - build: Add m68k as target architecture to mozbuild r=glandium

Adds the basic definitions for m68k to mozbuild, allowing to build Spidermonkey.

Comment 166

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - js:jit: Enable AtomicOperations-feeling-lucky.h on m68k r=lth

This allows the build on m68k to use the atomic operations provided by GCC.

Depends on D77285

Comment 167

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - mfbt: Reorder parameters for MOZ_ALIGNED_DECL r=jwalden

Currently, MOZ_ALIGNED_DECL uses the order (_type, _align) for its
parameters. However, this order makes the code less readable when
_type is a larger object like a struct because the value for _align
would be at the end of the struct definition. By swapping the order
of _type and _align, the alignment value will always be next to
the type name, regardless how far the definition of _type extends.

Depends on D77287

Comment 168

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - mfbt:tests: Handle targets with less strict alignment in TestCompactPair r=jesup

Previously, the tests assumed that the alignment of int and long equals
their size. This commit fixes the tests for targets like m68k that have
sizeof(int) == 4 and alignof(int) == 2. A static helper function sizemax
was introduced as the offset of the second element in Pair<int,long>
might be either determined by its alignment requirement or the size of
the preceding int element and we use the helper function to pick the
larger of the two values.

Depends on D77288

Comment 169

[John Paul Adrian Glaubitz]

Attachment: Bug 1325771 - mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea r=glandium

Define RETURN_INSTR for m68k in TestPoisonArea, i.e. the m68k assembly
opcodes for "rts ; rts".

Depends on D77289

Comment 170

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/393a6ff847ce
build: Add m68k as target architecture to mozbuild r=glandium
https://hg.mozilla.org/integration/autoland/rev/24e6299e112d
js:jit: Enable AtomicOperations-feeling-lucky.h on m68k r=lth
https://hg.mozilla.org/integration/autoland/rev/d69ac62c063f
mfbt: Reorder parameters for MOZ_ALIGNED_DECL r=jwalden
https://hg.mozilla.org/integration/autoland/rev/b3e0fb410a1c
mfbt:tests: Handle targets with less strict alignment in TestCompactPair r=jesup
https://hg.mozilla.org/integration/autoland/rev/ace40545b46c
mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea r=glandium

Comment 171

[Dorel Luca [:dluca]]

Backed out 5 changesets (bug 1325771) for Spidermonkey failures and build bustage in build/src/mfbt/tests/TestCompactPair.cpp. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304925017&repo=autoland&lineNumber=5167

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=ace40545b46c2c994e05a51ec466e3cb81a97d0c

Backout:
https://hg.mozilla.org/integration/autoland/rev/c593e06b6cf405c5056e20337f2c3acb9883f428

Comment 172

[John Paul Adrian Glaubitz]

Thanks, I fix that error. Looks like size_t just needs the std:: namespace specifier. Not sure why this slipped through.

Comment 173

[John Paul Adrian Glaubitz]

Fixed. Builds fine now.

But it looks like moz-phab reset the reviewed flags for all patches :(.

Comment 174

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47230cc3258d
build: Add m68k as target architecture to mozbuild r=glandium
https://hg.mozilla.org/integration/autoland/rev/1792304075b3
js:jit: Enable AtomicOperations-feeling-lucky.h on m68k r=lth
https://hg.mozilla.org/integration/autoland/rev/98d02ae1933f
mfbt: Reorder parameters for MOZ_ALIGNED_DECL r=jwalden

Comment 175

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/47230cc3258d
https://hg.mozilla.org/mozilla-central/rev/1792304075b3
https://hg.mozilla.org/mozilla-central/rev/98d02ae1933f

Comment 176

[John Paul Adrian Glaubitz]

Thanks for pushing my changes.

Unfortunately, two of the changes are missing:

https://phabricator.services.mozilla.com/D77290
https://phabricator.services.mozilla.com/D77289

Any idea why these are not in despite being set as accepted?

Comment 177

[John Paul Adrian Glaubitz]

D77289 has been accepted again. Can someone merged D77289 and D77290 now?

Do I need to reopen the bug report?

Comment 178

[Dorel Luca [:dluca]]

Hi,
Landed D77289 and D77290 onto autoland

Comment 179

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea81e4489c10
mfbt:tests: Handle targets with less strict alignment in TestCompactPair r=jesup
https://hg.mozilla.org/integration/autoland/rev/a47141066d20
mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea r=glandium

Comment 180

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ea81e4489c10
https://hg.mozilla.org/mozilla-central/rev/a47141066d20