Status

()

Firefox
Build Config
11 months ago
5 months ago

People

(Reporter: John Paul Adrian Glaubitz, Assigned: John Paul Adrian Glaubitz, NeedInfo)

Tracking

51 Branch
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 31 obsolete attachments)

2.31 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.37 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.03 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
915 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
1008 bytes, patch
jesup
: review+
Details | Diff | Splinter Review
942 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
1.83 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
933 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
2.89 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.53 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.23 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.09 KB, patch
Waldo
: review-
Details | Diff | Splinter Review
(Assignee)

Description

11 months ago
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
(Assignee)

Updated

11 months ago
OS: Unspecified → Linux
Hardware: Unspecified → Other
(Assignee)

Comment 1

11 months ago
Created attachment 8821739 [details] [diff] [review]
0001-Bug-1325771-Add-m68k-as-target-architecture-to-mozbu.patch
(Assignee)

Comment 2

11 months ago
Created attachment 8821740 [details] [diff] [review]
0002-Bug-1325771-xpcom-Fix-syntax-error-in-PrepareAndDisp.patch
(Assignee)

Comment 3

11 months ago
Created attachment 8821741 [details] [diff] [review]
0003-Bug-1325771-xpcom-Fix-type-of-result-in-NS_InvokeByI.patch
(Assignee)

Comment 4

11 months ago
Created attachment 8821742 [details] [diff] [review]
0004-Bug-1325771-mfbt-tests-Define-RETURN_INSTR-for-m68k-.patch
(Assignee)

Comment 5

11 months ago
Created attachment 8821743 [details] [diff] [review]
0005-Bug-1325771-mfbt-double-conversion-Update-for-m68k-f.patch
(Assignee)

Comment 6

11 months ago
Created attachment 8821744 [details] [diff] [review]
0006-Bug-1325771-media-webrtc-Add-platform-defines-for-m6.patch
(Assignee)

Comment 7

11 months ago
Created attachment 8821746 [details] [diff] [review]
0007-Bug-1325771-ipc-chromium-Add-platform-defines-for-m6.patch
(Assignee)

Comment 8

11 months ago
Created attachment 8821747 [details] [diff] [review]
0008-Bug-1325771-js-jit-Make-sure-JitCode-has-at-least-4-.patch
(Assignee)

Comment 9

11 months ago
Created attachment 8821748 [details] [diff] [review]
0009-Bug-1325771-js-vm-Make-sure-AccessorShape-and-Shape-.patch
(Assignee)

Comment 10

11 months ago
Created attachment 8821749 [details] [diff] [review]
0010-Bug-1325771-layout-style-Make-sure-nsCSSValue-has-at.patch
(Assignee)

Comment 11

11 months ago
Created attachment 8821750 [details] [diff] [review]
0011-Bug-1325771-mfbt-tests-Account-for-alignment-when-te.patch

Updated

11 months ago
Component: Untriaged → Build Config
(Assignee)

Comment 12

11 months ago
Created attachment 8821782 [details] [diff] [review]
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).
Attachment #8821750 - Attachment is obsolete: true
(Assignee)

Comment 13

11 months ago
Created attachment 8821783 [details] [diff] [review]
0011-Bug-1325771-mfbt-tests-Handle-platforms-with-less-st.patch
Attachment #8821782 - Attachment is obsolete: true
(Assignee)

Comment 14

11 months ago
Created attachment 8821784 [details] [diff] [review]
0011-Bug-1325771-mfbt-tests-Handle-targets-with-less-stri.patch

Fixed spelling in commit message.
Attachment #8821783 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8821739 - Flags: review?(mh+mozilla)
Attachment #8821739 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821740 - Flags: review?(mh+mozilla)
Attachment #8821740 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821741 - Flags: review?(mh+mozilla)
Attachment #8821741 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821742 - Flags: review?(mh+mozilla)
Attachment #8821742 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821743 - Flags: review?(mh+mozilla)
Attachment #8821743 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821744 - Flags: review?(mh+mozilla)
Attachment #8821744 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821746 - Flags: review?(mh+mozilla)
Attachment #8821746 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821747 - Flags: review?(mh+mozilla)
Attachment #8821747 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821748 - Flags: review?(mh+mozilla)
Attachment #8821748 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821749 - Flags: review?(mh+mozilla)
Attachment #8821749 - Flags: review?(martin)
(Assignee)

Updated

11 months ago
Attachment #8821784 - Flags: review?(mh+mozilla)
Attachment #8821784 - Flags: review?(martin)
(Assignee)

Comment 15

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

Updated

11 months ago
Attachment #8821739 - Flags: review?(martin) → review+

Updated

11 months ago
Attachment #8821740 - Flags: review?(martin) → review+

Comment 16

11 months ago
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+

Updated

11 months ago
Attachment #8821742 - Flags: review?(martin) → review+

Updated

11 months ago
Attachment #8821743 - Flags: review?(martin) → review+

Updated

11 months ago
Attachment #8821744 - Flags: review?(martin) → review+

