Full build of fx-team fails on MacOS X (Errors in DebugOnly.h)

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Unassigned)

Tracking

unspecified
Firefox 49
All
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
I can't build (full "back-end" build) fx-team on MacOS X currently (d55c8a642e3a). It works from a random older changeset (9426bf385146).

I'm using NDK r11b and have been running clobber/bootstrap.

 > 1:06.08 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:39:23: error: template declaration of 'mozilla::MOZ_STACK_CLASS mozilla::DebugOnly'
 > 1:06.08  class MOZ_STACK_CLASS DebugOnly
 > 1:06.08                        ^
 > 1:06.08 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:41:1: error: expected primary-expression before 'public'
 > 1:06.08  public:
 > 1:06.08  ^
 > 1:06.08 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:41:1: error: expected '}' before 'public'
 > 1:06.08 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:70:3: error: 'MOZ_IMPLICIT' does not name a type
 > 1:06.09    MOZ_IMPLICIT DebugOnly(const T&) { }
 > 1:06.09    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:71:19: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly(const DebugOnly&) { }
 > 1:06.09                    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:72:3: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly& operator=(const T&) { return *this; }
 > 1:06.09    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:73:22: error: 'void mozilla::operator++(int)' must have an argument of class or enumerated type
 > 1:06.09    void operator++(int) { }
 > 1:06.09                       ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:74:22: error: 'void mozilla::operator--(int)' must have an argument of class or enumerated type
 > 1:06.09    void operator--(int) { }
 > 1:06.09                       ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:75:3: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly& operator+=(const T&) { return *this; }
 > 1:06.09    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:76:3: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly& operator-=(const T&) { return *this; }
 > 1:06.09    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:77:3: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly& operator&=(const T&) { return *this; }
 > 1:06.09    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:78:3: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly& operator|=(const T&) { return *this; }
 > 1:06.09    ^
 > 1:06.09 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:79:3: error: 'DebugOnly' does not name a type
 > 1:06.09    DebugOnly& operator^=(const T&) { return *this; }
 > 1:06.09    ^
 > 1:06.10 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:87:13: error: expected class-name before '(' token
 > 1:06.10    ~DebugOnly() {}
 > 1:06.10              ^
 > 1:06.10 /Users/sebastian/Projects/Mozilla/fx-team/obj-arm-linux-androideabi/dist/include/mozilla/DebugOnly.h:90:1: error: expected declaration before '}' token
 > 1:06.10  } // namespace mozilla

More:
https://pastebin.mozilla.org/8872706
@ glandium: are these endian.h build problems related to libc++ changes around bug 1269251 and bug 1269171? I wasn't able to find a regression point, but I first hit the problem about a week ago.

The problem seems to a problem with the Mac's case-insensitive filesystem that causes some Android libc++ header files to accidentally include "mfbt/Endian.h" instead of <endian.h>.

I have a possible fix that just renames mfbt/Endian.h to EndianUtils.h to avoid the name conflict, but that is more of a bandaid over something that could be a bigger problem lurking.

https://hg.mozilla.org/try/rev/b5294dc2e5bca29a5dc5c789711ba75bbe8c57f4

But I still see one endian.h problem on Try, even with my EndianUtils.h patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7351e8eeb423b05372e83c97e8369d2f61787011

/home/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include/sys/endian.h:52:11: error: expected identifier or '(' before '__extension__'
/home/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include/sys/endian.h:53:11: error: expected identifier or '(' before '__extension__'
/home/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include/sys/endian.h:54:11: error: expected identifier or '(' before '__extension__'
/home/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include/sys/endian.h:55:11: error: expected identifier or '(' before '__extension__'
gmake[6]: *** [/home/worker/workspace/build/src/obj-firefox/security/nss/lib/util/errstrs.o] Error 1
Flags: needinfo?(mh+mozilla)
OS: Android → Mac OS X
Blocks: 1273934
So, the reason this is happening is because mfbt/Compression.cpp include mfbt/lz4.c, which includes <endian.h>, and in mfbt/, like everywhere else, we compile with -I$(srcdir), so that include does match Endian.h in the mfbt directory on a case-insensitive file system.

Now, the interesting thing is that this is true independently of bug 1273934, but there is a catch: the include of <endian.h> in lz4.c is behind a ifdef __GLIBC__.

And /this/ is kind of scary... we definitely should *not* have __GLIBC__ defined on Android cross-builds, and the timing would suggest that the switch to libc++ did that.

