[WIP NOT FOR COMMIT] Switch Android to libc++

[WIP NOT FOR COMMIT] Switch Android to libc++

BUG=427718

[pasko, 2014-10-30 15:20:29]

pasko@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2014-10-30 15:20:31]

needs some more cleanups anyway, so here are my not really useful notes

https://codereview.chromium.org/691783003/diff/20001/content/browser/android/...
File content/browser/android/content_view_statics.cc (right):

https://codereview.chromium.org/691783003/diff/20001/content/browser/android/...
content/browser/android/content_view_statics.cc:86: //    DCHECK_NE(pos,
suspended_processes_.end());
DCHECK(pos != suspended_processes_.end())

https://codereview.chromium.org/691783003/diff/20001/tools/android/run_pie/ru...
File tools/android/run_pie/run_pie.gyp (right):

https://codereview.chromium.org/691783003/diff/20001/tools/android/run_pie/ru...
tools/android/run_pie/run_pie.gyp:24: '-l<(android_libc++_library)',
++ in names of variables is always awesome

[Nico, 2014-10-30 17:02:40]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-10-30 17:02:41]

Awesome :-)

It looks like there are a few things that could be landed independently while
still on stlport (missing includes, maybe some of the s/begin/data/ changes),
I'd land those separate from the actual switch.

Looking forward to binary size / perf data :-)

https://codereview.chromium.org/691783003/diff/20001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/691783003/diff/20001/base/compiler_specific.h...
base/compiler_specific.h:10: #define typeof(x) __typeof__(x)
See http://crbug.com/427584 for this – we shouldn't use typeof.

[Fabrice (no longer in Chrome), 2014-10-30 17:32:53]

I cleaned up the patch a bit more.
If someone wants to try to understand why address_tracker_linux_unittest.cc
needs this change, be my guest. I'm going to guess libc++ has a specialization
for std::vector<char> that does not play nice with GCC, for some reason.

On 2014/10/30 17:02:41, Nico wrote:
> Awesome :-)
> 
> It looks like there are a few things that could be landed independently while
> still on stlport (missing includes, maybe some of the s/begin/data/ changes),
> I'd land those separate from the actual switch.

Unfortunately std::vector::data() is C++11 stuff that is not in stlport. The
alternative would be &my_vec[0] :/
The include stuff can go to separate CLs though.

https://codereview.chromium.org/691783003/diff/20001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/691783003/diff/20001/base/compiler_specific.h...
base/compiler_specific.h:10: #define typeof(x) __typeof__(x)
On 2014/10/30 17:02:41, Nico wrote:
> See http://crbug.com/427584 for this – we shouldn't use typeof.

Done.

https://codereview.chromium.org/691783003/diff/20001/content/browser/android/...
File content/browser/android/content_view_statics.cc (right):

https://codereview.chromium.org/691783003/diff/20001/content/browser/android/...
content/browser/android/content_view_statics.cc:86: //    DCHECK_NE(pos,
suspended_processes_.end());
On 2014/10/30 15:20:31, pasko wrote:
> DCHECK(pos != suspended_processes_.end())

Done.

[pasko, 2014-10-30 17:38:55]

On 2014/10/30 17:02:41, Nico wrote:
> Awesome :-)
> 
> It looks like there are a few things that could be landed independently while
> still on stlport (missing includes, maybe some of the s/begin/data/ changes),
> I'd land those separate from the actual switch.

+1 to land the easy parts asap (rebasing less is more!)

[Fabrice (no longer in Chrome), 2014-11-05 12:33:04]

fdegans@chromium.org changed reviewers:
+ torne@chromium.org

[Fabrice (no longer in Chrome), 2014-11-05 12:33:06]

Rebased following https://codereview.chromium.org/693773003/ and added gn
changes, found a bunch of stuff we shouldn't need anymore in the process. Looks
like the gn side is badly out-of-date.

I have questions (in-line) because I am not 100% sure about the non-Android
configuration and I think we can remove more hacks.

I noticed there are references to stlport in nacl but I do not know how to deal
with that or who to contact about this.

Regarding webview, +torne to explain how we are very likely going to break
everything.

https://codereview.chromium.org/691783003/diff/80001/base/containers/hash_tab...
File base/containers/hash_tables.h (right):

