Tim Armstrong has posted comments on this change. Change subject: Ported native-toolchain to work on ppc64le ......................................................................
Patch Set 3: (13 comments) Sorry for the slow turnaround - was on holiday for a couple of weeks. I think we're converging here to something that will be more maintainable. http://gerrit.cloudera.org:8080/#/c/6468/3/buildall.sh File buildall.sh: Line 106: CRCUTIL_VERSION=440ba7babeff77ffad992df3a10c767f184e946e-p2\ The -p2 patch should work for x86-64 too, right? Let's just bump the version for both platforms and move the old version under "BUILD_HISTORICAL" like some of the examples above. Line 262: elif [[ "$ARCH_NAME" == "ppc64le" ]]; then The control flow here changed for x86-64 - now if BUILD_HISTORICAL is true, we don't build -p2. Line 263: BREAKPAD_VERSION=88e5b2c8806bac3f2c80d2fe80094be5bd371601-p3 $SOURCE_DIR/source/breakpad/build.sh Same as above - let's just bump the version for both platforms. http://gerrit.cloudera.org:8080/#/c/6468/3/functions.sh File functions.sh: PS3, Line 305: [ This logic is pretty weird - the patches will be numbered differently on different architectures. Actually is this even needed any more - can we just remove it? http://gerrit.cloudera.org:8080/#/c/6468/3/source/breakpad/breakpad-88e5b2c8806bac3f2c80d2fe80094be5bd371601-patches/0003-Build-breakpad_88e5b2c-on-ppc64le.patch File source/breakpad/breakpad-88e5b2c8806bac3f2c80d2fe80094be5bd371601-patches/0003-Build-breakpad_88e5b2c-on-ppc64le.patch: I really encourage you to get this patch into upstream breakpad. The problem is that if we rebase onto a new upstream breakpad, and your changes no longer work or don't cleanly apply, we don't have the knowledge or time to fix it so might have to discard the patch until you can look at it. I didn't validate the PPC implementation but I did a pass to make sure that all the PPC stuff is included in the appropriate #ifdefs. Mostly looks good, but there were a few places where logical changes are outside of #ifdefs and thus affect other platforms. I actually can't build breakpad on x86 after applying this patch. 64 -std=c++11 -MT src/client/linux/minidump_writer/minidump_writer.o -MD -MP -MF $depbase.Tpo -c -o src/client/linux/minidump_writer/minidump_writer.o src/client/linux/minidump_writer/minidump_writer.cc &&\ mv -f $depbase.Tpo $depbase.Po src/client/linux/handler/exception_handler.cc: In member function ‘bool google_breakpad::ExceptionHandler::HandleSignal(int, siginfo_t*, void*)’: src/client/linux/handler/exception_handler.cc:456:27: error: ‘struct mcontext_t’ has no member named ‘fp_regs’ if (uc_ptr->uc_mcontext.fp_regs) { ^ src/client/linux/handler/exception_handler.cc:457:63: error: ‘struct mcontext_t’ has no member named ‘fp_regs’ memcpy(&g_crash_context_.float_state, uc_ptr->uc_mcontext.fp_regs, ^ src/client/linux/handler/exception_handler.cc: In member function ‘bool google_breakpad::ExceptionHandler::WriteMinidump()’: src/client/linux/handler/exception_handler.cc:692:60: error: ‘struct mcontext_t’ has no member named ‘fp_regs’ memcpy(&context.float_state, context.context.uc_mcontext.fp_regs, ^ Makefile:4986: recipe for target 'src/client/linux/handler/exception_handler.o' failed make: *** [src/client/linux/handler/exception_handler.o] Error 1 make: *** Waiting for unfinished jobs.... tarmstrong@tarmstrong-box:~/Impala/native-toolchain$ (gvim:12601): GLib-GObject-WARNING **: cannot retrieve class for invalid (unclassed) type '<invalid>' http://gerrit.cloudera.org:8080/#/c/6468/3/source/breakpad/breakpad-88e5b2c_ppc.patch File source/breakpad/breakpad-88e5b2c_ppc.patch: Was this accidentally checked in? http://gerrit.cloudera.org:8080/#/c/6468/3/source/crcutil/build.sh File source/crcutil/build.sh: Line 36: Don't need this change http://gerrit.cloudera.org:8080/#/c/6468/3/source/crcutil/crcutil-440ba7babeff77ffad992df3a10c767f184e946e-patches/0002_crc_interface_cc.patch File source/crcutil/crcutil-440ba7babeff77ffad992df3a10c767f184e946e-patches/0002_crc_interface_cc.patch: Line 7: + #if CRCUTIL_USE_MM_CRC32 If I understand correctly, this is just a hack to remove code that doesn't compile without SSE4? And it doesn't matter because this is just example code? Can you comment that just so it's clear. E.g.: // Hack: get example to compile without SSE4. Line 12: + return 0; The second return should be in a #else, right? Otherwise the function has two returns on x86-64? http://gerrit.cloudera.org:8080/#/c/6468/3/source/kudu/build.sh File source/kudu/build.sh: Line 93: Can you revert the formatting changes? it just makes it harder to see where the lines came from historically. http://gerrit.cloudera.org:8080/#/c/6468/3/source/libunwind/build.sh File source/libunwind/build.sh: Line 35: echo "ppc64_test_altivec_LDADD = \$(LIBUNWIND)" >> $THIS_DIR/$PACKAGE-$PACKAGE_VERSION/tests/Makefile.am This could be a patch too, right? Sorry to be pedantic but I want to make things are set up to reduce the chances of us accidentally breaking your changes. http://gerrit.cloudera.org:8080/#/c/6468/3/source/llvm/build-3.3.sh File source/llvm/build-3.3.sh: Line 87: TARGET+="powerpc,cpp" nit: don't need TARGET= above, can just use TARGET= here and on line 88 http://gerrit.cloudera.org:8080/#/c/6468/3/source/llvm/build-source-tarball.sh File source/llvm/build-source-tarball.sh: Line 79: if [[ "$ARCH_NAME" == "ppc64le" ]]; then nit: don't need TARGET= above, can just use TARGET= here and on line 82 -- To view, visit http://gerrit.cloudera.org:8080/6468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7931752ca497bc7a5e3cc574bbb54637f382c72a Gerrit-PatchSet: 3 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Valencia Edna Serrao <vser...@us.ibm.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Valencia Edna Serrao <vser...@us.ibm.com> Gerrit-HasComments: Yes
