impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [native-toolchain-CR] Ported native-toolchain to work on ppc64le
Date Fri, 12 May 2017 01:06:36 GMT
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 <vserrao@us.ibm.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Valencia Edna Serrao <vserrao@us.ibm.com>
Gerrit-HasComments: Yes

Mime
View raw message