https://codereview.chromium.org/691783003/diff/80001/base/containers/hash_tab...
base/containers/hash_tables.h:36: #elif defined(COMPILER_GCC)
This essentially does a hack for pre-C++11 headers, which we now have everywhere
but on Mac (?), can we change this to test for OS_MACOSX instead?

https://codereview.chromium.org/691783003/diff/80001/build/config/android/con...
File build/config/android/config.gni (right):

https://codereview.chromium.org/691783003/diff/80001/build/config/android/con...
build/config/android/config.gni:126: # Libc++ stuff
---------------------------------------------------------------
Should we move this to build/compiler/BUILD.gn?
That's the only place where the variable is used. In fact, it wasn't even used
before this change.

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/util.h
File third_party/re2/util/util.h (right):

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/uti...
third_party/re2/util/util.h:50: !defined(_LIBCPP_ABI_VERSION)
So, essentially Mac, right?

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/uti...
third_party/re2/util/util.h:58: #if defined(WIN32)
I'm not sure that is still necessary, we've been using a C++11-compliant STL on
Windows for a while, right?

[Torne, 2014-11-05 12:37:19]

In principle webview should be fine, however I've just landed
https://codereview.chromium.org/705753002/ which does the exact opposite of this
CL: *forces* using libstdc++ on host instead of libc++, and on mac hosts the
libstdc++ is the one from gcc 4.2 which definitely doesn't have any C++11
features :(

If we want to use libc++ for host binaries we have to do some gross rpath
hackery when linking the binaries because we build host executables to the wrong
location in the gyp android backend for gross reasons. If we can get away
without using it for host binaries for now then this should be fine as long as
the android_aosp bot is green.

[pasko, 2014-11-05 13:02:45]

Nico, are you going to work out a way to build on Mac with libc++ soon? Maybe
after migration to x86-64-only? What are the plans?

[Fabrice (no longer in Chrome), 2014-11-05 13:13:04]

On 2014/11/05 12:37:19, Torne wrote:
> In principle webview should be fine, however I've just landed
> https://codereview.chromium.org/705753002/ which does the exact opposite of
this
> CL: *forces* using libstdc++ on host instead of libc++, and on mac hosts the
> libstdc++ is the one from gcc 4.2 which definitely doesn't have any C++11
> features :(
> 
> If we want to use libc++ for host binaries we have to do some gross rpath
> hackery when linking the binaries because we build host executables to the
wrong
> location in the gyp android backend for gross reasons. If we can get away
> without using it for host binaries for now then this should be fine as long as
> the android_aosp bot is green.

Looking at the code changes in the CL, I do not think it should break anything
if it is only the host binaries. And even if it does, the Chromium mac builders
should pick it in the try bots.

[Torne, 2014-11-05 13:56:00]

On 2014/11/05 13:13:04, Fabrice de Gans wrote:
> On 2014/11/05 12:37:19, Torne wrote:
> > In principle webview should be fine, however I've just landed
> > https://codereview.chromium.org/705753002/ which does the exact opposite of
> this
> > CL: *forces* using libstdc++ on host instead of libc++, and on mac hosts the
> > libstdc++ is the one from gcc 4.2 which definitely doesn't have any C++11
> > features :(
> > 
> > If we want to use libc++ for host binaries we have to do some gross rpath
> > hackery when linking the binaries because we build host executables to the
> wrong
> > location in the gyp android backend for gross reasons. If we can get away
> > without using it for host binaries for now then this should be fine as long
as
> > the android_aosp bot is green.
> 
> Looking at the code changes in the CL, I do not think it should break anything
> if it is only the host binaries. And even if it does, the Chromium mac
builders
> should pick it in the try bots.

If the chromium build on mac is still using apple's libstdc++ then that will
serve to catch any non-android-specific problems here, yes.

[Nico, 2014-11-05 17:19:28]

On Wed, Nov 5, 2014 at 5:02 AM, <pasko@chromium.org> wrote:

> Nico, are you going to work out a way to build on Mac with libc++ soon?
> Maybe
> after migration to x86-64-only? What are the plans?
>

I'll look at it eventually. Chrome/Android seemed like longer lead time, so
I wanted to get this started first. Chrome/Mac will also require rpath
hackery; if this is all it takes to get webview host targets to libc++
maybe I can look at that at the same time.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-11-05 17:30:10]

https://codereview.chromium.org/691783003/diff/80001/base/containers/hash_tab...
File base/containers/hash_tables.h (right):

https://codereview.chromium.org/691783003/diff/80001/base/containers/hash_tab...
base/containers/hash_tables.h:36: #elif defined(COMPILER_GCC)
On 2014/11/05 12:33:05, Fabrice de Gans wrote:
> This essentially does a hack for pre-C++11 headers, which we now have
everywhere
> but on Mac (?),

And webview host, apparently. base/ is built as host target.

> can we change this to test for OS_MACOSX instead?

I'd prefer to get working c++11 libraries everywhere first, and then do global
cleanups (like this) once we have that.

https://codereview.chromium.org/691783003/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/691783003/diff/80001/build/common.gypi#newcod...
build/common.gypi:3920: ],
Why do you need this? Everything should already build with this flag, I think.

https://codereview.chromium.org/691783003/diff/80001/build/config/compiler/BU...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/691783003/diff/80001/build/config/compiler/BU...
build/config/compiler/BUILD.gn:475: cflags_cc = [ "-std=gnu++11" ]
Same question as in the gyp file

https://codereview.chromium.org/691783003/diff/80001/net/base/address_tracker...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/691783003/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux_unittest.cc:145: std::vector<int8_t> buffer_;
I'm guessing this is because Buffer is a vector<char> typedef, and char is
unsigned on android. But looking through the class, I can't see where this is
important and I see a few places where char*s are inserted to the buffer.

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/util.h
File third_party/re2/util/util.h (right):

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/uti...
third_party/re2/util/util.h:58: #if defined(WIN32)
On 2014/11/05 12:33:06, Fabrice de Gans wrote:
> I'm not sure that is still necessary, we've been using a C++11-compliant STL
on
> Windows for a while, right?

Yes, but re2 is a third_party project and re2 can't assume this. We eventually
want to reduce our downstream diff for re2 to 0 and pull re2 via deps.

[Nico, 2014-11-07 00:56:39]

https://codereview.chromium.org/691783003/diff/80001/net/base/address_tracker...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/691783003/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux_unittest.cc:145: std::vector<int8_t> buffer_;
On 2014/11/05 17:30:10, Nico wrote:
> I'm guessing this is because Buffer is a vector<char> typedef, and char is
> unsigned on android. But looking through the class, I can't see where this is
> important and I see a few places where char*s are inserted to the buffer.

(this wasn't answered)

https://codereview.chromium.org/691783003/diff/80001/net/spdy/spdy_prefixed_b...
File net/spdy/spdy_prefixed_buffer_reader_test.cc (right):

https://codereview.chromium.org/691783003/diff/80001/net/spdy/spdy_prefixed_b...
net/spdy/spdy_prefixed_buffer_reader_test.cc:35: //EXPECT_THAT(buffer_copy,
ElementsAreArray("foobar"));
Is this the only thing that's keeping this from being ready for review for real?

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/util.h
File third_party/re2/util/util.h (right):

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/uti...
third_party/re2/util/util.h:49: #if defined(__GNUC__) && !defined(USE_CXX0X) &&
!defined(OS_ANDROID) && \
I think the OS_ANDROID change here can be undone too.

I think you can revert all the changes in
third_party/re2/patches/re2-android.patch and delete that file (and update
third_party/re2/README.chromium to remove the mention of that patch)

[pasko, 2014-11-07 11:05:52]

https://codereview.chromium.org/691783003/diff/80001/net/spdy/spdy_prefixed_b...
File net/spdy/spdy_prefixed_buffer_reader_test.cc (right):

https://codereview.chromium.org/691783003/diff/80001/net/spdy/spdy_prefixed_b...
net/spdy/spdy_prefixed_buffer_reader_test.cc:35: //EXPECT_THAT(buffer_copy,
ElementsAreArray("foobar"));
On 2014/11/07 00:56:39, Nico wrote:
> Is this the only thing that's keeping this from being ready for review for
real?