Nathan, would you mind taking a deeper look here? Because this sounds like a recipe for disaster.
Flags: needinfo?(mh+mozilla) → needinfo?(nfroyd)
09:31 <glandium> aaaaah it's more subtle than coming from lz4.c
09:32 <glandium> I was misled by it rooting to Compression.cpp
09:32 <glandium> libc++ has a header that includes endian.h
09:35 <glandium> froydnj: so, libcxx/include/__config checks for __LITTLE_ENDIAN__ and __BIG_ENDIAN__, but not for __BYTE_ORDER__, which modern compilers *do* set
09:36 <glandium> froydnj: an easy workaround would be to build with -D__LITTLE_ENDIAN__
09:36 <glandium> froydnj: -D__LITTLE_ENDIAN__=1, even
09:37 <glandium> on the longer term, libc++ should add a check for __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__/__ORDER_BIG_ENDIAN__
Turns out that more recent versions of libc++ do check for __BYTE_ORDER__.

The build error from comment 1 is really weird; the relevant section of the file looks like:

/* glibc compatibility. */
__BEGIN_DECLS
uint32_t htonl(uint32_t) __pure2;
uint16_t htons(uint16_t) __pure2;
uint32_t ntohl(uint32_t) __pure2;
uint16_t ntohs(uint16_t) __pure2;
__END_DECLS

and __pure2 is defined (<sys/cdefs.h>) as:

#define __pure2 __attribute__((__const__)) /* Android-added: used by FreeBSD libm */