Updated

11 months ago
Attachment #8821746 - Flags: review?(martin) → review+

Comment 17

11 months ago
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?

Updated

11 months ago
Attachment #8821784 - Flags: review?(martin) → review+
(Assignee)

Comment 18

11 months ago
Created attachment 8822076 [details] [diff] [review]
0012-Bug-1325771-js-jit-Use-PowerPC-atomic-operations-on-.patch
Attachment #8822076 - Flags: review?(mh+mozilla)
Attachment #8822076 - Flags: review?(martin)
(Assignee)

Comment 19

11 months ago
(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.
(Assignee)

Comment 20

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

Updated

11 months ago
Attachment #8822076 - Flags: review?(martin) → review+
(Assignee)

Comment 21

11 months ago
Created attachment 8822649 [details] [diff] [review]
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.
Attachment #8822076 - Attachment is obsolete: true
Attachment #8822076 - Flags: review?(mh+mozilla)
Attachment #8822649 - Flags: review?(mh+mozilla)
Attachment #8822649 - Flags: review?(martin)

Updated

11 months ago
Attachment #8822649 - Flags: review?(martin) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

10 months ago
Attachment #8821739 - Attachment is obsolete: true
Attachment #8821739 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821740 - Attachment is obsolete: true
Attachment #8821740 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821741 - Attachment is obsolete: true
Attachment #8821741 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821742 - Attachment is obsolete: true
Attachment #8821742 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821743 - Attachment is obsolete: true
Attachment #8821743 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821744 - Attachment is obsolete: true
Attachment #8821744 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821746 - Attachment is obsolete: true
Attachment #8821746 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8821747 - Attachment is obsolete: true
Attachment #8821747 - Flags: review?(mh+mozilla)
Attachment #8821747 - Flags: review?(martin)

Updated

10 months ago
Attachment #8821748 - Attachment is obsolete: true
Attachment #8821748 - Flags: review?(mh+mozilla)
Attachment #8821748 - Flags: review?(martin)

Updated

10 months ago
Attachment #8821749 - Attachment is obsolete: true
Attachment #8821749 - Flags: review?(mh+mozilla)
Attachment #8821749 - Flags: review?(martin)

Updated

10 months ago
Attachment #8821784 - Attachment is obsolete: true
Attachment #8821784 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8822649 - Attachment is obsolete: true
Attachment #8822649 - Flags: review?(mh+mozilla)

Comment 34

10 months ago
mozreview-review
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 35

10 months ago
mozreview-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 36

10 months ago
mozreview-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 38

10 months ago
mozreview-review
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 39

10 months ago
mozreview-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+

Updated

10 months ago
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.
(Assignee)

Comment 41

10 months ago
(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 :).
(Assignee)

Comment 42

10 months ago
(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.
(Assignee)

Comment 44

10 months ago
(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?

Comment 46

10 months ago
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

10 months ago
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.
(Assignee)

Comment 48

10 months ago
(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);
(Assignee)

Comment 49

10 months ago
(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.
(Assignee)

Comment 50

10 months ago
(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

10 months ago
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+
(Assignee)

Comment 54

10 months ago
(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

10 months ago
mozreview-review
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+
(Assignee)

Comment 56

10 months ago
(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.
(Assignee)

Comment 57

10 months ago
(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.
(Assignee)

Comment 58

10 months ago
Created attachment 8826416 [details] [diff] [review]
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)
Attachment #8826416 - Flags: review?(jdemooij)
(Assignee)

Comment 59

10 months ago
Created attachment 8826419 [details] [diff] [review]
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
Attachment #8826419 - Flags: review?(jdemooij)
(Assignee)

Comment 60

10 months ago
Created attachment 8826420 [details] [diff] [review]
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.
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.
(Assignee)

Comment 62

10 months ago
(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.

Updated

10 months ago
Attachment #8826416 - Flags: review?(jdemooij) → review+

Comment 63

10 months ago
mozreview-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)

Comment 64

10 months ago
(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)
(Assignee)

Comment 65

10 months ago
(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.
(Assignee)

Comment 67

10 months ago
(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

10 months ago
mozreview-review
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

10 months ago
mozreview-review
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+
(Assignee)

Comment 70

10 months ago
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 72

10 months ago
mozreview-review
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 73

10 months ago
mozreview-review
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)

Updated

10 months ago
Attachment #8826419 - Flags: review?(jdemooij)

Updated

10 months ago
Attachment #8826420 - Flags: review?(jdemooij)
(Assignee)

Comment 74

10 months ago
(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.
(Assignee)

Comment 75

10 months ago
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.
(Assignee)

Comment 76

10 months ago
Created attachment 8831550 [details] [diff] [review]
0001-Bug-1325771-Add-m68k-as-target-architecture-to-mozbu.patch
Attachment #8831550 - Flags: review?(mh+mozilla)
(Assignee)

Comment 77

10 months ago
Created attachment 8831551 [details] [diff] [review]
0002-Bug-1325771-xpcom-Fix-syntax-error-in-PrepareAndDisp.patch
Attachment #8831551 - Flags: review?(nfroyd)
(Assignee)

Comment 78

10 months ago
Created attachment 8831552 [details] [diff] [review]
0003-Bug-1325771-xpcom-Fix-type-of-result-in-NS_InvokeByI.patch
Attachment #8831552 - Flags: review?(nfroyd)
(Assignee)

Comment 79

10 months ago
Created attachment 8831553 [details] [diff] [review]
0004-Bug-1325771-mfbt-tests-Define-RETURN_INSTR-for-m68k-.patch
Attachment #8831553 - Flags: review?(mh+mozilla)
(Assignee)

Comment 80

10 months ago
Created attachment 8831554 [details] [diff] [review]
0005-Bug-1325771-media-webrtc-Add-platform-defines-for-m6.patch
Attachment #8831554 - Flags: review?(rjesup)
(Assignee)

Comment 81

10 months ago
Created attachment 8831555 [details] [diff] [review]
0006-Bug-1325771-ipc-chromium-Add-platform-defines-for-m6.patch
Attachment #8831555 - Flags: review?(mh+mozilla)
(Assignee)

Comment 82

10 months ago
Created attachment 8831556 [details] [diff] [review]
0007-Bug-1325771-mfbt-tests-Handle-targets-with-less-stri.patch
Attachment #8831556 - Flags: review?(jdemooij)
(Assignee)

Comment 83

10 months ago
Created attachment 8831557 [details] [diff] [review]
0008-Bug-1325771-js-jit-Use-PowerPC-atomic-operations-on-.patch
Attachment #8831557 - Flags: review?(jdemooij)
(Assignee)

Comment 84

10 months ago
Created attachment 8831558 [details] [diff] [review]
0009-Bug-1325771-mfbt-Reorder-parameters-for-MOZ_ALIGNED_.patch
Attachment #8831558 - Flags: review?(mh+mozilla)
(Assignee)

Comment 85

10 months ago
Created attachment 8831559 [details] [diff] [review]
0010-Bug-1325771-js-src-Make-sure-Cell-has-at-least-4-byt.patch
Attachment #8831559 - Flags: review?(jdemooij)
(Assignee)

Comment 86

10 months ago
Created attachment 8831560 [details] [diff] [review]
0011-Bug-1325771-js-src-Make-sure-shadow-Shape-has-at-lea.patch
Attachment #8831560 - Flags: review?(jdemooij)
(Assignee)

Comment 87

10 months ago
Created attachment 8831561 [details] [diff] [review]
0012-Bug-1325771-layout-style-Make-sure-nsCSSValue-has-at.patch
Attachment #8831561 - Flags: review?(cam)
(Assignee)

Comment 88

10 months ago
Created attachment 8831562 [details] [diff] [review]
0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch
Attachment #8831562 - Flags: review?(mh+mozilla)
(Assignee)

Updated

10 months ago
Attachment #8825659 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825660 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8826420 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825661 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825662 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825663 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825664 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825665 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825666 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825667 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825668 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825669 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8825670 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8826416 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8826419 - Attachment is obsolete: true
(Assignee)

Comment 89

10 months ago
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.
(Assignee)

Updated

10 months ago
Flags: needinfo?(glaubitz)
Please stop asking for review on patches that have already been reviewed and contain no changes.
(Assignee)

Comment 91

10 months ago
(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.

Updated

10 months ago
Attachment #8831554 - Flags: review?(rjesup) → review+
Attachment #8831551 - Flags: review?(nfroyd) → review+
Attachment #8831552 - Flags: review?(nfroyd) → review+

Comment 92

10 months ago
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)

Updated

10 months ago
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.

Updated

10 months ago
Attachment #8831560 - Flags: review?(jdemooij) → review+

Updated

10 months ago
Attachment #8831559 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 94

10 months ago
(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 :).
(Assignee)

Updated

10 months ago
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+
(Assignee)

Comment 96

10 months ago
> 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+

Updated

10 months ago
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.
(Assignee)

Comment 104

10 months ago
(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.
(Assignee)

Comment 106

10 months ago
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.

Comment 110

10 months ago
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.

Comment 112

10 months ago
Thanks for the pointer, very helpful!
(Assignee)

Comment 113

10 months ago
(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.
(Assignee)

Comment 114

10 months ago
(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.
(Assignee)

Comment 115

10 months ago
(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.
Comment hidden (off-topic)
(Assignee)

Comment 122

10 months ago
(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.
(Assignee)

Comment 123

10 months ago
(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.
(Assignee)

Comment 124

9 months ago
(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.
(Assignee)

Comment 126

9 months ago
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
(Assignee)

Comment 127

9 months ago
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.)
(Assignee)

Comment 129

9 months ago
(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.
(Assignee)

Comment 130

8 months ago
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.

Comment 132

8 months ago
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)
You need to log in before you can comment on or make changes to this bug.