there are a few other issues afair, Fabrice pointed me yesterday at some tests
crashing caused by printf using positional parameters, not being supported
properly for some reason.

Can we have 'use_custom_libcxx' to guard the choice of the libc++ for now? It
seems there will be more issues to discover, solving them with targeted patches
one by one sounds like an easier way forward (despite more gyp conditioning).
WDYT?

fyi: Fabrice will be OOO till next Wednesday.

[Nico, 2014-11-07 16:37:01]

https://codereview.chromium.org/691783003/diff/80001/net/spdy/spdy_prefixed_b...
File net/spdy/spdy_prefixed_buffer_reader_test.cc (right):

https://codereview.chromium.org/691783003/diff/80001/net/spdy/spdy_prefixed_b...
net/spdy/spdy_prefixed_buffer_reader_test.cc:35: //EXPECT_THAT(buffer_copy,
ElementsAreArray("foobar"));
On 2014/11/07 11:05:52, pasko wrote:
> On 2014/11/07 00:56:39, Nico wrote:
> > Is this the only thing that's keeping this from being ready for review for
> real?
> 
> there are a few other issues afair, Fabrice pointed me yesterday at some tests
> crashing caused by printf using positional parameters, not being supported
> properly for some reason.

:-(

If you could extract an example of this, we can fix this upstream in the ndk's
support library.

> Can we have 'use_custom_libcxx' to guard the choice of the libc++ for now? It
> seems there will be more issues to discover, solving them with targeted
patches
> one by one sounds like an easier way forward (despite more gyp conditioning).
> WDYT?

'use_custom_libcxx' is used for a custom libcxx bundled with chromium, for asan.
I'd have a new 'android_use_libcxx' instead. But having some gyp setting for a
while to make the transition easier sounds fine to me.

[Fabrice (no longer in Chrome), 2014-11-12 11:52:30]

I copied the comments to the latest patch set.

I am trying to figure out what is wrong with positional parameters. It fails
differently with Clang and GCC but in both cases it's a case of the wrong
argument being used, which leads me to believe there's a stack corruption
involved somewhere.

https://codereview.chromium.org/691783003/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/691783003/diff/80001/build/common.gypi#newcod...
build/common.gypi:3920: ],
On 2014/11/05 17:30:10, Nico wrote:
> Why do you need this? Everything should already build with this flag, I think.