So there's no __extension__ anywhere.
Flags: needinfo?(nfroyd)
I think taking Chris's patch to unbust things is reasonable, but we obviously have to figure out what's up with that build error.  I would love love love to know what the preprocessed source for the file GCC is complaining about looks like.  I guess I'll have to try a local Mac Android build to figure that out.
Duplicate of this bug: 1275382
(In reply to Chris Peterson [:cpeterson] from comment #1)
> @ glandium: are these endian.h build problems related to libc++ changes
> around bug 1269251 and bug 1269171? I wasn't able to find a regression
> point, but I first hit the problem about a week ago.
> 
> The problem seems to a problem with the Mac's case-insensitive filesystem
> that causes some Android libc++ header files to accidentally include
> "mfbt/Endian.h" instead of <endian.h>.
> 
> I have a possible fix that just renames mfbt/Endian.h to EndianUtils.h to
> avoid the name conflict, but that is more of a bandaid over something that
> could be a bigger problem lurking.
> 
> https://hg.mozilla.org/try/rev/b5294dc2e5bca29a5dc5c789711ba75bbe8c57f4
> 
> But I still see one endian.h problem on Try, even with my EndianUtils.h
> patch:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7351e8eeb423b05372e83c97e8369d2f61787011

That's a taskcluster tier-2 build, which isn't important...yet.  But given that we can't reproduce this problem on normal Linux builds or Mac builds...gbrown, how are we building this that might cause it to be different on taskcluster?  Any chance we could get access to a taskcluster instance with a failing build so we can poke around?
Flags: needinfo?(gbrown)
Comment on attachment 8755596 [details] [diff] [review]
EndianUtils.patch

r+ to this patch so Chris can land it if he wants and unblock Android developers on Macs.
Flags: needinfo?(cpeterson)
Attachment #8755596 - Flags: review+
(In reply to Nathan Froyd [:froydnj] from comment #8)
> Any chance we could get access to a taskcluster
> instance with a failing build so we can poke around?

That's easy - use the "One-click Loaner":

  - in treeherder, select the failing tc build;
  - in the Job Details pane, click on "Inspect Task";
  - in the Task Inspector, sign-in (link at top right);
  - in the Task Inspector, select the "Task" tab;
  - in the Task tab of Task Inspector, scroll down to the "Debug" section, and click "One click Loaner"


I'm looking now to see what might be different between the tc and buildbot builds, but not sure what I'm looking for yet...
(In reply to Nathan Froyd [:froydnj] from comment #9)
> Comment on attachment 8755596 [details] [diff] [review]
> EndianUtils.patch
> 
> r+ to this patch so Chris can land it if he wants and unblock Android
> developers on Macs.

I'd rather try the -D__LITTLE_ENDIAN__=1 trick.
(In reply to Geoff Brown [:gbrown] (pto May 28-June 13) from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #8)
> > Any chance we could get access to a taskcluster
> > instance with a failing build so we can poke around?
> 
> That's easy - use the "One-click Loaner":

This is *so* cool.

> I'm looking now to see what might be different between the tc and buildbot
> builds, but not sure what I'm looking for yet...

So the failing-to-build file is in NSS, and the command line looks like this:

/usr/bin/ccache /home/worker/workspace/build/src/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-gcc -std=gnu99 -o /home/worker/workspace/build/src/obj-firefox/security/nss/lib/util/errstrs.o -c -Os -gdwarf-2 -fPIC -DLINUX2_1 -idirafter /home/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include -mandroid -fno-short-enums -fno-exceptions -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -mno-unaligned-access -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pipe -g -freorder-blocks -fno-reorder-functions -Os -funwind-tables -DCHECK_FORK_GETPID  -include /home/worker/workspace/build/src/security/manager/android_stub.h -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Wall -DNSS_NO_GCC48 -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT -DNSS_DISABLE_DBM -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_ENABLE_TLS_1_3 -I/home/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/home/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/home/worker/workspace/build/src/obj-firefox/dist/include/nss -I/home/worker/workspace/build/src/obj-firefox/dist/private/nss -I/usr/include errstrs.c

That's a lot of junk, but the important part is at the end, where it says:

-I/usr/include

so that's include the *host's* /usr/include, rather than the (presumably) Android sysroot's /usr/include.  Looking at a successful build, we see for errstrs.c:

/usr/local/bin/python2.7 /builds/slave/try-and-api-15-000000000000000/build/src/sccache/sccache.py /builds/slave/try-and-api-15-000000000000000/build/src/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-gcc -std=gnu99 -o /builds/slave/try-and-api-15-000000000000000/build/src/obj-firefox/security/nss/lib/util/errstrs.o -c -Os -gdwarf-2 -fPIC -DLINUX2_1 -idirafter /builds/slave/try-and-api-15-000000000000000/build/src/android-ndk/platforms/android-9/arch-arm/usr/include -mandroid -fno-short-enums -fno-exceptions -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -mno-unaligned-access -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pipe -g -freorder-blocks -fno-reorder-functions -Os -funwind-tables -DCHECK_FORK_GETPID  -include /builds/slave/try-and-api-15-000000000000000/build/src/security/manager/android_stub.h -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Wall -DNSS_NO_GCC48 -DXP_UNIX -UDEBUG -DNDEBUG -D_REENTRANT -DNSS_DISABLE_DBM -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_ENABLE_TLS_1_3 -I/builds/slave/try-and-api-15-000000000000000/build/src/obj-firefox/dist/include/nspr -I/builds/slave/try-and-api-15-000000000000000/build/src/obj-firefox/dist/include/nspr -I/builds/slave/try-and-api-15-000000000000000/build/src/obj-firefox/dist/include/nss -I/builds/slave/try-and-api-15-000000000000000/build/src/obj-firefox/dist/private/nss  errstrs.c

Notice the lack of -I/usr/include before the errstrs.c argument.

It looks like -I/usr/include is getting pulled in through XCFLAGS, which is set to the value of X_CFLAGS, which is checking for the presence of X11 (!).  That would explain why it finds host libraries; it doesn't explain why taskcluster builds find X11 libraries when non-taskcluster builds don't (or even local developer builds, since I'm pretty sure I have the X headers installed so I can do Linux builds).

configure looks for libXt.{so,a,sl} in *lots* of different places and on the taskcluster builds it finds it in /usr/lib.  On my Linux system, libXt.so lives in /usr/lib/x86_64-linux-gnu, which is not on the list of configure's places to search (thanks to using an old configure, probably?).  That explains the taskcluster mismatch; the sysroot is setup slightly different on this cross image than the regular linux64 images, perhaps?  Assuming we could change how the taskcluster image is laid out, we still have a bit of a footgun here...glandium, WDYT?
Flags: needinfo?(mh+mozilla)
As per irc, it's an unintended consequence of bug 1271829. The goal there was to not have the second DEFAULT_GMAKE_FLAGS += XCFLAGS=... override the first in the same Makefile.in. The problem is that there appears to be a XCFLAGS variable set as an AC_SUBST at the top-level.

So the easiest thing to do would be to rename the XCFLAGS "local" variable bug 1271829 introduced in config/external/nss/Makefile.in.
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd [:froydnj] from comment #12)
> configure looks for libXt.{so,a,sl} in *lots* of different places and on the
> taskcluster builds it finds it in /usr/lib.  On my Linux system, libXt.so
> lives in /usr/lib/x86_64-linux-gnu, which is not on the list of configure's
> places to search (thanks to using an old configure, probably?).  That
> explains the taskcluster mismatch; the sysroot is setup slightly different
> on this cross image than the regular linux64 images, perhaps?

I would not be surprised if there are slight differences. As I understand it, we have tried to install the same known-to-be-important packages as on the regular linux64 images, but we haven't tried to make the taskcluster test images identical to the regular images.
Flags: needinfo?(gbrown)

Comment 15

2 years ago
FWIW I tried switching back to mozstlport as a temporary fix to allow me to do a native build, but it looks like Bug 1269319 breaks using mozstlport on android. Should I file a bug about that, or is that something we don't care about? (I'm completely unfamiliar with this area of code, and I have no idea about what the plans are - I'm guessing we don't really care, but I just wanted to be certain.)

I.e. I added "ac_add_options --with-android-cxx-stl=mozstlport" to my mozconfig, which causes the following build errors because of the remove of the copy constructors for Alignment2:

15:08.35 In file included from /Users/andrzejhunt/ff/fx3/build/stlport/stlport/stl/_vector.h:38:0,
15:08.35                  from /Users/andrzejhunt/ff/fx3/build/stlport/stlport/vector:37,
15:08.35                  from /Users/andrzejhunt/ff/fx3/objdir-frontend/dist/system_wrappers/vector:3,
15:08.36                  from /Users/andrzejhunt/ff/fx3/objdir-frontend/dist/stl_wrappers/vector:44,
15:08.36                  from /Users/andrzejhunt/ff/fx3/mozglue/build/BionicGlue.cpp:16:
15:08.37 /Users/andrzejhunt/ff/fx3/build/stlport/stlport/stl/_alloc.h: In instantiation of 'std::priv::_STLP_alloc_proxy<_Value, _Tp, _MaybeReboundAlloc>::_STLP_alloc_proxy(const _MaybeReboundAlloc&, _Value) [with _Value = AtForkFuncs*; _Tp = AtForkFuncs; _MaybeReboundAlloc = SpecialAllocator<AtForkFuncs>]':
15:08.37 /Users/andrzejhunt/ff/fx3/build/stlport/stlport/stl/_vector.h:67:58:   required from 'std::priv::_Vector_base<_Tp, _Alloc>::_Vector_base(const _Alloc&) [with _Tp = AtForkFuncs; _Alloc = SpecialAllocator<AtForkFuncs>]'
15:08.37 /Users/andrzejhunt/ff/fx3/build/stlport/stlport/stl/_vector.h:220:47:   required from 'std::vector<_Tp, _Alloc>::vector(const allocator_type&) [with _Tp = AtForkFuncs; _Alloc = SpecialAllocator<AtForkFuncs>; std::vector<_Tp, _Alloc>::allocator_type = SpecialAllocator<AtForkFuncs>]'
15:08.38 /Users/andrzejhunt/ff/fx3/mozglue/build/BionicGlue.cpp:64:65:   required from here
15:08.38 /Users/andrzejhunt/ff/fx3/build/stlport/stlport/stl/_alloc.h:481:41: error: use of deleted function 'SpecialAllocator<AtForkFuncs>::SpecialAllocator(const SpecialAllocator<AtForkFuncs>&)'
15:08.38      _MaybeReboundAlloc(__a), _M_data(__p) {}
15:08.38                                          ^
15:08.38 /Users/andrzejhunt/ff/fx3/mozglue/build/BionicGlue.cpp:35:8: note: 'SpecialAllocator<AtForkFuncs>::SpecialAllocator(const SpecialAllocator<AtForkFuncs>&)' is implicitly deleted because the default definition would be ill-formed:
15:08.38  struct SpecialAllocator: public std::allocator<T>
15:08.38         ^
15:08.38 /Users/andrzejhunt/ff/fx3/mozglue/build/BionicGlue.cpp:35:8: error: use of deleted function 'mozilla::AlignedStorage2<T>::AlignedStorage2(const mozilla::AlignedStorage2<T>&) [with T = AtForkFuncs]'
15:08.38 In file included from /Users/andrzejhunt/ff/fx3/mozglue/build/BionicGlue.cpp:14:0:
15:08.38 /Users/andrzejhunt/ff/fx3/objdir-frontend/dist/include/mozilla/Alignment.h:148:3: note: declared here
15:08.38    AlignedStorage2(const AlignedStorage2&) = delete;
15:08.39    ^
15:08.39 
15:08.39 In the directory  /Users/andrzejhunt/ff/fx3/objdir-frontend/mozglue/build
15:08.39 The following command failed to execute properly:
15:08.39 /Users/andrzejhunt/opt/android-ndk-r11c/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-g++ -std=gnu++11 -o BionicGlue.o -c -I/Users/andrzejhunt/ff/fx3/objdir-frontend/dist/stl_wrappers -I/Users/andrzejhunt/ff/fx3/objdir-frontend/dist/system_wrappers -include /Users/andrzejhunt/ff/fx3/config/gcc_hidden.h -DDEBUG=1 -DTRACING=1 -DIMPL_MFBT -I/Users/andrzejhunt/ff/fx3/mozglue/build -I/Users/andrzejhunt/ff/fx3/objdir-frontend/mozglue/build -I/Users/andrzejhunt/ff/fx3/objdir-frontend/dist/include -I/Users/andrzejhunt/ff/fx3/objdir-frontend/dist/include/nspr -I/Users/andrzejhunt/ff/fx3/objdir-frontend/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /Users/andrzejhunt/ff/fx3/objdir-frontend/mozilla-config.h -MD -MP -MF .deps/BionicGlue.o.pp -idirafter /Users/andrzejhunt/opt/android-ndk-r11c/platforms/android-9/arch-arm/usr/include -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wthread-safety -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -mandroid -fno-short-enums -fno-exceptions -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -mno-unaligned-access -isystem /Users/andrzejhunt/ff/fx3/build/stlport/stlport -isystem /Users/andrzejhunt/ff/fx3/build/stlport/overrides -isystem /Users/andrzejhunt/opt/android-ndk-r11c/sources/cxx-stl/system/include -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pipe -g -funwind-tables /Users/andrzejhunt/ff/fx3/mozglue/build/BionicGlue.cpp
15:08.44 make[5]: *** [BionicGlue.o] Error 1
Based on glandium's comment 11, I will hold off on landing the s/Endian.h/EndianUtils.h/ patch. It only papers over the real problem, even if shadowing a stdlib header filename (endian.h) was a bad idea.
Flags: needinfo?(cpeterson)

Updated

2 years ago
Blocks: 1270544
Depends on: 1276069
No longer depends on: 1276069
Duplicate of this bug: 1276069
I'm addressing comment 12 and subsequent in bug 1276069
I'm addressing comment 12 and subsequent in bug 1276069(In reply to Chris Peterson [:cpeterson] from comment #16)
> Based on glandium's comment 11, I will hold off on landing the
> s/Endian.h/EndianUtils.h/ patch. It only papers over the real problem, even
> if shadowing a stdlib header filename (endian.h) was a bad idea.

Actually, all things considered, with bug 1276069 fixed, this is not worse than defining __LITTLE_ENDIAN__ forcefully everywhere.
No longer blocks: 1270544
Comment on attachment 8755596 [details] [diff] [review]
EndianUtils.patch

(In reply to Mike Hommey [:glandium] from comment #19)
> > Based on glandium's comment 11, I will hold off on landing the
> > s/Endian.h/EndianUtils.h/ patch. It only papers over the real problem, even
> > if shadowing a stdlib header filename (endian.h) was a bad idea.
> 
> Actually, all things considered, with bug 1276069 fixed, this is not worse
> than defining __LITTLE_ENDIAN__ forcefully everywhere.

Mike, are you recommending that I land this EndianUtils.h patch instead of Nathan defining __LITTLE_ENDIAN__ globally?
Attachment #8755596 - Flags: feedback?(mh+mozilla)
Yes. Keeping Endian.h is /also/ calling for future problems, so we might as well get rid of it.
Just my opinion, but if we are trying to override a system routing with our own version, either calling it Endian or EndianUtils to override endian would seem to be not the best idea.  I would think we should call it something like moz_endian.h.
Attachment #8755596 - Flags: feedback?(mh+mozilla)

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86cda9d3eaa2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(In reply to Bill Gianopoulos [:WG9s] from comment #23)
> Just my opinion, but if we are trying to override a system routing with our
> own version, either calling it Endian or EndianUtils to override endian
> would seem to be not the best idea.  I would think we should call it
> something like moz_endian.h.

MFBT's EndianUtils.h contains C++ utility functions that are not a replacement for the macros in the standard endian.h.
You need to log in before you can comment on or make changes to this bug.