|
|
Created:
6 years, 9 months ago by Zhenyu Liang Modified:
6 years, 9 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionEliminate build warnings in base/ for Android x64
This CL fixes format strings of printf and type conversions
that cause compilation warnings while building Android x64.
BUG=346626
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257942
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed the style nits #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : rebase #
Messages
Total messages: 36 (0 generated)
lgtm % nits, good stuff, thanks!! I'm not an owner, so: +mark for base/* OWNERs https://codereview.chromium.org/185423006/diff/1/base/debug/stack_trace_andro... File base/debug/stack_trace_android.cc (right): https://codereview.chromium.org/185423006/diff/1/base/debug/stack_trace_andro... base/debug/stack_trace_android.cc:107: #ifdef __LP64__ nit: similarly to the previous file, please move this up, in between #include and the namespace declaration in line 13. https://codereview.chromium.org/185423006/diff/1/base/strings/string_piece_un... File base/strings/string_piece_unittest.cc (right): https://codereview.chromium.org/185423006/diff/1/base/strings/string_piece_un... base/strings/string_piece_unittest.cc:668: static_cast<typename BasicStringPiece<TypeParam>::size_type>(0))); nit: indent by another 2 spaces (and in 671 below as well).
lgtm with a nit (and +1 to bulach's comments). https://codereview.chromium.org/185423006/diff/1/base/debug/proc_maps_linux.cc File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/185423006/diff/1/base/debug/proc_maps_linux.c... base/debug/proc_maps_linux.cc:23: #else Since SCNxPTR is already defined as "lx" by Bionic, this can be made simpler as: #if defined(OS_ANDROID) && !defined(__LP64__) #undef SCNxPTR define SCNxPTR "x" #endif Please also update the commment approprately.
https://codereview.chromium.org/185423006/diff/20001/base/debug/proc_maps_lin... File base/debug/proc_maps_linux.cc (right): https://codereview.chromium.org/185423006/diff/20001/base/debug/proc_maps_lin... base/debug/proc_maps_linux.cc:18: // is incompatible with Bionic's stdint.h defining uintptr_t as a unsigned int: What does Bionic’s inttypes.h define PRI/SCNxPTR as in __LP64__, and what does stdint.h define uintptr_t as? At the very least, the comment is incorrect. But it’s also quite possible that you don’t need this new __LP64__ case inside the #ifdef, and can just put !defined(__LP64__) on line 16.
In 32-bit mode, SCNxPTR is already defined as "x" in current NDK. In 64-bit mode, it's defined as "lx", see: https://android.googlesource.com/platform/bionic/+/master/libc/include/inttyp... So, I think it's safe to remove this redefinition.
I had tried on my local machine. No warnings and errors while compiling proc_maps_linux.cc both in 32bit and 64bit. Mark, is it OK to land?
Zhenyu Liang wrote: > In 32-bit mode, SCNxPTR is already defined as "x" in current NDK. > In 64-bit mode, it's defined as "lx", see: > https://android.googlesource.com/platform/bionic/+/master/libc/include/inttyp... > > So, I think it's safe to remove this redefinition. See https://code.google.com/p/android/issues/detail?id=57218. It was not safe to remove the redefinition in the past, but Bionic and the NDK were fixed after we discovered this bug. Have our minimum requirements for these changed so that we’re guaranteed to get versions with fixes for that bug? Marcus, do you know?
afaict it's still a problem in the NDK versions we use: third_party/android_tools/ndk/platforms((0582bdc...))$ git grep SCNxPTR android-14/arch-arm/usr/include/inttypes.h:#define SCNxPTR "lx" /* uintptr_t */ all |inttypes.h| there define as "SCNxPTR" as "lx".. I suppose we still need the ifdef on our side, and now need to include the __LP64__ to be extra safe?
Yeah, I guess we need to use what Ross suggested.
On 2014/03/10 19:03:06, Mark Mentovai wrote: > Yeah, I guess we need to use what Ross suggested. Make sense. Add !defined(__LP64__) to the condition.
lgtm (but Mark is the real OWNER), thanks!
LGTM
The CQ bit was checked by zhenyu.liang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/185423006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by zhenyu.liang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/185423006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was unchecked by zhenyu.liang@intel.com
The CQ bit was checked by zhenyu.liang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/185423006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by zhenyu.liang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/185423006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for base/debug/proc_maps_linux.cc: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file base/debug/proc_maps_linux.cc Hunk #1 FAILED at 6. 1 out of 1 hunk FAILED -- saving rejects to file base/debug/proc_maps_linux.cc.rej Patch: base/debug/proc_maps_linux.cc Index: base/debug/proc_maps_linux.cc =================================================================== --- base/debug/proc_maps_linux.cc (revision 254431) +++ base/debug/proc_maps_linux.cc (working copy) @@ -6,16 +6,17 @@ #include <fcntl.h> -#if defined(OS_LINUX) +#if defined(OS_LINUX) || defined(OS_ANDROID) #include <inttypes.h> #endif #include "base/file_util.h" #include "base/strings/string_split.h" -#if defined(OS_ANDROID) -// Bionic's inttypes.h defines PRI/SCNxPTR as an unsigned long int, which -// is incompatible with Bionic's stdint.h defining uintptr_t as a unsigned int: +#if defined(OS_ANDROID) && !defined(__LP64__) +// In 32-bit mode, Bionic's inttypes.h defines PRI/SCNxPTR as an +// unsigned long int, which is incompatible with Bionic's stdint.h +// defining uintptr_t as an unsigned int: // https://code.google.com/p/android/issues/detail?id=57218 #undef SCNxPTR #define SCNxPTR "x"
The CQ bit was checked by zhenyu.liang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/185423006/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by zhenyu.liang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/185423006/100001
Message was sent while issue was closed.
Change committed as 257942 |