Done.

https://codereview.chromium.org/691783003/diff/80001/build/config/compiler/BU...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/691783003/diff/80001/build/config/compiler/BU...
build/config/compiler/BUILD.gn:475: cflags_cc = [ "-std=gnu++11" ]
On 2014/11/05 17:30:10, Nico wrote:
> Same question as in the gyp file

Done.

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/util.h
File third_party/re2/util/util.h (right):

https://codereview.chromium.org/691783003/diff/80001/third_party/re2/util/uti...
third_party/re2/util/util.h:49: #if defined(__GNUC__) && !defined(USE_CXX0X) &&
!defined(OS_ANDROID) && \
On 2014/11/07 00:56:39, Nico wrote:
> I think the OS_ANDROID change here can be undone too.
> 
> I think you can revert all the changes in
> third_party/re2/patches/re2-android.patch and delete that file (and update
> third_party/re2/README.chromium to remove the mention of that patch)

Done.

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux_unittest.cc:125: output->insert(output->end(),
buffer_.begin(), buffer_.end());
That's the line that fails to compile with buffer_ defined with type Buffer. It
only fails with GCC, it's fine with Clang.

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux_unittest.cc:145: std::vector<int8_t> buffer_;
On 2014/11/07 00:56:39, Nico wrote:
> On 2014/11/05 17:30:10, Nico wrote:
> > I'm guessing this is because Buffer is a vector<char> typedef, and char is
> > unsigned on android. But looking through the class, I can't see where this
is
> > important and I see a few places where char*s are inserted to the buffer.
> 
> (this wasn't answered)

Sorry, I missed it. This only fails with GCC, Clang compiles it fine. I have
added a comment on the line failing to compile.

https://codereview.chromium.org/691783003/diff/120001/net/spdy/spdy_prefixed_...
File net/spdy/spdy_prefixed_buffer_reader_test.cc (right):

https://codereview.chromium.org/691783003/diff/120001/net/spdy/spdy_prefixed_...
net/spdy/spdy_prefixed_buffer_reader_test.cc:35: //EXPECT_THAT(buffer_copy,
ElementsAreArray("foobar"));
On 2014/11/07 16:37:01, Nico wrote:
> On 2014/11/07 11:05:52, pasko wrote:
> > On 2014/11/07 00:56:39, Nico wrote:
> > > Is this the only thing that's keeping this from being ready for review for
> > real?
> > 
> > there are a few other issues afair, Fabrice pointed me yesterday at some
tests
> > crashing caused by printf using positional parameters, not being supported
> > properly for some reason.
> 
> :-(
> 
> If you could extract an example of this, we can fix this upstream in the ndk's
> support library.
> 

I have been trying to do that but I think there is a stack corruption involved.
On the other hand, it is "just" a failing test, I haven't been able to find a
single use case for positional parameters in the whole codebase.

> > Can we have 'use_custom_libcxx' to guard the choice of the libc++ for now?
It
> > seems there will be more issues to discover, solving them with targeted
> patches
> > one by one sounds like an easier way forward (despite more gyp
conditioning).
> > WDYT?
> 
> 'use_custom_libcxx' is used for a custom libcxx bundled with chromium, for
asan.
> I'd have a new 'android_use_libcxx' instead. But having some gyp setting for a
> while to make the transition easier sounds fine to me.

We'd need to add a define for the transition period. Most of the changes here
are breaking changes. I think it's too much hassle for this.

[pasko, 2014-11-12 12:52:52]

https://codereview.chromium.org/691783003/diff/120001/net/spdy/spdy_prefixed_...
File net/spdy/spdy_prefixed_buffer_reader_test.cc (right):

https://codereview.chromium.org/691783003/diff/120001/net/spdy/spdy_prefixed_...
net/spdy/spdy_prefixed_buffer_reader_test.cc:35: //EXPECT_THAT(buffer_copy,
ElementsAreArray("foobar"));
On 2014/11/12 11:52:30, Fabrice wrote:
> On 2014/11/07 16:37:01, Nico wrote:
> > On 2014/11/07 11:05:52, pasko wrote:
> > > On 2014/11/07 00:56:39, Nico wrote:
> > > > Is this the only thing that's keeping this from being ready for review
for
> > > real?
> > > 
> > > there are a few other issues afair, Fabrice pointed me yesterday at some
> tests
> > > crashing caused by printf using positional parameters, not being supported
> > > properly for some reason.
> > 
> > :-(
> > 
> > If you could extract an example of this, we can fix this upstream in the
ndk's
> > support library.
> > 
> 
> I have been trying to do that but I think there is a stack corruption
involved.
> On the other hand, it is "just" a failing test, I haven't been able to find a
> single use case for positional parameters in the whole codebase.
> 
> > > Can we have 'use_custom_libcxx' to guard the choice of the libc++ for now?
> It
> > > seems there will be more issues to discover, solving them with targeted
> > patches
> > > one by one sounds like an easier way forward (despite more gyp
> conditioning).
> > > WDYT?
> > 
> > 'use_custom_libcxx' is used for a custom libcxx bundled with chromium, for
> asan.
> > I'd have a new 'android_use_libcxx' instead. But having some gyp setting for
a
> > while to make the transition easier sounds fine to me.
> 
> We'd need to add a define for the transition period. Most of the changes here
> are breaking changes. I think it's too much hassle for this.

can you file a bug and just do:
#if defined(ANDROID_LIBCXX) // not sure the correct macro
#undef EXPECT_THAT
#define EXPECT_THAT(a, b)
#endif

[Nico, 2014-11-13 06:21:12]

It'd be good to understand what's going on with the spdy tests. (Funnily, they
were also problematic for clang/win: http://llvm.org/bugs/show_bug.cgi?id=21520
but that was a windows-specific thing)

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux_unittest.cc:125: output->insert(output->end(),
buffer_.begin(), buffer_.end());
On 2014/11/12 11:52:29, Fabrice wrote:
> That's the line that fails to compile with buffer_ defined with type Buffer.
It
> only fails with GCC, it's fine with Clang.

Bizarre, output and buffer_ have the same type. Oh, is it because this is a
const method and so the buffer_ begin / end() methods return const iterators?
…no, insert() is supposed to be templated on the type of the 2nd and 3rd
parameter. What's the error message?

[Fabrice (no longer in Chrome), 2014-11-13 10:52:07]

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux_unittest.cc:125: output->insert(output->end(),
buffer_.begin(), buffer_.end());
On 2014/11/13 06:21:12, Nico wrote:
> On 2014/11/12 11:52:29, Fabrice wrote:
> > That's the line that fails to compile with buffer_ defined with type Buffer.
> It
> > only fails with GCC, it's fine with Clang.
> 
> Bizarre, output and buffer_ have the same type. Oh, is it because this is a
> const method and so the buffer_ begin / end() methods return const iterators?
> …no, insert() is supposed to be templated on the type of the 2nd and 3rd
> parameter. What's the error message?

Turns out the issue is the same in both tests. I have managed to track it down
to what I think is a bug in GCC but I can't manage to reproduce consistently on
a smaller case.
Here's the error message:
[...]/ndk/sources/cxx-stl/llvm-libc++/libcxx/include/iterator:1232:31: error:
'std::__1::__wrap_iter<_Iter>::__wrap_iter(std::__1::__wrap_iter<_Iter>::iterator_type)
[with _Iter = const char*; std::__1::__wrap_iter<_Iter>::iterator_type = const
char*]' is private
     _LIBCPP_INLINE_VISIBILITY __wrap_iter(iterator_type __x) _NOEXCEPT :
__i(__x) {}
It is a private constructor used by the vector class. The __wrap_iter class
declares vector class as friend but for some reason it is ignored by GCC. I have
managed to "reproduce" the error message with Clang by commenting the friend
declaration in the iterator header.

[pasko, 2014-11-13 12:20:12]

On 2014/11/13 10:52:07, Fabrice wrote:
>
https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
> File net/base/address_tracker_linux_unittest.cc (right):
> 
>
https://codereview.chromium.org/691783003/diff/120001/net/base/address_tracke...
> net/base/address_tracker_linux_unittest.cc:125: output->insert(output->end(),
> buffer_.begin(), buffer_.end());
> On 2014/11/13 06:21:12, Nico wrote:
> > On 2014/11/12 11:52:29, Fabrice wrote:
> > > That's the line that fails to compile with buffer_ defined with type
Buffer.
> > It
> > > only fails with GCC, it's fine with Clang.
> > 
> > Bizarre, output and buffer_ have the same type. Oh, is it because this is a
> > const method and so the buffer_ begin / end() methods return const
iterators?
> > …no, insert() is supposed to be templated on the type of the 2nd and 3rd
> > parameter. What's the error message?
> 
> Turns out the issue is the same in both tests. I have managed to track it down
> to what I think is a bug in GCC but I can't manage to reproduce consistently
on
> a smaller case.

Then the GCC folks will be quite happy with preprocessed source.

> Here's the error message:
> [...]/ndk/sources/cxx-stl/llvm-libc++/libcxx/include/iterator:1232:31: error:
>
'std::__1::__wrap_iter<_Iter>::__wrap_iter(std::__1::__wrap_iter<_Iter>::iterator_type)
> [with _Iter = const char*; std::__1::__wrap_iter<_Iter>::iterator_type = const
> char*]' is private
>      _LIBCPP_INLINE_VISIBILITY __wrap_iter(iterator_type __x) _NOEXCEPT :
> __i(__x) {}
> It is a private constructor used by the vector class. The __wrap_iter class
> declares vector class as friend but for some reason it is ignored by GCC. I
have
> managed to "reproduce" the error message with Clang by commenting the friend
> declaration in the iterator header.

[jdduke (slow), 2014-12-16 01:36:39]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-12-16 01:36:39]

