Closed Bug 1325771 Opened 7 years ago Closed 4 years ago

Spidermonkey build support for Linux m68k broken

Categories

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

51 Branch
Other
Linux
defect

Tracking

(firefox79 fixed)

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: glaubitz, Assigned: glaubitz)

References

Details

Attachments

(15 files, 44 obsolete files)

1004 bytes, patch
lth
: review+
Details | Diff | Splinter Review
2.03 KB, patch
Details | Diff | Splinter Review
3.42 KB, patch
Details | Diff | Splinter Review
777 bytes, patch
Details | Diff | Splinter Review
1.66 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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
OS: Unspecified → Linux
Hardware: Unspecified → Other
Component: Untriaged → Build Config
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).
Attachment #8821750 - Attachment is obsolete: true
Attachment #8821782 - Attachment is obsolete: true
Fixed spelling in commit message.
Attachment #8821783 - Attachment is obsolete: true
Attachment #8821739 - Flags: review?(mh+mozilla)
Attachment #8821739 - Flags: review?(martin)
Attachment #8821740 - Flags: review?(mh+mozilla)
Attachment #8821740 - Flags: review?(martin)
Attachment #8821741 - Flags: review?(mh+mozilla)
Attachment #8821741 - Flags: review?(martin)
Attachment #8821742 - Flags: review?(mh+mozilla)
Attachment #8821742 - Flags: review?(martin)
Attachment #8821743 - Flags: review?(mh+mozilla)
Attachment #8821743 - Flags: review?(martin)
Attachment #8821744 - Flags: review?(mh+mozilla)
Attachment #8821744 - Flags: review?(martin)
Attachment #8821746 - Flags: review?(mh+mozilla)
Attachment #8821746 - Flags: review?(martin)
Attachment #8821747 - Flags: review?(mh+mozilla)
Attachment #8821747 - Flags: review?(martin)
Attachment #8821748 - Flags: review?(mh+mozilla)
Attachment #8821748 - Flags: review?(martin)
Attachment #8821749 - Flags: review?(mh+mozilla)
Attachment #8821749 - Flags: review?(martin)
Attachment #8821784 - Flags: review?(mh+mozilla)
Attachment #8821784 - Flags: review?(martin)
I just noticed, we're still lacking atomic operations for m68k (likewise alpha and hppa). Currently investigating on a possible patch.
Attachment #8821739 - Flags: review?(martin) → review+
Attachment #8821740 - Flags: review?(martin) → review+
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
Attachment #8821741 - Flags: review?(martin) → review+
Attachment #8821742 - Flags: review?(martin) → review+
Attachment #8821743 - Flags: review?(martin) → review+
Attachment #8821744 - Flags: review?(martin) → review+
Attachment #8821746 - Flags: review?(martin) → review+
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?
Attachment #8821784 - Flags: review?(martin) → review+
Attachment #8822076 - Flags: review?(mh+mozilla)
Attachment #8822076 - Flags: review?(martin)
(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.
Just confirmed: With all patches applied, 'make' and 'make install' are executed without any issues.
Attachment #8822076 - Flags: review?(martin) → review+
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.
Attachment #8822076 - Attachment is obsolete: true
Attachment #8822076 - Flags: review?(mh+mozilla)
Attachment #8822649 - Flags: review?(mh+mozilla)
Attachment #8822649 - Flags: review?(martin)
Attachment #8822649 - Flags: review?(martin) → review+
Attachment #8821739 - Attachment is obsolete: true
Attachment #8821739 - Flags: review?(mh+mozilla)
Attachment #8821740 - Attachment is obsolete: true
Attachment #8821740 - Flags: review?(mh+mozilla)
Attachment #8821741 - Attachment is obsolete: true
Attachment #8821741 - Flags: review?(mh+mozilla)
Attachment #8821742 - Attachment is obsolete: true
Attachment #8821742 - Flags: review?(mh+mozilla)
Attachment #8821743 - Attachment is obsolete: true
Attachment #8821743 - Flags: review?(mh+mozilla)
Attachment #8821744 - Attachment is obsolete: true
Attachment #8821744 - Flags: review?(mh+mozilla)
Attachment #8821746 - Attachment is obsolete: true
Attachment #8821746 - Flags: review?(mh+mozilla)
Attachment #8821747 - Attachment is obsolete: true
Attachment #8821747 - Flags: review?(mh+mozilla)
Attachment #8821747 - Flags: review?(martin)
Attachment #8821748 - Attachment is obsolete: true
Attachment #8821748 - Flags: review?(mh+mozilla)
Attachment #8821748 - Flags: review?(martin)
Attachment #8821749 - Attachment is obsolete: true
Attachment #8821749 - Flags: review?(mh+mozilla)
Attachment #8821749 - Flags: review?(martin)
Attachment #8821784 - Attachment is obsolete: true
Attachment #8821784 - Flags: review?(mh+mozilla)
Attachment #8822649 - Attachment is obsolete: true
Attachment #8822649 - Flags: review?(mh+mozilla)
Comment on attachment 8825659 [details]
Bug 1325771 - Add m68k as target architecture to mozbuild.

https://reviewboard.mozilla.org/r/103750/#review104464
Attachment #8825659 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8825662 [details]
Bug 1325771 - mfbt:tests: Define RETURN_INSTR for m68k in TestPoisonArea.

https://reviewboard.mozilla.org/r/103756/#review104468
Attachment #8825662 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8825663 [details]
Bug 1325771 - mfbt:double-conversion: Update for m68k from upstream.

https://reviewboard.mozilla.org/r/103758/#review104474
Attachment #8825663 - Flags: review?(mh+mozilla) → review+
The double-conversion one was fixed upstream close to three years ago, apparently. Maybe we should consider an update?
Flags: needinfo?(jwalden+bmo)
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.
Attachment #8825668 - Flags: review?(cam) → review+
Comment on attachment 8825665 [details]
Bug 1325771 - ipc:chromium: Add platform defines for m68k.

https://reviewboard.mozilla.org/r/103762/#review104478
Attachment #8825665 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → glaubitz
(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.
(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 :).
(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.
(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.
(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.
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?
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...
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.
(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);
(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.
(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.
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 on attachment 8825660 [details]
Bug 1325771 - xpcom: Fix syntax error in PrepareAndDispatch on Linux/m68k.

https://reviewboard.mozilla.org/r/103752/#review104540
Attachment #8825660 - Flags: review?(nfroyd) → review+
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
Attachment #8825661 - Flags: review?(nfroyd) → review+
(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 on attachment 8825664 [details]
Bug 1325771 - media:webrtc: Add platform defines for m68k.

https://reviewboard.mozilla.org/r/103760/#review104586
Attachment #8825664 - Flags: review?(rjesup) → review+
(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.
(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.
Updated to have the m68k-specific part below "#elif defined(JS_CODEGEN_NONE)" (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1325495#c11)
Attachment #8826416 - Flags: review?(jdemooij)
New patch. Since shadow::Shape is not derived from Cell, it cannot inherit the alignment from that class unfortunately.
Attachment #8826420 - Flags: review?(jdemooij)
(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.
(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.
Attachment #8826416 - Flags: review?(jdemooij) → review+
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.
Attachment #8825670 - Flags: review?(jdemooij)
(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?
Flags: needinfo?(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.
(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.
(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 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 on attachment 8825669 [details]
Bug 1325771 - mfbt:tests: Handle targets with less strict alignment in TestPair.

https://reviewboard.mozilla.org/r/103770/#review107238
Attachment #8825669 - Flags: review?(jwalden+bmo) → review+
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.
Depends on: 1332797
(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 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.
Attachment #8825666 - Flags: review?(jdemooij)
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.
Attachment #8825667 - Flags: review?(jdemooij)
Attachment #8826419 - Flags: review?(jdemooij)
Attachment #8826420 - Flags: review?(jdemooij)
(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.
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.
Attachment #8825659 - Attachment is obsolete: true
Attachment #8825660 - Attachment is obsolete: true
Attachment #8826420 - Attachment is obsolete: true
Attachment #8825661 - Attachment is obsolete: true
Attachment #8825662 - Attachment is obsolete: true
Attachment #8825663 - Attachment is obsolete: true
Attachment #8825664 - Attachment is obsolete: true
Attachment #8825665 - Attachment is obsolete: true
Attachment #8825666 - Attachment is obsolete: true
Attachment #8825667 - Attachment is obsolete: true
Attachment #8825668 - Attachment is obsolete: true
Attachment #8825669 - Attachment is obsolete: true
Attachment #8825670 - Attachment is obsolete: true
Attachment #8826416 - Attachment is obsolete: true
Attachment #8826419 - Attachment is obsolete: true
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.
Flags: needinfo?(glaubitz)
Please stop asking for review on patches that have already been reviewed and contain no changes.
(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.
Attachment #8831554 - Flags: review?(rjesup) → review+
Attachment #8831551 - Flags: review?(nfroyd) → review+
Attachment #8831552 - Flags: review?(nfroyd) → review+
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.
Attachment #8831556 - Flags: review?(jdemooij)
Attachment #8831557 - Flags: review?(jdemooij) → review+
(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.
Attachment #8831560 - Flags: review?(jdemooij) → review+
Attachment #8831559 - Flags: review?(jdemooij) → review+
(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 :).
Attachment #8831556 - Flags: review?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
Attachment #8831561 - Flags: review?(cam) → review+
> 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 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.
Attachment #8831556 - Flags: review?(jwalden+bmo) → review+
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+
Attachment #8831550 - Flags: review?(mh+mozilla) → review+
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+
Attachment #8831553 - Flags: review?(mh+mozilla) → review+
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+
Attachment #8831555 - Flags: review?(mh+mozilla) → review+
Attachment #8831558 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8831562 - Flags: review?(mh+mozilla)
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.
Attachment #8831562 - Flags: review-
BTW, considering rust is now mandatory, until there is a rust compiler backend for m68k, this bug is not going to do anything useful.
(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?
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.
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.
(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?
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?
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.
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.
(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.
Thanks for the pointer, very helpful!
(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.
(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.
(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.
(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.
(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.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(glaubitz)
(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
(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.
(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.
(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.
(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
> 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.
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
Hello!

Is there anything I can do to get this resolved?

Thanks,
Adrian
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.)
(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.
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
Then do

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

instead.
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)
Flags: needinfo?(glaubitz)
Component: Build Config → General: Unsupported Platforms
Product: Firefox → Firefox Build System
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.
(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.
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.
Priority: -- → P5
Summary: Build support for Linux m68k broken → Spidermonkey build support for Linux m68k broken
Attachment #8831557 - Attachment is obsolete: true
Attachment #9146097 - Flags: review?(jdemooij)
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.
Attachment #8831551 - Attachment is obsolete: true
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.
Attachment #8831552 - Attachment is obsolete: true
Attachment #8831550 - Attachment is obsolete: true
Attachment #9146100 - Flags: review?(mh+mozilla)
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.
Attachment #8831562 - Attachment is obsolete: true
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.
Attachment #8831561 - Attachment is obsolete: true
Attachment #8831558 - Attachment is obsolete: true
Attachment #9146104 - Flags: review?(mh+mozilla)
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.
Attachment #9146097 - Flags: review?(jdemooij) → review?(lhansen)

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

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?
Attachment #9146097 - Flags: review?(lhansen) → review+
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.
Attachment #8831560 - Attachment is obsolete: true
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.
Attachment #8831559 - Attachment is obsolete: true
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.
Attachment #8831555 - Attachment is obsolete: true
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 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.
Attachment #8831554 - Attachment is obsolete: true

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.

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

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.

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

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

All added to Phabricator. Thanks!

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

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 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
Attachment #9146100 - Flags: review?(mh+mozilla)
Attachment #9146107 - Flags: review?(mh+mozilla)
Attachment #9146104 - Flags: review?(mh+mozilla)

(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. ;-)

(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

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.

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

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

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

Depends on D77285

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

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

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

Depends on D77289

Pushed by dluca@mozilla.com:
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

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

Flags: needinfo?(glaubitz)

Fixed. Builds fine now.

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

Pushed by ccoroiu@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

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?

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

Do I need to reopen the bug report?

Flags: needinfo?(dluca)

Hi,
Landed D77289 and D77290 onto autoland

Flags: needinfo?(dluca)
Pushed by dluca@mozilla.com:
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
Attachment #9146129 - Flags: review?(rjesup)
You need to log in before you can comment on or make changes to this bug.