Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
835633003:
Enable libc++ on Android (Closed)
|
Created:
5 years, 11 months ago by jdduke (slow) Modified:
5 years, 10 months ago Reviewers:
jamesr, cpu_(ooo_6.6-7.5), boliu, davidben, piman, Nico, pasko, Fabrice (no longer in Chrome) CC:
chromium-reviews, cbentzel+watch_chromium.org, android-webview-reviews_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, erikwright+watch_chromium.org, jbudorick+watch_chromium.org, jshin+watch_chromium.org, andrewhsieh, Takashi Toyoshima, Torne, Tom Hudson, danakj, reveman, vmpstr, hans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable libc++ on Android
Switch Chrome on Android to use libc++ instead of stlport. WebView AOSP
builds will continue to use stlport for the time being.
Note that this change is exploratory, and likely to be reverted before
branch point (ensuring WebView and Chrome remain in lockstep with
respect to standard library dependencies).
BUG=427718
TBR=cpu@chromium.org
Committed: https://crrev.com/8681920fa22ded465054301ce96657a4ddaf2a04
Cr-Commit-Position: refs/heads/master@{#315085}
Patch Set 1 #Patch Set 2 : Fix compile #Patch Set 3 : Working build #Patch Set 4 : Rebase with fixed AOSP dependency #Patch Set 5 : Rebase #Patch Set 6 : Fix clang #Patch Set 7 : Fix spdy test... still not sure what's going on with positional parameters #Patch Set 8 : Rebase #Patch Set 9 : libc++ for non-AOSP on Android #Patch Set 10 : USE_STLPORT for aosp #Patch Set 11 : Really fix AOSP #
Total comments: 3
Patch Set 12 : Rebase #Patch Set 13 : Rebase to friending fixes #
Total comments: 14
Patch Set 14 : Test GN #Patch Set 15 : Rebased to positional param revert #
Total comments: 6
Patch Set 16 : Code review #
Messages
Total messages: 64 (16 generated)
Looks like the remaining failure is for aosp from within Skia (see below). I
recall danalbert@ mentioning a skia move to libc++ for aosp a while back?
In file included from
external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247:0,
from
external/chromium_org/third_party/skia/include/core/SkTypes.h:13,
from
external/chromium_org/third_party/skia/include/core/SkFixed.h:11,
from
external/chromium_org/third_party/skia/include/core/SkScalar.h:11,
from
external/chromium_org/third_party/skia/include/core/SkColor.h:13,
from
external/chromium_org/third_party/skia/include/core/SkBitmap.h:11,
from
external/chromium_org/third_party/skia/src/ports/SkFontHost_FreeType_common.cpp:9:
prebuilts/ndk/current/sources/android/support/include/stdlib.h:47:90: error:
declaration of C function 'long long int strtoll_l(const char*, char**, size_t,
locale_t)' conflicts with
long long strtoll_l(const char *nptr, char **endptr, size_t base,
locale_t loc);
^
In file included from
prebuilts/ndk/current/sources/android/support/include/stdlib.h:33:0,
from
external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247,
from
external/chromium_org/third_party/skia/include/core/SkTypes.h:13,
from
external/chromium_org/third_party/skia/include/core/SkFixed.h:11,
from
external/chromium_org/third_party/skia/include/core/SkScalar.h:11,
from
external/chromium_org/third_party/skia/include/core/SkColor.h:13,
from
external/chromium_org/third_party/skia/include/core/SkBitmap.h:11,
from
external/chromium_org/third_party/skia/src/ports/SkFontHost_FreeType_common.cpp:9:
prebuilts/ndk/current/platforms/android-21/arch-arm/usr/include/stdlib.h:79:18:
error: previous declaration 'long long int strtoll_l(const char*, char**, int,
locale_t)' here
extern long long strtoll_l(const char *, char **, int, locale_t)
__LIBC_ABI_PUBLIC__;
On 2015/01/07 01:05:00, jdduke wrote: > Looks like the remaining failure is for aosp from within Skia (see below). I > recall danalbert@ mentioning a skia move to libc++ for aosp a while back? > > In file included from > external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247:0, > from > external/chromium_org/third_party/skia/include/core/SkTypes.h:13, > from > external/chromium_org/third_party/skia/include/core/SkFixed.h:11, > from > external/chromium_org/third_party/skia/include/core/SkScalar.h:11, > from > external/chromium_org/third_party/skia/include/core/SkColor.h:13, > from > external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, > from > external/chromium_org/third_party/skia/src/ports/SkFontHost_FreeType_common.cpp:9: > prebuilts/ndk/current/sources/android/support/include/stdlib.h:47:90: error: > declaration of C function 'long long int strtoll_l(const char*, char**, size_t, > locale_t)' conflicts with > long long strtoll_l(const char *nptr, char **endptr, size_t base, > locale_t loc); > > ^ > In file included from > prebuilts/ndk/current/sources/android/support/include/stdlib.h:33:0, > from > external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247, > from > external/chromium_org/third_party/skia/include/core/SkTypes.h:13, > from > external/chromium_org/third_party/skia/include/core/SkFixed.h:11, > from > external/chromium_org/third_party/skia/include/core/SkScalar.h:11, > from > external/chromium_org/third_party/skia/include/core/SkColor.h:13, > from > external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, > from > external/chromium_org/third_party/skia/src/ports/SkFontHost_FreeType_common.cpp:9: > prebuilts/ndk/current/platforms/android-21/arch-arm/usr/include/stdlib.h:79:18: > error: previous declaration 'long long int strtoll_l(const char*, char**, int, > locale_t)' here > extern long long strtoll_l(const char *, char **, int, locale_t) > __LIBC_ABI_PUBLIC__; Looks like a bug in the NDK. I'm not sure where the android/support/include/stdlib.h came from, but it has size_t as a base for those functions rather than an int. andrewhsieh: I'm guessing that was just copied from the bionic versions that were similarly broken when I first added them and they just need to be updated, but I don't know the process for updating these headers.
Indeed, the headers in prebuilts/ndk is out of sync. https://android-review.googlesource.com/#/c/121981 should fix it. The process of updating is readlly just dumping released ndk onto prebuilts/ndk/current, but it hasn't happen since last Summer :( On Wed, Jan 7, 2015 at 9:17 AM, <danalbert@google.com> wrote: > On 2015/01/07 01:05:00, jdduke wrote: > >> Looks like the remaining failure is for aosp from within Skia (see >> below). I >> recall danalbert@ mentioning a skia move to libc++ for aosp a while back? >> > > In file included from >> external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247:0, >> from >> external/chromium_org/third_party/skia/include/core/SkTypes.h:13, >> from >> external/chromium_org/third_party/skia/include/core/SkFixed.h:11, >> from >> external/chromium_org/third_party/skia/include/core/SkScalar.h:11, >> from >> external/chromium_org/third_party/skia/include/core/SkColor.h:13, >> from >> external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, >> from >> > > external/chromium_org/third_party/skia/src/ports/ > SkFontHost_FreeType_common.cpp:9: > >> prebuilts/ndk/current/sources/android/support/include/stdlib.h:47:90: >> error: >> declaration of C function 'long long int strtoll_l(const char*, char**, >> > size_t, > >> locale_t)' conflicts with >> long long strtoll_l(const char *nptr, char **endptr, size_t >> base, >> locale_t loc); >> > > > ^ >> In file included from >> prebuilts/ndk/current/sources/android/support/include/stdlib.h:33:0, >> from >> external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247, >> from >> external/chromium_org/third_party/skia/include/core/SkTypes.h:13, >> from >> external/chromium_org/third_party/skia/include/core/SkFixed.h:11, >> from >> external/chromium_org/third_party/skia/include/core/SkScalar.h:11, >> from >> external/chromium_org/third_party/skia/include/core/SkColor.h:13, >> from >> external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, >> from >> > > external/chromium_org/third_party/skia/src/ports/ > SkFontHost_FreeType_common.cpp:9: > > prebuilts/ndk/current/platforms/android-21/arch-arm/ > usr/include/stdlib.h:79:18: > >> error: previous declaration 'long long int strtoll_l(const char*, char**, >> int, >> locale_t)' here >> extern long long strtoll_l(const char *, char **, int, locale_t) >> __LIBC_ABI_PUBLIC__; >> > > Looks like a bug in the NDK. I'm not sure where the > android/support/include/stdlib.h came from, but it has size_t as a base > for > those functions rather than an int. > > andrewhsieh: I'm guessing that was just copied from the bionic versions > that > were similarly broken when I first added them and they just need to be > updated, > but I don't know the process for updating these headers. > > https://codereview.chromium.org/835633003/ > -- Thanks, Andrew To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/07 10:19:47, chromium-reviews wrote: > Indeed, the headers in prebuilts/ndk is out of sync. > https://android-review.googlesource.com/#/c/121981 should fix it. The > process of updating is readlly just dumping released ndk onto > prebuilts/ndk/current, but it hasn't happen since last Summer :( Thanks Andrew, if you ping us when that's landed in AOSP master we can roll the version used by the bot here. > On Wed, Jan 7, 2015 at 9:17 AM, <mailto:danalbert@google.com> wrote: > > > On 2015/01/07 01:05:00, jdduke wrote: > > > >> Looks like the remaining failure is for aosp from within Skia (see > >> below). I > >> recall danalbert@ mentioning a skia move to libc++ for aosp a while back? > >> > > > > In file included from > >> external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247:0, > >> from > >> external/chromium_org/third_party/skia/include/core/SkTypes.h:13, > >> from > >> external/chromium_org/third_party/skia/include/core/SkFixed.h:11, > >> from > >> external/chromium_org/third_party/skia/include/core/SkScalar.h:11, > >> from > >> external/chromium_org/third_party/skia/include/core/SkColor.h:13, > >> from > >> external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, > >> from > >> > > > > external/chromium_org/third_party/skia/src/ports/ > > SkFontHost_FreeType_common.cpp:9: > > > >> prebuilts/ndk/current/sources/android/support/include/stdlib.h:47:90: > >> error: > >> declaration of C function 'long long int strtoll_l(const char*, char**, > >> > > size_t, > > > >> locale_t)' conflicts with > >> long long strtoll_l(const char *nptr, char **endptr, size_t > >> base, > >> locale_t loc); > >> > > > > > > ^ > >> In file included from > >> prebuilts/ndk/current/sources/android/support/include/stdlib.h:33:0, > >> from > >> external/chromium_org/third_party/skia/include/core/SkPostConfig.h:247, > >> from > >> external/chromium_org/third_party/skia/include/core/SkTypes.h:13, > >> from > >> external/chromium_org/third_party/skia/include/core/SkFixed.h:11, > >> from > >> external/chromium_org/third_party/skia/include/core/SkScalar.h:11, > >> from > >> external/chromium_org/third_party/skia/include/core/SkColor.h:13, > >> from > >> external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, > >> from > >> > > > > external/chromium_org/third_party/skia/src/ports/ > > SkFontHost_FreeType_common.cpp:9: > > > > prebuilts/ndk/current/platforms/android-21/arch-arm/ > > usr/include/stdlib.h:79:18: > > > >> error: previous declaration 'long long int strtoll_l(const char*, char**, > >> int, > >> locale_t)' here > >> extern long long strtoll_l(const char *, char **, int, locale_t) > >> __LIBC_ABI_PUBLIC__; > >> > > > > Looks like a bug in the NDK. I'm not sure where the > > android/support/include/stdlib.h came from, but it has size_t as a base > > for > > those functions rather than an int. > > > > andrewhsieh: I'm guessing that was just copied from the bionic versions > > that > > were similarly broken when I first added them and they just need to be > > updated, > > but I don't know the process for updating these headers. > > > > https://codereview.chromium.org/835633003/ > > > > > > -- > Thanks, > Andrew > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
It's merged now. Thanks! On Wed, Jan 7, 2015 at 6:39 PM, <torne@chromium.org> wrote: > On 2015/01/07 10:19:47, chromium-reviews wrote: > >> Indeed, the headers in prebuilts/ndk is out of sync. >> https://android-review.googlesource.com/#/c/121981 should fix it. The >> process of updating is readlly just dumping released ndk onto >> prebuilts/ndk/current, but it hasn't happen since last Summer :( >> > > Thanks Andrew, if you ping us when that's landed in AOSP master we can > roll the > version used by the bot here. > > > On Wed, Jan 7, 2015 at 9:17 AM, <mailto:danalbert@google.com> wrote: >> > > > On 2015/01/07 01:05:00, jdduke wrote: >> > >> >> Looks like the remaining failure is for aosp from within Skia (see >> >> below). I >> >> recall danalbert@ mentioning a skia move to libc++ for aosp a while >> back? >> >> >> > >> > In file included from >> >> external/chromium_org/third_party/skia/include/core/ >> SkPostConfig.h:247:0, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkTypes.h:13, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkFixed.h:11, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkScalar.h:11, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkColor.h:13, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, >> >> from >> >> >> > >> > external/chromium_org/third_party/skia/src/ports/ >> > SkFontHost_FreeType_common.cpp:9: >> > >> >> prebuilts/ndk/current/sources/android/support/include/stdlib.h:47:90: >> >> error: >> >> declaration of C function 'long long int strtoll_l(const char*, char**, >> >> >> > size_t, >> > >> >> locale_t)' conflicts with >> >> long long strtoll_l(const char *nptr, char **endptr, >> size_t >> >> base, >> >> locale_t loc); >> >> >> > >> > >> > ^ >> >> In file included from >> >> prebuilts/ndk/current/sources/android/support/include/stdlib.h:33:0, >> >> from >> >> external/chromium_org/third_party/skia/include/core/ >> SkPostConfig.h:247, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkTypes.h:13, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkFixed.h:11, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkScalar.h:11, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkColor.h:13, >> >> from >> >> external/chromium_org/third_party/skia/include/core/SkBitmap.h:11, >> >> from >> >> >> > >> > external/chromium_org/third_party/skia/src/ports/ >> > SkFontHost_FreeType_common.cpp:9: >> > >> > prebuilts/ndk/current/platforms/android-21/arch-arm/ >> > usr/include/stdlib.h:79:18: >> > >> >> error: previous declaration 'long long int strtoll_l(const char*, >> char**, >> >> int, >> >> locale_t)' here >> >> extern long long strtoll_l(const char *, char **, int, locale_t) >> >> __LIBC_ABI_PUBLIC__; >> >> >> > >> > Looks like a bug in the NDK. I'm not sure where the >> > android/support/include/stdlib.h came from, but it has size_t as a base >> > for >> > those functions rather than an int. >> > >> > andrewhsieh: I'm guessing that was just copied from the bionic versions >> > that >> > were similarly broken when I first added them and they just need to be >> > updated, >> > but I don't know the process for updating these headers. >> > >> > https://codereview.chromium.org/835633003/ >> > >> > > > > -- >> Thanks, >> Andrew >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/835633003/ > -- Thanks, Andrew To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/07 11:32:44, chromium-reviews wrote: > It's merged now. Thanks! Thank you! Rolled in with https://codereview.chromium.org/796393006/.
Making progress:
In file included from
external/chromium_org/base/atomicops_internals_portable.h:35:0,
from external/chromium_org/base/atomicops.h:178,
from external/chromium_org/base/atomic_ref_count.h:14,
from external/chromium_org/base/memory/ref_counted.h:11,
from
external/chromium_org/gpu/command_buffer/common/buffer.h:9,
from
external/chromium_org/gpu/command_buffer/common/command_buffer.h:8,
from
external/chromium_org/gpu/command_buffer/common/cmd_buffer_common.cc:10:
prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/atomic:539:2:
error: #error <atomic> is not implemented
I was under the impression <atomic> worked for >= GCC 4.7, maybe there's an
additional location for enabling (latomic?) on AOSP?
On 2015/01/07 20:27:31, jdduke wrote: > Making progress: > > In file included from > external/chromium_org/base/atomicops_internals_portable.h:35:0, > from external/chromium_org/base/atomicops.h:178, > from external/chromium_org/base/atomic_ref_count.h:14, > from external/chromium_org/base/memory/ref_counted.h:11, > from > external/chromium_org/gpu/command_buffer/common/buffer.h:9, > from > external/chromium_org/gpu/command_buffer/common/command_buffer.h:8, > from > external/chromium_org/gpu/command_buffer/common/cmd_buffer_common.cc:10: > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/atomic:539:2: > error: #error <atomic> is not implemented > > I was under the impression <atomic> worked for >= GCC 4.7, maybe there's an > additional location for enabling (latomic?) on AOSP? <__config> in the llvm-libc++ code contains emulation for clang's __has_feature() which hardcodes atomic support to false, with a comment saying // (_GNUC_VER >= 409) seems to support _Atomic in -std=c11 not -std=c++11 ! So it looks like the libc++ version in Android doesn't think gcc supports this.
On 2015/01/08 11:07:30, Torne wrote: > On 2015/01/07 20:27:31, jdduke wrote: > > Making progress: > > > > In file included from > > external/chromium_org/base/atomicops_internals_portable.h:35:0, > > from external/chromium_org/base/atomicops.h:178, > > from external/chromium_org/base/atomic_ref_count.h:14, > > from external/chromium_org/base/memory/ref_counted.h:11, > > from > > external/chromium_org/gpu/command_buffer/common/buffer.h:9, > > from > > external/chromium_org/gpu/command_buffer/common/command_buffer.h:8, > > from > > external/chromium_org/gpu/command_buffer/common/cmd_buffer_common.cc:10: > > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/atomic:539:2: > > error: #error <atomic> is not implemented > > > > I was under the impression <atomic> worked for >= GCC 4.7, maybe there's an > > additional location for enabling (latomic?) on AOSP? > > <__config> in the llvm-libc++ code contains emulation for clang's > __has_feature() which hardcodes atomic support to false, with a comment saying > // (_GNUC_VER >= 409) seems to support _Atomic in -std=c11 not -std=c++11 ! > > So it looks like the libc++ version in Android doesn't think gcc supports this. The latest trunk of libc++ has changed the support check to "#if !__has_feature(cxx_atomic) && _GNUC_VER < 407", so it looks like we need a newer libc++ in AOSP for this.
https://android-review.googlesource.com/#/c/122290 under verification updates prebuilts/ndk llvm-libc++ to NDK r10d with GCC/atomic support. This is closer to upstream but in the long run NDK's libc++ will be built from the same source (release drop) as platform's external/libcxx[abi] maintained by danalbert@ On Thu, Jan 8, 2015 at 7:09 PM, <torne@chromium.org> wrote: > On 2015/01/08 11:07:30, Torne wrote: > >> On 2015/01/07 20:27:31, jdduke wrote: >> > Making progress: >> > >> > In file included from >> > external/chromium_org/base/atomicops_internals_portable.h:35:0, >> > from external/chromium_org/base/atomicops.h:178, >> > from external/chromium_org/base/atomic_ref_count.h:14, >> > from external/chromium_org/base/ >> memory/ref_counted.h:11, >> > from >> > external/chromium_org/gpu/command_buffer/common/buffer.h:9, >> > from >> > external/chromium_org/gpu/command_buffer/common/command_buffer.h:8, >> > from >> > external/chromium_org/gpu/command_buffer/common/cmd_ >> buffer_common.cc:10: >> > >> > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/ > include/atomic:539:2: > >> > error: #error <atomic> is not implemented >> > >> > I was under the impression <atomic> worked for >= GCC 4.7, maybe >> there's an >> > additional location for enabling (latomic?) on AOSP? >> > > <__config> in the llvm-libc++ code contains emulation for clang's >> __has_feature() which hardcodes atomic support to false, with a comment >> saying >> // (_GNUC_VER >= 409) seems to support _Atomic in -std=c11 not -std=c++11 >> ! >> > > So it looks like the libc++ version in Android doesn't think gcc supports >> > this. > > The latest trunk of libc++ has changed the support check to "#if > !__has_feature(cxx_atomic) && _GNUC_VER < 407", so it looks like we need a > newer > libc++ in AOSP for this. > > https://codereview.chromium.org/835633003/ > -- Thanks, Andrew To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/08 12:06:50, chromium-reviews wrote: > https://android-review.googlesource.com/#/c/122290 under verification > updates prebuilts/ndk llvm-libc++ to NDK r10d with GCC/atomic support. > This is closer to upstream but in the long run NDK's libc++ will be built > from the same source (release drop) as platform's external/libcxx[abi] > maintained by danalbert@ Fantastic, thanks!
jdduke@chromium.org changed reviewers: + fdegans@chromium.org, jamesr@chromium.org, pasko@chromium.org
With the landing of https://codereview.chromium.org/846633003/, I think we'll be very close to a functional libc++ build for all relevant Android targets. fdegans@, pasko@: Do you want me to polish this and push for review? Or should we push a previous versions, e.g., https://codereview.chromium.org/691783003/ (I think jamesr@ may also have an older variant around somewhere)?
On 2015/01/14 00:01:16, jdduke wrote: > With the landing of https://codereview.chromium.org/846633003/, I think we'll be > very close to a functional libc++ build for all relevant Android targets. Woohoo! > fdegans@, pasko@: Do you want me to polish this and push for review? Or should > we push a previous versions, e.g., https://codereview.chromium.org/691783003/ (I > think jamesr@ may also have an older variant around somewhere)? https://codereview.chromium.org/691783003/ is a derivation of my old patch - not sure if I have an older one lying around still but if I do it'll probably be even less useful.
On 2015/01/14 00:04:07, jamesr wrote: > On 2015/01/14 00:01:16, jdduke wrote: > > With the landing of https://codereview.chromium.org/846633003/, I think we'll > be > > very close to a functional libc++ build for all relevant Android targets. > > Woohoo! > > > fdegans@, pasko@: Do you want me to polish this and push for review? Or > should > > we push a previous versions, e.g., https://codereview.chromium.org/691783003/ > (I > > think jamesr@ may also have an older variant around somewhere)? if you have cycles, you are very welcome! My feeling of the work remaining, to make sure that: 1. nothing breaks at runtime (trybots ftw) 2. perf tests like smoothness/raster/thread_times/PLT don't regress significantly .. and the binary size is not expected to drift much, so we can consider these <100KiB increase acceptable. > https://codereview.chromium.org/691783003/ is a derivation of my old patch - not > sure if I have an older one lying around still but if I do it'll probably be > even less useful. there are older patches around, from awong, myself, and others and others, and you are right, they are not useful right now.
On 2015/01/14 01:17:24, pasko wrote: > On 2015/01/14 00:04:07, jamesr wrote: > > On 2015/01/14 00:01:16, jdduke wrote: > > > With the landing of https://codereview.chromium.org/846633003/, I think > we'll > > be > > > very close to a functional libc++ build for all relevant Android targets. > > > > Woohoo! > > > > > fdegans@, pasko@: Do you want me to polish this and push for review? Or > > should > > > we push a previous versions, e.g., > https://codereview.chromium.org/691783003/ > > (I > > > think jamesr@ may also have an older variant around somewhere)? > > if you have cycles, you are very welcome! > > My feeling of the work remaining, to make sure that: > 1. nothing breaks at runtime (trybots ftw) > 2. perf tests like smoothness/raster/thread_times/PLT don't regress > significantly > > .. and the binary size is not expected to drift much, so we can consider these > <100KiB increase acceptable. > > > https://codereview.chromium.org/691783003/ is a derivation of my old patch - > not > > sure if I have an older one lying around still but if I do it'll probably be > > even less useful. > > there are older patches around, from awong, myself, and others and others, and > you are right, they are not useful right now. Great, I'll see what I can do. There was a runtime issue with component builds, but Chris tracked it down to a bad regex assumption. The corresponding fix should land soon: https://codereview.chromium.org/849973002/.
Still hitting some snags on aosp. Atomics look good now, but Skia compilation
fails with:
In file included from
prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/exception:81:0,
from
prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/new:68,
from
external/chromium_org/third_party/skia/include/core/SkTemplates.h:16,
from
external/chromium_org/third_party/skia/include/core/SkRefCnt.h:16,
from
external/chromium_org/third_party/skia/include/core/SkFlattenable.h:11,
from
external/chromium_org/third_party/skia/include/core/SkColorTable.h:14,
from
external/chromium_org/third_party/skia/include/core/SkBitmap.h:12,
from
external/chromium_org/third_party/skia/src/ports/SkFontHost_FreeType_common.cpp:9:
prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/type_traits:1722:8:
error: redefinition of 'struct std::__1::__member_pointer_traits_imp<_Rp
(_Class::*)(_Param ...), true, false>'
struct __member_pointer_traits_imp<_Rp (_Class::*)(_Param...) &, true, false>
^
prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/type_traits:1688:8:
error: previous definition of 'struct std::__1::__member_pointer_traits_imp<_Rp
(_Class::*)(_Param ...), true, false>'
struct __member_pointer_traits_imp<_Rp (_Class::*)(_Param...), true, false>
andrewhsieh@: Look at all familiar?
SkFontHost_FreeType_common.cpp in aosp lunch aosp_arm-eng compile w/o problem. I replace "-I external/libcxx/include" with "-I prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include" and it compile as well. Did you add -std=gnu++11 or -std=c++11 ? prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.9/bin/arm-linux-androideabi-g++ -I external/skia/include/config -I external/skia/include/core -I external/skia/include/pathops -I external/skia/include/pipe -I external/skia/include/ports -I external/skia/include/utils -I external/skia/include/xml -I external/skia/src/core -I external/skia/src/sfnt -I external/skia/src/image -I external/skia/src/opts -I external/skia/src/utils -I external/skia/include/gpu -I external/skia/src/gpu -I external/skia/include/effects -I external/skia/src/effects -I external/skia/include/images -I external/jpeg -I external/skia/src/lazy -I external/skia/src/images -I external/skia/third_party/etc1 -I external/skia/third_party/ktx -I external/webp/include -I external/giflib -I external/libpng -I external/expat/lib -I external/freetype/include -I external/skia/include/utils/win -I external/skia/src/ports -I external/skia/include/pdf -I external/skia/src/pdf -I external/sfntly/cpp/src -I external/zlib -I external/libcxx/include -I external/skia -I out.aosp_arm-eng/target/product/generic/obj/SHARED_LIBRARIES/libskia_intermediates -I out.aosp_arm-eng/target/product/generic/gen/SHARED_LIBRARIES/libskia_intermediates -I libnativehelper/include/nativehelper -I external/zlib -I external/icu/icu4c/source/common -I external/icu/icu4c/source/i18n -isystem system/core/include -isystem hardware/libhardware/include -isystem hardware/libhardware_legacy/include -isystem hardware/ril/include -isystem libnativehelper/include -isystem frameworks/native/include -isystem frameworks/native/opengl/include -isystem frameworks/av/include -isystem frameworks/base/include -isystem out.aosp_arm-eng/target/product/generic/obj/include -isystem bionic/libc/arch-arm/include -isystem bionic/libc/include -isystem bionic/libc/kernel/uapi -isystem bionic/libc/kernel/uapi/asm-arm -isystem bionic/libm/include -isystem bionic/libm/include/arm -c -fno-exceptions -Wno-multichar -msoft-float -ffunction-sections -fdata-sections -funwind-tables -fstack-protector -Wa,--noexecstack -Werror=format-security -D_FORTIFY_SOURCE=2 -fno-short-enums -no-canonical-prefixes -fno-canonical-system-headers -march=armv7-a -mfloat-abi=softfp -mfpu=vfpv3-d16 -include build/core/combo/include/arch/linux-arm/AndroidConfig.h -I build/core/combo/include/arch/linux-arm/ -fno-builtin-sin -fno-strict-volatile-bitfields -Wno-psabi -mthumb-interwork -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -g -Wstrict-aliasing=2 -fgcse-after-reload -frerun-cse-after-loop -frename-registers -DNDEBUG -UDEBUG -fvisibility-inlines-hidden -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -Wsign-promo -std=gnu++11 -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -UDEBUG -mthumb -Os -fomit-frame-pointer -fno-strict-aliasing -fno-rtti -DANDROID_LARGE_MEMORY_DEVICE -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -fPIC -D_USING_LIBCXX -Wno-invalid-offsetof -MD -MF out.aosp_arm-eng/target/product/generic/obj/SHARED_LIBRARIES/libskia_intermediates/src/ports/SkFontHost_FreeType_common.d -o out.aosp_arm-eng/target/product/generic/obj/SHARED_LIBRARIES/libskia_intermediates/src/ports/SkFontHost_FreeType_common.o external/skia/src/ports/SkFontHost_FreeType_common.cpp On Wed, Jan 14, 2015 at 2:04 PM, <jdduke@chromium.org> wrote: > Still hitting some snags on aosp. Atomics look good now, but Skia > compilation > fails with: > > In file included from > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/ > include/exception:81:0, > from > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/include/new:68, > from > external/chromium_org/third_party/skia/include/core/SkTemplates.h:16, > from > external/chromium_org/third_party/skia/include/core/SkRefCnt.h:16, > from > external/chromium_org/third_party/skia/include/core/SkFlattenable.h:11, > from > external/chromium_org/third_party/skia/include/core/SkColorTable.h:14, > from > external/chromium_org/third_party/skia/include/core/SkBitmap.h:12, > from > external/chromium_org/third_party/skia/src/ports/ > SkFontHost_FreeType_common.cpp:9: > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/ > include/type_traits:1722:8: > error: redefinition of 'struct std::__1::__member_pointer_traits_imp<_Rp > (_Class::*)(_Param ...), true, false>' > struct __member_pointer_traits_imp<_Rp (_Class::*)(_Param...) &, true, > false> > ^ > prebuilts/ndk/current/sources/cxx-stl/llvm-libc++/libcxx/ > include/type_traits:1688:8: > error: previous definition of 'struct std::__1::__member_pointer_ > traits_imp<_Rp > (_Class::*)(_Param ...), true, false>' > struct __member_pointer_traits_imp<_Rp (_Class::*)(_Param...), true, > false> > > andrewhsieh@: Look at all familiar? > > https://codereview.chromium.org/835633003/ > -- Thanks, Andrew To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
torne@, tomhudson@: Is there perhaps some Skia/AOSP deps wizardry that needs to happen here?
Patchset #8 (id:140001) has been deleted
Patchset #11 (id:220001) has been deleted
Patchset #12 (id:260001) has been deleted
Patchset #11 (id:240001) has been deleted
Patchset #11 (id:280001) has been deleted
Patchset #12 (id:320001) has been deleted
Patchset #11 (id:300001) has been deleted
jdduke@chromium.org changed reviewers: + thakis@chromium.org
thakis@: The latest patchset accommodates libc++ for Chromium Android builds, and stlport for AOSP builds. Everything appears to compile and run properly, the key unresolved issues being: 1) Some cc unit test failures: (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...). Still can't tell whether this indicates a fundamental problem, perhaps a libc++ Android quirk we've yet to come across, or the test can be simply tweaked. 2) Positional arguments: I could completely remove the test, or keep it ifdef'ed out as it is currently. Thoughts? Try landing this for a week? Or just hold off until after branch and AOSP is on track for standalone builds?
Hmm, those cc tests depend on std::bitset<> which isn't a terribly common data structure so it's possible that it's broken. Or that something else is wonky.
On 2015/01/29 22:44:14, jdduke wrote: > thakis@: The latest patchset accommodates libc++ for Chromium Android builds, > and stlport for AOSP builds. Everything appears to compile and run properly, the > key unresolved issues being: > > 1) Some cc unit test failures: > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...). > Still can't tell whether this indicates a fundamental problem, perhaps a libc++ > Android quirk we've yet to come across, or the test can be simply tweaked. Can you repro these locally? I think it'd be good to understand what'd going on here. > 2) Positional arguments: > I could completely remove the test, or keep it ifdef'ed out as it is currently. I'd revert https://codereview.chromium.org/9702002 which will remove the test. > Thoughts? Try landing this for a week? Or just hold off until after branch and > AOSP is on track for standalone builds? I'd try landing it and see what breaks. (Once the cc test failures are understood.)
On 2015/01/29 22:48:54, Nico wrote: > On 2015/01/29 22:44:14, jdduke wrote: > > thakis@: The latest patchset accommodates libc++ for Chromium Android builds, > > and stlport for AOSP builds. Everything appears to compile and run properly, > the > > key unresolved issues being: > > > > 1) Some cc unit test failures: > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...). > > Still can't tell whether this indicates a fundamental problem, perhaps a > libc++ > > Android quirk we've yet to come across, or the test can be simply tweaked. > > Can you repro these locally? I think it'd be good to understand what'd going on > here. > Yup, definitely repros consistently locally. My first hunch was bitset weirdness as jamesr mentioned, which still may be the case though the obvious places in the test don't seem to indicate anything out of the ordinary. Will keep digging.
https://codereview.chromium.org/835633003/diff/340001/net/base/address_tracke... File net/base/address_tracker_linux_unittest.cc (right): https://codereview.chromium.org/835633003/diff/340001/net/base/address_tracke... net/base/address_tracker_linux_unittest.cc:126: // GCC + libc++ friending issues on Android. Is this still needed? I thought aosp keeps using stlport for now, and the libc++ in our ndk has the workaround from what i understand https://codereview.chromium.org/835633003/diff/340001/net/spdy/spdy_prefixed_... File net/spdy/spdy_prefixed_buffer_reader_test.cc (right): https://codereview.chromium.org/835633003/diff/340001/net/spdy/spdy_prefixed_... net/spdy/spdy_prefixed_buffer_reader_test.cc:33: EXPECT_EQ(StringPiece(buffer), StringPiece("foobar")); same question for this file
https://codereview.chromium.org/835633003/diff/340001/net/base/address_tracke... File net/base/address_tracker_linux_unittest.cc (right): https://codereview.chromium.org/835633003/diff/340001/net/base/address_tracke... net/base/address_tracker_linux_unittest.cc:126: // GCC + libc++ friending issues on Android. On 2015/01/29 22:51:33, Nico wrote: > Is this still needed? I thought aosp keeps using stlport for now, and the libc++ > in our ndk has the workaround from what i understand Do you know when the fix rolled/landed? I tried reverting this change yesterday but it complained.
On Thu, Jan 29, 2015 at 2:56 PM, <jdduke@chromium.org> wrote: > > https://codereview.chromium.org/835633003/diff/340001/net/ > base/address_tracker_linux_unittest.cc > File net/base/address_tracker_linux_unittest.cc (right): > > https://codereview.chromium.org/835633003/diff/340001/net/ > base/address_tracker_linux_unittest.cc#newcode126 > net/base/address_tracker_linux_unittest.cc:126: // GCC + libc++ > friending issues on Android. > On 2015/01/29 22:51:33, Nico wrote: > >> Is this still needed? I thought aosp keeps using stlport for now, and >> > the libc++ > >> in our ndk has the workaround from what i understand >> > > Do you know when the fix rolled/landed? I tried reverting this change > yesterday but it complained. > It's here: https://chromium-review.googlesource.com/#/c/243891/ looks like it didn't land / roll yet. But we should wait for that to happen before landing this CL. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/29 22:56:37, jdduke wrote: > https://codereview.chromium.org/835633003/diff/340001/net/base/address_tracke... > File net/base/address_tracker_linux_unittest.cc (right): > > https://codereview.chromium.org/835633003/diff/340001/net/base/address_tracke... > net/base/address_tracker_linux_unittest.cc:126: // GCC + libc++ friending issues > on Android. > On 2015/01/29 22:51:33, Nico wrote: > > Is this still needed? I thought aosp keeps using stlport for now, and the > libc++ > > in our ndk has the workaround from what i understand > > Do you know when the fix rolled/landed? I tried reverting this change yesterday > but it complained. Sorry, I'll poke people tomorrow but there's only 3 people who have commit access to the android_tools repo so it still hasn't landed.
On 2015/01/29 22:57:49, Nico wrote: > It's here: https://chromium-review.googlesource.com/#/c/243891/ looks like > it didn't land / roll yet. But we should wait for that to happen before > landing this CL. Sounds good, the less churn the better.
On 2015/01/29 22:50:36, jdduke wrote: > On 2015/01/29 22:48:54, Nico wrote: > > On 2015/01/29 22:44:14, jdduke wrote: > > > thakis@: The latest patchset accommodates libc++ for Chromium Android > builds, > > > and stlport for AOSP builds. Everything appears to compile and run properly, > > the > > > key unresolved issues being: > > > > > > 1) Some cc unit test failures: > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...). > > > Still can't tell whether this indicates a fundamental problem, perhaps a > > libc++ > > > Android quirk we've yet to come across, or the test can be simply tweaked. > > > > Can you repro these locally? I think it'd be good to understand what'd going > on > > here. > > > > Yup, definitely repros consistently locally. My first hunch was bitset weirdness > as jamesr mentioned, which still may be the case though the obvious places in > the test don't seem to indicate anything out of the ordinary. Will keep digging. I think we can rule out bitset. Rolled a minimal version and replaced the one used by the task worker pool, fails identically (while passing on other platforms).
So, it looks like all scheduled task sets run to completion (calling DidFinishRunningTileTasks), but not in the order expected by the test. I'm not familiar enough with this code to say whether this is a legitimate error or a bad test expectation that happens to pass on all other platforms. The ordering difference might suggest some weird/different heap behavior (see make_heap/push_heap/pop_heap in task_graph_runner.cc), but it's hard to say, particularly as there's a ton of duplicated code across each of the worker pool |ScheduleTasks| implementations (looks like it was copy+pasted?). Victor, can you chime in here?
On 2015/01/30 01:27:25, jdduke wrote: > So, it looks like all scheduled task sets run to completion (calling > DidFinishRunningTileTasks), but not in the order expected by the test. I'm not > familiar enough with this code to say whether this is a legitimate error or a > bad test expectation that happens to pass on all other platforms. > > The ordering difference might suggest some weird/different heap behavior (see > make_heap/push_heap/pop_heap in task_graph_runner.cc), but it's hard to say, > particularly as there's a ton of duplicated code across each of the worker pool > |ScheduleTasks| implementations (looks like it was copy+pasted?). > > Victor, can you chime in here? reveman@ and vmpstr@ would know more about |ScheduleTasks|. Given the failure, I suspect the events could be returned in a different order due to variation in some algorithm like std::make_heap used in TaskGraphRunner. The notification tasks currently all get priority |kTaskSetFinishedTaskPriority|, e.g. here https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/gpu_t... If I'm right then changing to priority |kTaskSetFinishedTaskPriority + task_set| will allow the events to run in deterministic order.
On 2015/01/30 05:27:38, vmiura wrote: > On 2015/01/30 01:27:25, jdduke wrote: > > Victor, can you chime in here? > > reveman@ and vmpstr@ would know more about |ScheduleTasks|. > > Given the failure, I suspect the events could be returned in a different order > due to variation in some algorithm like std::make_heap used in TaskGraphRunner. > > The notification tasks currently all get priority > |kTaskSetFinishedTaskPriority|, e.g. here > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/gpu_t... > > If I'm right then changing to priority |kTaskSetFinishedTaskPriority + task_set| > will allow the events to run in deterministic order. You are the winner, good sir! With that adjustment to each of the pool implementations, all of the tests pass. Thanks!
Patchset #13 (id:380001) has been deleted
thakis@: Thoughts on the latest patchset? I've removed the workarounds for the two files (spdy_prefixed_buffer_reader_test.cc and address_tracker_linux_unittest.cc) that are no longer necessary. I've also landed a fix for the cc tests separately (some subtle differences in heap behavior). Haven't done anything about the positional arguments string formatting, I defer to your judgment there.
Thanks a lot for taking over! Bunch of comments, mostly nits. https://codereview.chromium.org/835633003/diff/400001/base/containers/hash_ta... File base/containers/hash_tables.h (left): https://codereview.chromium.org/835633003/diff/400001/base/containers/hash_ta... base/containers/hash_tables.h:87: #if !defined(OS_ANDROID) Doesn't that need to stay for aosp? https://codereview.chromium.org/835633003/diff/400001/base/strings/stringprin... File base/strings/stringprintf_unittest.cc (right): https://codereview.chromium.org/835633003/diff/400001/base/strings/stringprin... base/strings/stringprintf_unittest.cc:166: // TODO(fdegans): Fix positional parameters, or remove test entirely. Nit: I now realize this message is not entirely helpful :) Maybe rephrase to something like "Fix positional parameters with libc++ on Android, or disable the feature entirely."? https://codereview.chromium.org/835633003/diff/400001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/835633003/diff/400001/build/common.gypi#newco... build/common.gypi:4734: # Specify that we want to statically link libc++ from the Nit: revert this. https://codereview.chromium.org/835633003/diff/400001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/835633003/diff/400001/build/config/android/co... build/config/android/config.gni:132: # Libc++ stuff --------------------------------------------------------------- I think this whole section can go to compiler/BUILD.gn That's the only place where the variable is used. In fact, in the current state it's not even used anywhere. https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... build/config/compiler/BUILD.gn:534: # Android setup. This is specifically for standard library and linking setup on Android. There is another general Android section higher in the file for cflags. https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... build/config/compiler/BUILD.gn:544: defines += [ "__GNU_SOURCE=1" ] # Necessary for clone(). I am not sure if this would not be better fitted for the section that also defines "ANDROID" higher in the file, is it standard library related? https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... build/config/compiler/BUILD.gn:551: # Libc++ setup. This can go away if you change the above comment.
https://codereview.chromium.org/835633003/diff/400001/base/containers/hash_ta... File base/containers/hash_tables.h (left): https://codereview.chromium.org/835633003/diff/400001/base/containers/hash_ta... base/containers/hash_tables.h:87: #if !defined(OS_ANDROID) On 2015/02/04 13:24:22, Fabrice wrote: > Doesn't that need to stay for aosp? I actually don't know why this was removed, probably a faulty git reset on my part. https://codereview.chromium.org/835633003/diff/400001/base/strings/stringprin... File base/strings/stringprintf_unittest.cc (right): https://codereview.chromium.org/835633003/diff/400001/base/strings/stringprin... base/strings/stringprintf_unittest.cc:166: // TODO(fdegans): Fix positional parameters, or remove test entirely. On 2015/02/04 13:24:22, Fabrice wrote: > Nit: I now realize this message is not entirely helpful :) > Maybe rephrase to something like "Fix positional parameters with libc++ on > Android, or disable the feature entirely."? Looks like this will go away completely, will rebase after thakis@ lands the revert. https://codereview.chromium.org/835633003/diff/400001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/835633003/diff/400001/build/common.gypi#newco... build/common.gypi:4734: # Specify that we want to statically link libc++ from the On 2015/02/04 13:24:23, Fabrice wrote: > Nit: revert this. Done. https://codereview.chromium.org/835633003/diff/400001/build/config/android/co... File build/config/android/config.gni (right): https://codereview.chromium.org/835633003/diff/400001/build/config/android/co... build/config/android/config.gni:132: # Libc++ stuff --------------------------------------------------------------- On 2015/02/04 13:24:23, Fabrice wrote: > I think this whole section can go to compiler/BUILD.gn > That's the only place where the variable is used. > In fact, in the current state it's not even used anywhere. Done. https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... build/config/compiler/BUILD.gn:534: # Android setup. On 2015/02/04 13:24:23, Fabrice wrote: > This is specifically for standard library and linking setup on Android. > There is another general Android section higher in the file for cflags. Done. https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... build/config/compiler/BUILD.gn:544: defines += [ "__GNU_SOURCE=1" ] # Necessary for clone(). On 2015/02/04 13:24:23, Fabrice wrote: > I am not sure if this would not be better fitted for the section that also > defines "ANDROID" higher in the file, is it standard library related? Do we even need __GNU_SOURCE any longer? I'll try removing it. https://codereview.chromium.org/835633003/diff/400001/build/config/compiler/B... build/config/compiler/BUILD.gn:551: # Libc++ setup. On 2015/02/04 13:24:23, Fabrice wrote: > This can go away if you change the above comment. Done.
jdduke@chromium.org changed reviewers: + boliu@chromium.org
OK, rebased to the positional parameter revert. Nico, could you take a look? +boliu for android_webview/ review.
On 2015/02/06 17:05:02, jdduke wrote: > OK, rebased to the positional parameter revert. Nico, could you take a look? > > +boliu for android_webview/ review. lgtm
lgtm. Much excite! (cc hans update.sh change fyi) https://codereview.chromium.org/835633003/diff/440001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/835633003/diff/440001/build/common.gypi#newco... build/common.gypi:4662: '-latomic', (is this needed already?) https://codereview.chromium.org/835633003/diff/440001/build/config/compiler/B... File build/config/compiler/BUILD.gn (left): https://codereview.chromium.org/835633003/diff/440001/build/config/compiler/B... build/config/compiler/BUILD.gn:577: "gcc", where did this go? https://codereview.chromium.org/835633003/diff/440001/tools/android/run_pie/r... File tools/android/run_pie/run_pie.gyp (right): https://codereview.chromium.org/835633003/diff/440001/tools/android/run_pie/r... tools/android/run_pie/run_pie.gyp:21: # Don't inherit unneeded dependencies on stlport.so, so the binary remains "on libc++" I assume this file isn't used in the webview build.
LGTM thanks a lot! https://codereview.chromium.org/835633003/diff/440001/build/config/compiler/B... File build/config/compiler/BUILD.gn (left): https://codereview.chromium.org/835633003/diff/440001/build/config/compiler/B... build/config/compiler/BUILD.gn:577: "gcc", On 2015/02/06 17:17:19, Nico wrote: > where did this go? It's no longer needed. I'm the one who removed it. The GN version was badly out-of-date.
New patchsets have been uploaded after l-g-t-m from thakis@chromium.org,fdegans@chromium.org
https://codereview.chromium.org/835633003/diff/440001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/835633003/diff/440001/build/common.gypi#newco... build/common.gypi:4662: '-latomic', On 2015/02/06 17:17:19, Nico wrote: > (is this needed already?) I believe so, as base/atomicops.h switches on _LIBCPP_VERSION. https://codereview.chromium.org/835633003/diff/440001/tools/android/run_pie/r... File tools/android/run_pie/run_pie.gyp (right): https://codereview.chromium.org/835633003/diff/440001/tools/android/run_pie/r... tools/android/run_pie/run_pie.gyp:21: # Don't inherit unneeded dependencies on stlport.so, so the binary remains On 2015/02/06 17:17:19, Nico wrote: > "on libc++" Done. > > I assume this file isn't used in the webview build. Right, I believe it's only used for testing.
lgtm, ship it :-)
jdduke@chromium.org changed reviewers: + davidben@chromium.org, piman@chromium.org
+davidben@ for net/ +piman@ for third_party/mesa/
net lgtm!
jdduke@chromium.org changed reviewers: + cpu@chromium.org - piman@chromium.org
+cpu for third_party/.
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/835633003/460001
Message was sent while issue was closed.
Committed patchset #16 (id:460001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/8681920fa22ded465054301ce96657a4ddaf2a04 Cr-Commit-Position: refs/heads/master@{#315085}
Message was sent while issue was closed.
piman@chromium.org changed reviewers: + piman@chromium.org
Message was sent while issue was closed.
LGTM for mesa. Exciting!
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:460001) has been created in https://codereview.chromium.org/903323002/ by jam@chromium.org. The reason for reverting is: Slows down the slowest bot on CQ by 20 minutes. BUG=456396. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
