570134 - Building with Apples llvm-gcc-4.2 fails on IPC

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nomis101]

After Bug 492843 was fixed, it was possible to build with Apples llvm-gcc-4.2. I've tried it today and now it fails at ipc. I've build this on Mac OS X 10.6 with gcc version 4.2.1 (Based on Apple Inc. build 5646) (LLVM build 2207.5)

/Volumes/Developer/temp/src/mozilla/ipc/chromium/src/chrome/common/ipc_channel_posix.cc: In member function ‘bool IPC::Channel::ChannelImpl::ProcessOutgoingMessages()’:
/Volumes/Developer/temp/src/mozilla/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:598: error: size of array ‘buf’ is not an integral constant-expression
message_pump_mac.mm
llvm-g++-4.2 -o message_pump_mac.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Darwin10.3.0\" -DOSARCH=Darwin -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD  -DOS_MACOSX=1 -DOS_POSIX=1  -DHAVE_CONFIG_H -I/Volumes/Developer/temp/src/mozilla/ipc/chromium/src -I/Volumes/Developer/temp/src/mozilla/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/Volumes/Developer/temp/src/mozilla/ipc/chromium/src/third_party/libevent -I/Volumes/Developer/temp/src/mozilla/ipc/chromium/src/third_party/libevent/mac -I/Volumes/Developer/temp/src/mozilla/ipc/chromium -I. -I../../dist/include -I../../dist/include/nsprpub  -I/Volumes/Developer/temp/src/mozilla/obj-x86_64-apple-darwin10.3.0/dist/include/nspr -I/Volumes/Developer/temp/src/mozilla/obj-x86_64-apple-darwin10.3.0/dist/include/nss       -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-strict-aliasing -fpascal-strings -fno-common -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -DNO_X11 -DMOZILLA_CLIENT -include ../../mozilla-config.h -Wp,-MD,.deps/message_pump_mac.pp -fobjc-exceptions /Volumes/Developer/temp/src/mozilla/ipc/chromium/src/base/message_pump_mac.mm
make[5]: *** [ipc_channel_posix.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_platform] Error 2
make[2]: *** [tier_platform] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

(For i386 builds, we still need a fix for Bug 508285)

Comment 1

[Benjamin Smedberg]

Looks like a compiler bug to me, that should be a compile-time-constant expression. It might help if you extracted the relevant preprocessed source so that CMSG_SPACE is expanded.

Comment 2

[Nomis101]

Attachment: preprocessed ipc_channel_posix.ii

OK, if I build with -save-temps, than I get a ipc_channel_posix.ii, which is the preprocessed source (and a empty ipc_channel_posix.s assembly file). I've zipped and attached the preprocessed source.

Thunderbird doesn't use IPC, right? Because I can build Thunderbird with llvm, but I can't build Firefox.

Comment 3

[Benjamin Smedberg]

The relevant line is:

char buf[(((__darwin_size_t)((char *)(__darwin_size_t)(sizeof(struct cmsghdr)) + (sizeof(__uint32_t) - 1)) &~ (sizeof(__uint32_t) - 1)) + ((__darwin_size_t)((char *)(__darwin_size_t)(sizeof(int[FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE])) + (sizeof(__uint32_t) - 1)) &~ (sizeof(__uint32_t) - 1)))];

This is a compile-time constant, and so this is a bug in the llvm-gcc compiler. I'm going to close this as INVALID since it's not our bug, although if you can come up with an alternate version that compiles I'd probably take the patch.

Comment 4

[Nomis101]

It is reported that Xcode 4 will include a brand new version of the llvm compiler:
>Xcode 4 is also reported to include a significant new version of the modern Low Level Virtual Machine (LLVM) Compiler
http://www.appleinsider.com/articles/10/06/19/inside_apples_new_xcode_4_development_tool.html&page=3

So I will wait until Xcode 4 is official out and try it again with this new version. If it than works, than its OK. If it also doesn't work, than I report this bug to Apple and post the Bug number here.

Comment 5

[Benjamin Smedberg]

Maybe you can report it to clang/apple *before* the compiler is released, so that they know about it and have a chance of fixing it?

Comment 6

[Nomis101]

OK, I've reported this to Apple, its now Bug ID# 8114270.

Comment 7

[Adam Michel]

Attachment: Patches ipc_channel_posix.cc to fix VLA issue

I'm not sure this is a compiler bug.  It looks like llvm-gcc is just being overly pedantic about the char array being variable length.  Technically C++ doesn't support variable-length arrays by the spec, I believe.  Either way, if you use new to allocate the char array, it compiles.  I've attached a patch for ipc_channel_posix.cc that builds on my system.  I don't think this has any broader implications, but someone more clever than I can determine that hopefully.

Comment 8

[Adam Michel]

Attachment: Added a delete buf to the scope

This is essentially the same patch with an added "delete buf;" at the end of the while loop.  A friend pointed out it wasn't being freed otherwise and I didn't see that it was either.  Basically just for completeness.  Still compiles.

Comment 9

[Nomis101]

Ah, thanks for this patch. :-) If I could, than I would test it. But the chagrin with llvm-gcc is, it isn't officially supported and as soon as one thing is fixed another thing gets broken. At the moment I'm stuck with Bug 597836.

Comment 10

[Nomis101]

Today I was able to show, that with the newest version of Apples llvm-gcc-4.2 (LLVM build 2333.4), it is still not possible to build. But with this patch I was able to build FF with Apples llvm-gcc-4.2.
So please, can we have this patch? This patch makes FF4 compatible with Apples llvm-gcc-4.2.

Comment 11

[Benjamin Smedberg]

No.

Comment 12

[Adam Michel]

I was under the impression from one of your previous comments that you'd take a patch if it compiled.  This is a patch that compiles.  Is there something further I can do to make this patch acceptable?  It's basically a no-op, I don't see what the issue is.

Comment 13

[Benjamin Smedberg]

This patch causes an allocation to occur in a relatively hot path. That's really not ok.

Comment 14

[Adam Michel]

Attachment: Uses a tmp var which seems to dodge the compiler bug

I don't think this adds any overhead like the last fix. I don't know why it compiles, but it must be a compiler bug as indicated in previous comments, and this dodges it.

Comment 15

[Adam Michel]

Any thoughts on this patch? Does it resolve the issue without the performance concern?

Comment 16

[Benjamin Smedberg]

Comment on attachment 479504 [details] [diff] [review]
Uses a tmp var which seems to dodge the compiler bug

Yeah, that's fine. Please attach a new patch with the hard tabs replaced with spaces and aligned correctly.

Comment 17

[Adam Michel]

Attachment: Properly formatted version of previous patch

Fixed formatting.

Comment 18

[Adam Michel]

I think I'm done here, but it's been a bit since I submitted the cleaned up patch, thought I'd check in. Is there something else I should do? I wasn't sure if it was my place to mark it resolved.

Comment 19

[Benjamin Smedberg]

Do you have checkin privileges? I set checkin-needed so that the sheriff or another volunteer will actually push the patch to our tree.

Comment 20

[Adam Michel]

Nope, no check-in privileges here. Thanks!

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/88f837224a1d