Spidermonkey build support for Linux m68k broken
Categories
(Firefox Build System :: General: Unsupported Platforms, defect, P5)
Tracking
(firefox79 fixed)
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
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Comment 12•7 years ago
|
||
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).
Assignee | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
Fixed spelling in commit message.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 15•7 years ago
|
||
I just noticed, we're still lacking atomic operations for m68k (likewise alpha and hppa). Currently investigating on a possible patch.
Updated•7 years ago
|
Updated•7 years ago
|
Comment 16•7 years 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
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Comment 17•7 years 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•7 years ago
|
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years 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•7 years ago
|
||
Just confirmed: With all patches applied, 'make' and 'make install' are executed without any issues.
Updated•7 years ago
|
Assignee | ||
Comment 21•7 years ago
|
||
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.
Updated•7 years ago
|
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•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8825659 [details] Bug 1325771 - Add m68k as target architecture to mozbuild. https://reviewboard.mozilla.org/r/103750/#review104464
Comment 35•7 years 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
Comment 36•7 years 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
Comment 37•7 years ago
|
||
The double-conversion one was fixed upstream close to three years ago, apparently. Maybe we should consider an update?
Comment 38•7 years 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.
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8825665 [details] Bug 1325771 - ipc:chromium: Add platform defines for m68k. https://reviewboard.mozilla.org/r/103762/#review104478
Updated•7 years ago
|
Comment 40•7 years ago
|
||
(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•7 years 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•7 years 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.
Comment 43•7 years ago
|
||
(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•7 years 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.
Comment 45•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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 52•7 years ago
|
||
mozreview-review |
Comment on attachment 8825660 [details] Bug 1325771 - xpcom: Fix syntax error in PrepareAndDispatch on Linux/m68k. https://reviewboard.mozilla.org/r/103752/#review104540
![]() |
||
Comment 53•7 years ago
|
||
mozreview-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
Assignee | ||
Comment 54•7 years 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•7 years ago
|
||
mozreview-review |
Comment on attachment 8825664 [details] Bug 1325771 - media:webrtc: Add platform defines for m68k. https://reviewboard.mozilla.org/r/103760/#review104586
Assignee | ||
Comment 56•7 years 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•7 years 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•7 years ago
|
||
Updated to have the m68k-specific part below "#elif defined(JS_CODEGEN_NONE)" (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1325495#c11)
Assignee | ||
Comment 59•7 years ago
|
||
Updated according to: https://bugzilla.mozilla.org/show_bug.cgi?id=1325771#c51
Assignee | ||
Comment 60•7 years ago
|
||
New patch. Since shadow::Shape is not derived from Cell, it cannot inherit the alignment from that class unfortunately.
Comment 61•7 years ago
|
||
(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•7 years 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•7 years ago
|
Comment 63•7 years 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.
Comment 64•7 years 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?
Assignee | ||
Comment 65•7 years 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.
Comment 66•7 years 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 had misread the __attribute__ define thing, but I still think MOZ_ALIGNED_DECL would be better, albeit, after reordering its arguments.
Assignee | ||
Comment 67•7 years 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•7 years 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•7 years 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
Assignee | ||
Comment 70•7 years 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.
Comment 71•7 years ago
|
||
(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•7 years 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.
Comment 73•7 years 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.
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 74•7 years 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•7 years 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•7 years ago
|
||
Assignee | ||
Comment 77•7 years ago
|
||
Assignee | ||
Comment 78•7 years ago
|
||
Assignee | ||
Comment 79•7 years ago
|
||
Assignee | ||
Comment 80•7 years ago
|
||
Assignee | ||
Comment 81•7 years ago
|
||
Assignee | ||
Comment 82•7 years ago
|
||
Assignee | ||
Comment 83•7 years ago
|
||
Assignee | ||
Comment 84•7 years ago
|
||
Assignee | ||
Comment 85•7 years ago
|
||
Assignee | ||
Comment 86•7 years ago
|
||
Assignee | ||
Comment 87•7 years ago
|
||
Assignee | ||
Comment 88•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 89•7 years 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•7 years ago
|
![]() |
||
Comment 90•7 years ago
|
||
Please stop asking for review on patches that have already been reviewed and contain no changes.
Assignee | ||
Comment 91•7 years 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•7 years ago
|
![]() |
||
Updated•7 years ago
|
![]() |
||
Updated•7 years ago
|
Comment 92•7 years 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.
Updated•7 years ago
|
![]() |
||
Comment 93•7 years ago
|
||
(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•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 94•7 years 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•7 years ago
|
Comment 95•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee | ||
Comment 96•7 years 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 97•7 years 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]: ----------------------------------------------------------------- ...okay. I'm confused why I'm being asked to review again, but I can certainly just copy-paste my comments on it from last time. I'm not particularly satisfied with the forms either of those size expressions take. Partly because all these particulars are ABI-specific and apparently don't fully accord with intuition. And as a consequence, I can't make sense of exactly what/how the expressions correspond to the size as a consequence. I'd prefer, somehow, if we separated out the special-case from the intuitive case -- something like (sizeof(int) == 4 && alignof(int) == 2 && sizeof(long) == 4 && alignof(long) == 4) ? ... : 2 * sizeof(long) so that someone can work through the logic of how the size is computed. But I've been staring at this too long, so go with what you have if you can't somehow clean it up this way.
Comment 98•7 years ago
|
||
Comment on attachment 8831550 [details] [diff] [review] 0001-Bug-1325771-Add-m68k-as-target-architecture-to-mozbu.patch Review of attachment 8831550 [details] [diff] [review]: ----------------------------------------------------------------- This already had a r+
Comment 99•7 years ago
|
||
Comment on attachment 8831553 [details] [diff] [review] 0004-Bug-1325771-mfbt-tests-Define-RETURN_INSTR-for-m68k-.patch Review of attachment 8831553 [details] [diff] [review]: ----------------------------------------------------------------- This already had a r+
Comment 100•7 years ago
|
||
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+
Updated•7 years ago
|
Comment 101•7 years ago
|
||
Comment on attachment 8831562 [details] [diff] [review] 0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch Review of attachment 8831562 [details] [diff] [review]: ----------------------------------------------------------------- This should be patch 9.5 (after patch 9, before patch 10), or folded in patch 9. You should also add a few words in the commit message about why. It's better if people don't have to read the comments in this bug to figure that out in the future. This will also need to run through try because MSVC has some issues with __VA_ARGS__. I don't think they'd been hit with this change and patch 12, but better safe than sorry.
Comment 102•7 years ago
|
||
Comment on attachment 8831562 [details] [diff] [review] 0013-Bug-1325771-mfbt-Make-MOZ_ALIGNED_DECL-use-__VA_ARGS.patch Review of attachment 8831562 [details] [diff] [review]: ----------------------------------------------------------------- Um, actually, this is wrong. MOZ_ALIGNED_DECL is intended for use in declaring an aligned *variable*. It is not intended for use in declaring that a type should have particular alignment. Instead, you should use C++11 alignas. And additionally you very likely need to tag the type as MOZ_NON_PARAM and potentially fix any places where it's passed as an argument by value, to instead pass by reference or pointer.
Comment 103•7 years ago
|
||
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•7 years 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?
Comment 105•7 years ago
|
||
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•7 years 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.
Comment 107•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #106) > I consider this hostile behavior towards the community and downstream. I understand your frustration. As a developer it's nice to see our code run on all these different platforms, and I appreciate the heroic efforts from you and others to keep all these ports working. But please try to put yourself in our shoes for a minute. Rust offers us some game changing benefits (in terms of security, productivity, performance) that we really need in the very competitive browser world. We have been working towards this for years so it's also not something that should come as a huge surprise. Should we keep these improvements from 99.999% of our users because there exist some platforms without a Rust backend? What would you do?
Comment 108•7 years ago
|
||
I ask this as delicately as I can, but what precise action here do you consider hostile behavior? I truly don't understand My understanding from the last time I looked at Debian (it's been awhile, I'm more a Fedora person, myself -- but to each his own), is that it really really really wants to support a bunch of different architectures and hardware combinations. Mozilla and Firefox, on the other hand, don't attempt to function so broadly. We have a "tier" system into which platforms/architectures are categorized. Windows (32/64 Intel-compatible for now, conceivably ARM eventually), OS X (64 Intel, and maybe 32 but it's dying), and Linux (32/64 Intel) are tier 1. We do not break these; breakage is fixed or backed out in short order. Below tier 1 we have 2/3/etc. that we, to varying degrees, try not to break but make no guarantees about. We review patches for this functionality, and we try to stub out functionality in them so more-informed people can do complete fixes, and we'll land patches that are properly reviewed for non-tier 1 platforms, but it's not a strong focus. This is simply different fundamental ideas about compatibility. You/Debian have made a perfectly reasonable choice to try to support as many things as you can. We/Mozilla/Firefox have made a different, equally perfectly reasonable choice to focus our efforts on a few platforms, while lending what hand we can to those who want to work on more. It's just different priorities and philosophies. It doesn't seem like hostility to me. What am I missing?
Comment 109•7 years ago
|
||
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•7 years 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.
Comment 111•7 years ago
|
||
(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•7 years ago
|
||
Thanks for the pointer, very helpful!
Assignee | ||
Comment 113•7 years 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•7 years 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•7 years 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.
Comment 116•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #115) > > Are you also going to backport security fixes on the older version of > > Firefox you imply Debian should stick with? > > We have done this in the past and we have fought Mozilla in the past over > the trademark issue. Where "We" was me, and a) it was not sustainable (and it's even less so now), b) many security issues weren't fixed. > > BTW, Debian doesn't ship anything else than the ESR versions, and ESR52, > > which doesn't require Rust, is going to be supported until at least mid > > 2018. That leaves more than a year for Debian to make Rust work on all the > > architectures it cares about. Oh, and FYI, IIRC, the Debian security team > > prefers Firefox updates wth Rust, even if it means on x86 and x86-64 only, > > than the alternative. > > Debian is a very democratic project and no single entity gets to make such > decisions and you are supposed to know that. Debian is more a do-ocracy than a democracy. There is only one democratic process (GRs), and they are barely involved in package maintenance.
Comment 117•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #113) > yet you alienate contributors by letting them review and fix their patches > just to discard everything in the end. To be honest, I feel played by you > guys. Requiring Rust is not something that was decided last week, as you seem to suggest. People have been working towards this for years. I mentioned this 3 weeks ago (bug 1329194 comment 18) and there have been bugs + mailing list threads on this going back months if not years. Discussing this here is offtopic so this is my last comment.
![]() |
||
Comment 118•7 years ago
|
||
I went to go land these patches this morning (they have minimal impact, and getting them landed can at least start the process for them to be uplifted for ESR52), and saw that part 13 has been r-. John, can you please fix according to Jeff's review in comment 102?
Comment 119•7 years ago
|
||
(In reply to John Paul Adrian Glaubitz from comment #115) > (In reply to Mike Hommey [:glandium] from comment #109) > > With my Debian hat on, I'll add that it's Debian's problem if it doesn't > > have Rust packages on architectures other than x86, x86-64 and arm64 (and in > > fact, it doesn't even have cargo on arm64). > > No, it's not Debian's problem. It's Mozilla's problem because Rust's > codebase is far from being production grade that it actually works on > anything beyond x86. Don't blame us if your upstream project isn't ready for > production yet. Fedora doesn't agree with you. https://koji.fedoraproject.org/koji/buildinfo?buildID=829097
Comment 120•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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
Comment 125•7 years 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. 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•7 years 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•7 years ago
|
||
Hello! Is there anything I can do to get this resolved? Thanks, Adrian
Comment 128•7 years ago
|
||
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•7 years 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•7 years 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
Comment 131•7 years ago
|
||
Then do alignas(tl::Max<alignof(void*), 4>::value) instead.
Comment 132•7 years 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)
![]() |
||
Updated•6 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 133•5 years ago
|
||
FWIW, we're working on Rust for m68k now. Progress is slow, but we're getting there. I can already compile some files before the compiler bails out.
![]() |
||
Comment 134•5 years ago
|
||
(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.
Assignee | ||
Comment 135•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 136•4 years ago
|
||
Assignee | ||
Comment 137•4 years ago
|
||
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.
Assignee | ||
Comment 138•4 years ago
|
||
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.
Assignee | ||
Comment 139•4 years ago
|
||
Assignee | ||
Comment 140•4 years ago
|
||
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.
Assignee | ||
Comment 141•4 years ago
|
||
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.
Assignee | ||
Comment 142•4 years ago
|
||
Comment 143•4 years ago
|
||
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.
Assignee | ||
Comment 144•4 years ago
|
||
Assignee | ||
Comment 145•4 years ago
|
||
Ah, I didn't know there was a Phabricator instance now. Thanks for the heads-up!
Comment 146•4 years ago
|
||
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?
Assignee | ||
Comment 147•4 years ago
|
||
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.
Assignee | ||
Comment 148•4 years ago
|
||
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.
Assignee | ||
Comment 149•4 years ago
|
||
Assignee | ||
Comment 150•4 years ago
|
||
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.
Assignee | ||
Comment 151•4 years ago
|
||
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.
Assignee | ||
Comment 152•4 years ago
|
||
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.
Assignee | ||
Comment 153•4 years ago
|
||
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.
Assignee | ||
Comment 154•4 years ago
|
||
Define RETURN_INSTR for m68k in TestPoisonArea, i.e. the m68k assembly opcodes for "rts ; rts".
Assignee | ||
Comment 155•4 years ago
|
||
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.
Assignee | ||
Comment 156•4 years ago
|
||
This allows the build on m68k to use the atomic operations provided by GCC.
Assignee | ||
Comment 157•4 years ago
|
||
Adds the basic definitions for m68k to mozbuild, allowing to build Spidermonkey.
Assignee | ||
Comment 158•4 years ago
|
||
All added to Phabricator. Thanks!
Once this is merged, I'll follow up with the necessary changes for riscv64.
Assignee | ||
Comment 159•4 years ago
|
||
FWIW, we're currently also working on Rust support for m68k. So we even might be able to build the whole of Firefox at some point :).
Comment 160•4 years ago
|
||
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
Updated•4 years ago
|
Updated•4 years ago
|
Comment 161•4 years ago
|
||
(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. ;-)
Assignee | ||
Comment 162•4 years ago
|
||
(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
Assignee | ||
Comment 163•4 years ago
|
||
This is unfortunately still blocked by the last patch review by jwalden:
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.
Assignee | ||
Comment 164•4 years ago
|
||
Ok, it has been accepted now. Let's see when my changes get merged :).
Assignee | ||
Comment 165•4 years ago
|
||
Adds the basic definitions for m68k to mozbuild, allowing to build Spidermonkey.
Assignee | ||
Comment 166•4 years ago
|
||
This allows the build on m68k to use the atomic operations provided by GCC.
Depends on D77285
Assignee | ||
Comment 167•4 years ago
|
||
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
Assignee | ||
Comment 168•4 years ago
|
||
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
Assignee | ||
Comment 169•4 years ago
|
||
Define RETURN_INSTR for m68k in TestPoisonArea, i.e. the m68k assembly
opcodes for "rts ; rts".
Depends on D77289
Comment 170•4 years ago
|
||
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
Comment 171•4 years ago
|
||
Backed out 5 changesets (bug 1325771) for Spidermonkey failures and build bustage in build/src/mfbt/tests/TestCompactPair.cpp. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304925017&repo=autoland&lineNumber=5167
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=ace40545b46c2c994e05a51ec466e3cb81a97d0c
Backout:
https://hg.mozilla.org/integration/autoland/rev/c593e06b6cf405c5056e20337f2c3acb9883f428
Assignee | ||
Comment 172•4 years ago
|
||
Thanks, I fix that error. Looks like size_t just needs the std:: namespace specifier. Not sure why this slipped through.
Assignee | ||
Comment 173•4 years ago
|
||
Fixed. Builds fine now.
But it looks like moz-phab reset the reviewed flags for all patches :(.
Comment 174•4 years ago
|
||
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
Comment 175•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/47230cc3258d
https://hg.mozilla.org/mozilla-central/rev/1792304075b3
https://hg.mozilla.org/mozilla-central/rev/98d02ae1933f
Assignee | ||
Comment 176•4 years ago
|
||
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?
Assignee | ||
Comment 177•4 years ago
|
||
D77289 has been accepted again. Can someone merged D77289 and D77290 now?
Do I need to reopen the bug report?
Comment 179•4 years ago
|
||
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
Comment 180•4 years ago
|
||
bugherder |
Updated•3 years ago
|
Description
•