Issue 6531 - remove inspector and adapt for Android NDK r16b

[sergei, 2018-04-09 09:48:11]

AFAIR previous time it was landed without review, currently there is again the
time pressure, however, I think it is worth reviewing.

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
File gypfiles/standalone.gypi (left):

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
gypfiles/standalone.gypi:1093: 'CLANG_CXX_LANGUAGE_STANDARD': 'c++11',  #
-std=c++11
We are defining it in common.gypi and use '-std=c++14', the using of any kind 
of additionally defined C++ language standards complicates the debugging of the
building process and nobody knows what is actually used because they all are
added to the command line and sometimes with duplicates.

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
File gypfiles/standalone.gypi (right):

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
gypfiles/standalone.gypi:318: 'abi_target': 'i686-linux-android',
Some headers are located in <(android_sysroot)/usr/include/<(abi_target), and
there is no other definition of it, so I added them for all platforms mentioned
here. BTW we don't compile for mips(64), but I added it for the consistency.

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
gypfiles/standalone.gypi:384: 'android_libcpp_abi_include':
'<(android_stl)/llvm-libc++abi/include',
This conditional branch ('android_ndk_root==""') is not used but I edited it
merely for the sake of consistency.

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
gypfiles/standalone.gypi:390: 'android_sysroot': '<(android_ndk_root)/sysroot',
I had to split sysroot into two paths/variables. 'android_sysroot' is used to
compile the files because there are only headers, however the previous path
contains libraries and therefore it is used to link the binaries. The latter is
added below as 'android_platform_sysroot'. It's not in this 'variables' section
because when it's in that section it's not visible where it's used below.

https://codereview.adblockplus.org/29747555/diff/29747556/gypfiles/standalone...
gypfiles/standalone.gypi:402: 'android_libcpp_abi_include':
'<(android_stl)/llvm-libc++abi/include',
Some paths in Android NDK r16b are different, therefore 'libcxx' and libcxxabi
are moved.

https://codereview.adblockplus.org/29747555/diff/29747556/src/arm/simulator-a...
File src/arm/simulator-arm.h (right):

https://codereview.adblockplus.org/29747555/diff/29747556/src/arm/simulator-a...
src/arm/simulator-arm.h:216: int32_t CallFPImpl(byte* entry, double d0, double
d1);
Compare
https://chromium.googlesource.com/v8/v8.git/+/7fece33157c55b16decdf4cde199ca3...
https://chromium.googlesource.com/v8/v8.git/+/7fece33157c55b16decdf4cde199ca3...

However the compiler stumbles upon it, AFAIR intptr_t is defined as 64 bit or it
simply cannot convert one type (int32_t) to the type of pointer.

https://codereview.adblockplus.org/29747555/diff/29747556/src/v8.gyp
File src/v8.gyp (left):

https://codereview.adblockplus.org/29747555/diff/29747556/src/v8.gyp#oldcode40
src/v8.gyp:40: 'includes': ['../gypfiles/toolchain.gypi',
'../gypfiles/features.gypi', 'inspector/inspector.gypi'],
As in the previous changes
(https://github.com/adblockplus/v8-googlesource/commit/9a4991959e231351e52266b...)
we drop the inspector because it's not used.

The previous changes also contained the reordering of libraries for linking what
was the issue on linux to build mksnapshot for the host, however since we
reverted the workaround for the same issue in libadblockplus, that reordering is
not needed here anymore.

[hub, 2018-04-11 01:55:14]

https://codereview.adblockplus.org/29747555/diff/29747556/src/arm/simulator-a...
File src/arm/simulator-arm.h (right):

https://codereview.adblockplus.org/29747555/diff/29747556/src/arm/simulator-a...
src/arm/simulator-arm.h:216: int32_t CallFPImpl(byte* entry, double d0, double
d1);
On 2018/04/09 09:48:10, sergei wrote:
> Compare
>
https://chromium.googlesource.com/v8/v8.git/+/7fece33157c55b16decdf4cde199ca3...
>
https://chromium.googlesource.com/v8/v8.git/+/7fece33157c55b16decdf4cde199ca3...
> 
> However the compiler stumbles upon it, AFAIR intptr_t is defined as 64 bit or
it
> simply cannot convert one type (int32_t) to the type of pointer.

In master
https://chromium.googlesource.com/v8/v8.git/+/master/src/arm/simulator-arm.cc
the signature in the implementation is intptr_t. Maybe that's where it should be
changed instead?

[sergei, 2018-04-11 09:34:44]

https://codereview.adblockplus.org/29747555/diff/29747556/src/arm/simulator-a...
File src/arm/simulator-arm.h (right):

https://codereview.adblockplus.org/29747555/diff/29747556/src/arm/simulator-a...
src/arm/simulator-arm.h:216: int32_t CallFPImpl(byte* entry, double d0, double
d1);
On 2018/04/11 01:55:14, hub wrote:
> On 2018/04/09 09:48:10, sergei wrote:
> > Compare
> >
>
https://chromium.googlesource.com/v8/v8.git/+/7fece33157c55b16decdf4cde199ca3...
> >
>
https://chromium.googlesource.com/v8/v8.git/+/7fece33157c55b16decdf4cde199ca3...
> > 
> > However the compiler stumbles upon it, AFAIR intptr_t is defined as 64 bit
or
> it
> > simply cannot convert one type (int32_t) to the type of pointer.
> 
> In master
> https://chromium.googlesource.com/v8/v8.git/+/master/src/arm/simulator-arm.cc
> the signature in the implementation is intptr_t. Maybe that's where it should
be
> changed instead?

Done, perhaps it's better. When I was changing it I took a look at the return
value of `get_register` which return value is returned from this function.

[hub, 2018-04-11 09:44:31]

LGTM

[sergei, 2018-04-11 09:46:53]

I'm going to land it.

https://codereview.adblockplus.org/29747555/diff/29747556/src/v8.gyp
File src/v8.gyp (left):

https://codereview.adblockplus.org/29747555/diff/29747556/src/v8.gyp#oldcode40
src/v8.gyp:40: 'includes': ['../gypfiles/toolchain.gypi',
'../gypfiles/features.gypi', 'inspector/inspector.gypi'],
On 2018/04/09 09:48:10, sergei wrote:
> As in the previous changes
>
(https://github.com/adblockplus/v8-googlesource/commit/9a4991959e231351e52266b...)
> we drop the inspector because it's not used.

Forgot to mention that that inspector is not merely used, it even causes
compilation errors, so it's removed.