What else needs to happen here? Are there components of this change that that we
can start pushing forward? Does the latest 10d NDK release
(https://developer.android.com/tools/sdk/ndk/index.html#) change the landscape
in any fashion?

[Fabrice (no longer in Chrome), 2014-12-16 12:18:54]

On 2014/12/16 01:36:39, jdduke wrote:
> What else needs to happen here? Are there components of this change that that
we
> can start pushing forward? Does the latest 10d NDK release
> (https://developer.android.com/tools/sdk/ndk/index.html) change the landscape
> in any fashion?

At this point, we're mostly blocked on b/18366844. There is another test failure
but it has to do with positional parameters in the printf family of functions,
which are not used in the Chromium codebase.
Then, we need to do some benchmarking for size and speed.

[jdduke (slow), 2015-01-06 21:32:53]

On 2014/12/16 12:18:54, Fabrice (ooo until Jan 5) wrote:
> On 2014/12/16 01:36:39, jdduke wrote:
> > What else needs to happen here? Are there components of this change that
that
> we
> > can start pushing forward? Does the latest 10d NDK release
> > (https://developer.android.com/tools/sdk/ndk/index.html) change the
landscape
> > in any fashion?
> 
> At this point, we're mostly blocked on b/18366844. There is another test
failure
> but it has to do with positional parameters in the printf family of functions,
> which are not used in the Chromium codebase.
> Then, we need to do some benchmarking for size and speed.

Can I ask why we're blocked on b/18366844? As far as I can tell, the compilation
failure in address_tracker_linux_unittest.cc is the only such instance, and
there are a good number of workarounds (see my rebase of this patch at
https://codereview.chromium.org/835633003/). We definitely want the underlying
issue resolved going forward, but it would be a shame for that bug to block
progress when, at the moment, it's quite a minor hiccup in the codebase.

[pasko, 2015-01-13 13:01:15]

On 2015/01/06 21:32:53, jdduke wrote:
> On 2014/12/16 12:18:54, Fabrice (ooo until Jan 5) wrote:
> > On 2014/12/16 01:36:39, jdduke wrote:
> > > What else needs to happen here? Are there components of this change that
> that
> > we
> > > can start pushing forward? Does the latest 10d NDK release
> > > (https://developer.android.com/tools/sdk/ndk/index.html) change the
> landscape
> > > in any fashion?
> > 
> > At this point, we're mostly blocked on b/18366844. There is another test
> failure
> > but it has to do with positional parameters in the printf family of
functions,
> > which are not used in the Chromium codebase.
> > Then, we need to do some benchmarking for size and speed.
> 
> Can I ask why we're blocked on b/18366844? As far as I can tell, the
compilation
> failure in address_tracker_linux_unittest.cc is the only such instance, and
> there are a good number of workarounds (see my rebase of this patch at
> https://codereview.chromium.org/835633003/). We definitely want the underlying
> issue resolved going forward, but it would be a shame for that bug to block
> progress when, at the moment, it's quite a minor hiccup in the codebase.

the workaround sgtm, we should not be blocked on it, thank you, Jared

[jdduke (slow), 2015-01-14 01:47:40]

On 2015/01/13 13:01:15, pasko wrote:
> On 2015/01/06 21:32:53, jdduke wrote:
> > On 2014/12/16 12:18:54, Fabrice (ooo until Jan 5) wrote:
> > > On 2014/12/16 01:36:39, jdduke wrote:
> > > > What else needs to happen here? Are there components of this change that
> > that
> > > we
> > > > can start pushing forward? Does the latest 10d NDK release
> > > > (https://developer.android.com/tools/sdk/ndk/index.html) change the
> > landscape
> > > > in any fashion?
> > > 
> > > At this point, we're mostly blocked on b/18366844. There is another test
> > failure
> > > but it has to do with positional parameters in the printf family of
> functions,
> > > which are not used in the Chromium codebase.
> > > Then, we need to do some benchmarking for size and speed.
> > 
> > Can I ask why we're blocked on b/18366844? As far as I can tell, the
> compilation
> > failure in address_tracker_linux_unittest.cc is the only such instance, and
> > there are a good number of workarounds (see my rebase of this patch at
> > https://codereview.chromium.org/835633003/). We definitely want the
underlying
> > issue resolved going forward, but it would be a shame for that bug to block
> > progress when, at the moment, it's quite a minor hiccup in the codebase.
> 
> the workaround sgtm, we should not be blocked on it, thank you, Jared

FWIW, the ARC folks ran into some similar issues with positional parameters on
Android, though I'm not sure if it's related:
https://code.google.com/p/chromium/issues/detail?id=248851