Catch intptr_t to int conversion on 64-bit platforms.

Catch intptr_t to int conversion on 64-bit platforms.

Change the JNI generator to introduce a wrapper type
jni_int_wrapper which is used to prevent implicit
conversions which could be an intptr_t to int
conversion.

Change also includes fix (adding explicit case) for
a false positive which existings in the code base.

BUG=354405, 346626
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268741

[bulach, 2014-05-02 16:55:56]

very nice, thanks anton! some small nits, and a question / suggestion to make
this debug / release only, let me know your thoughts!

https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:666: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
probably better to leave this as is?

https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
base/android/jni_generator/jni_generator.py:993: return param.name + ".asJint()"
if param.datatype == 'int' else param.name
how about we make the jni_generator take a debug option, and we'd only generate
this code for debug?
we'd need a specific test for whatever other mode we don't use as default... 
this extra indirection is really great, thanks for doing that! however, I think
that for such the mistakes at buildtime, it'll not be worth it in the final
product, wdyt?

nit: single-quote and c_style, s/".asJint()"/'.as_jint()'/

https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper.h
File base/android/jni_int_wrapper.h (right):

https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
base/android/jni_int_wrapper.h:16: // jint that could be consider anti-patterns,
such as from size_t.
I think we could do with a 
#ifdef NDEBUG 
// Don't add any indirection on release mode.
typedef jint JniIntWrapper;
#else
// On debug mode, use the extra indirection to capture
// the type-narrowing above.
#endi

https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
base/android/jni_int_wrapper.h:18: class jni_int_wrapper {
nit: should be JniIntWrapper

https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
base/android/jni_int_wrapper.h:24: jint asJint() const { return i_; }
nit: and here, as_jint()

[rmcilroy, 2014-05-02 17:21:21]

On 2014/05/02 16:55:56, bulach wrote:
> very nice, thanks anton! some small nits, and a question / suggestion to make
> this debug / release only, let me know your thoughts!
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> File base/android/jni_generator/jni_generator.py (right):
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> base/android/jni_generator/jni_generator.py:666: // Copyright (c) 2014 The
> Chromium Authors. All rights reserved.
> probably better to leave this as is?
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> base/android/jni_generator/jni_generator.py:993: return param.name +
".asJint()"
> if param.datatype == 'int' else param.name
> how about we make the jni_generator take a debug option, and we'd only
generate
> this code for debug?
> we'd need a specific test for whatever other mode we don't use as default... 
> this extra indirection is really great, thanks for doing that! however, I
think
> that for such the mistakes at buildtime, it'll not be worth it in the final
> product, wdyt?
> 
> nit: single-quote and c_style, s/".asJint()"/'.as_jint()'/
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper.h
> File base/android/jni_int_wrapper.h (right):
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> base/android/jni_int_wrapper.h:16: // jint that could be consider
anti-patterns,
> such as from size_t.
> I think we could do with a 
> #ifdef NDEBUG 
> // Don't add any indirection on release mode.
> typedef jint JniIntWrapper;
> #else
> // On debug mode, use the extra indirection to capture
> // the type-narrowing above.
> #endi
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> base/android/jni_int_wrapper.h:18: class jni_int_wrapper {
> nit: should be JniIntWrapper
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> base/android/jni_int_wrapper.h:24: jint asJint() const { return i_; }
> nit: and here, as_jint()

Nice! Thanks Anton.

Just wondering, will this catch incorrect return values too? E.g.:

  jint_wrapper someJNIMethod();
  ...
  NativeObject* obj = reinterpret_cast<intptr_t>(someJNIMethod());

It would be great if we could catch these too though I guess it would be more
tricky.

[rmcilroy, 2014-05-02 17:28:35]

On 2014/05/02 17:21:21, rmcilroy wrote:
> On 2014/05/02 16:55:56, bulach wrote:
> > very nice, thanks anton! some small nits, and a question / suggestion to
make
> > this debug / release only, let me know your thoughts!
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > File base/android/jni_generator/jni_generator.py (right):
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > base/android/jni_generator/jni_generator.py:666: // Copyright (c) 2014 The
> > Chromium Authors. All rights reserved.
> > probably better to leave this as is?
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > base/android/jni_generator/jni_generator.py:993: return param.name +
> ".asJint()"
> > if param.datatype == 'int' else param.name
> > how about we make the jni_generator take a debug option, and we'd only
> generate
> > this code for debug?
> > we'd need a specific test for whatever other mode we don't use as default...

> > this extra indirection is really great, thanks for doing that! however, I
> think
> > that for such the mistakes at buildtime, it'll not be worth it in the final
> > product, wdyt?
> > 
> > nit: single-quote and c_style, s/".asJint()"/'.as_jint()'/
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper.h
> > File base/android/jni_int_wrapper.h (right):
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > base/android/jni_int_wrapper.h:16: // jint that could be consider
> anti-patterns,
> > such as from size_t.
> > I think we could do with a 
> > #ifdef NDEBUG 
> > // Don't add any indirection on release mode.
> > typedef jint JniIntWrapper;
> > #else
> > // On debug mode, use the extra indirection to capture
> > // the type-narrowing above.
> > #endi
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > base/android/jni_int_wrapper.h:18: class jni_int_wrapper {
> > nit: should be JniIntWrapper
> > 
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > base/android/jni_int_wrapper.h:24: jint asJint() const { return i_; }
> > nit: and here, as_jint()
> 
> Nice! Thanks Anton.
> 
> Just wondering, will this catch incorrect return values too? E.g.:
> 
>   jint_wrapper someJNIMethod();
>   ...
>   NativeObject* obj = reinterpret_cast<intptr_t>(someJNIMethod());
> 
> It would be great if we could catch these too though I guess it would be more
> tricky.

p.s., I realise the above isn't a lose of precision, but it implies that
precision has been lost somewhere if the intention was that someJNIMethod
returns a native pointer.

[Anton, 2014-05-06 13:02:48]

On 2014/05/02 17:28:35, rmcilroy wrote:
> On 2014/05/02 17:21:21, rmcilroy wrote:
> > On 2014/05/02 16:55:56, bulach wrote:
> > > very nice, thanks anton! some small nits, and a question / suggestion to
> make
> > > this debug / release only, let me know your thoughts!
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > > File base/android/jni_generator/jni_generator.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > > base/android/jni_generator/jni_generator.py:666: // Copyright (c) 2014 The
> > > Chromium Authors. All rights reserved.
> > > probably better to leave this as is?
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > > base/android/jni_generator/jni_generator.py:993: return param.name +
> > ".asJint()"
> > > if param.datatype == 'int' else param.name
> > > how about we make the jni_generator take a debug option, and we'd only
> > generate
> > > this code for debug?
> > > we'd need a specific test for whatever other mode we don't use as
default...
> 
> > > this extra indirection is really great, thanks for doing that! however, I
> > think
> > > that for such the mistakes at buildtime, it'll not be worth it in the
final
> > > product, wdyt?
> > > 
> > > nit: single-quote and c_style, s/".asJint()"/'.as_jint()'/
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper.h
> > > File base/android/jni_int_wrapper.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > > base/android/jni_int_wrapper.h:16: // jint that could be consider
> > anti-patterns,
> > > such as from size_t.
> > > I think we could do with a 
> > > #ifdef NDEBUG 
> > > // Don't add any indirection on release mode.
> > > typedef jint JniIntWrapper;
> > > #else
> > > // On debug mode, use the extra indirection to capture
> > > // the type-narrowing above.
> > > #endi
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > > base/android/jni_int_wrapper.h:18: class jni_int_wrapper {
> > > nit: should be JniIntWrapper
> > > 
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > > base/android/jni_int_wrapper.h:24: jint asJint() const { return i_; }
> > > nit: and here, as_jint()
> > 
> > Nice! Thanks Anton.
> > 
> > Just wondering, will this catch incorrect return values too? E.g.:
> > 
> >   jint_wrapper someJNIMethod();
> >   ...
> >   NativeObject* obj = reinterpret_cast<intptr_t>(someJNIMethod());
> > 
> > It would be great if we could catch these too though I guess it would be
more
> > tricky.
> 
> p.s., I realise the above isn't a lose of precision, but it implies that
> precision has been lost somewhere if the intention was that someJNIMethod
> returns a native pointer.

I don't think that there is a problem in the back path. The example above
isn't compiling code, perhaps it should read:

   jint someJNIMethod();
   ...
   NativeObject* obj = reinterpret_cast<NativeObject*>(someJNIMethod());

Here the compiler will give you an error because reinterpret_cast<> requires
that
the objects are the same size. So I think that an error here requires multiple
mistakes in combination before you get passed the compiler.

[Anton, 2014-05-06 14:32:13]

On 2014/05/02 16:55:56, bulach wrote:
> very nice, thanks anton! some small nits, and a question / suggestion to make
> this debug / release only, let me know your thoughts!
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> File base/android/jni_generator/jni_generator.py (right):
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> base/android/jni_generator/jni_generator.py:666: // Copyright (c) 2014 The
> Chromium Authors. All rights reserved.
> probably better to leave this as is?
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> base/android/jni_generator/jni_generator.py:993: return param.name +
".asJint()"
> if param.datatype == 'int' else param.name
> how about we make the jni_generator take a debug option, and we'd only
generate
> this code for debug?
> we'd need a specific test for whatever other mode we don't use as default... 
> this extra indirection is really great, thanks for doing that! however, I
think
> that for such the mistakes at buildtime, it'll not be worth it in the final
> product, wdyt?
> 
> nit: single-quote and c_style, s/".asJint()"/'.as_jint()'/
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper.h
> File base/android/jni_int_wrapper.h (right):
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> base/android/jni_int_wrapper.h:16: // jint that could be consider
anti-patterns,
> such as from size_t.
> I think we could do with a 
> #ifdef NDEBUG 
> // Don't add any indirection on release mode.
> typedef jint JniIntWrapper;
> #else
> // On debug mode, use the extra indirection to capture
> // the type-narrowing above.
> #endi
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> base/android/jni_int_wrapper.h:18: class jni_int_wrapper {
> nit: should be JniIntWrapper
> 
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> base/android/jni_int_wrapper.h:24: jint asJint() const { return i_; }
> nit: and here, as_jint()

PTAL. I am doing something similar to your suggestion. I don't pass a flag to
the JNI generator. I just use the inlining in g++ compiler, which seems to get
rid of the trivial identity function which I use for as_jint in NDEBUG mode.

[bulach, 2014-05-06 14:57:54]

lgtm, I like it a lot, thanks!!! 
(and thanks to peter for the original insight as well :)

truly small nits below, and please let ross take a quick look too.

https://codereview.chromium.org/267893002/diff/20001/base/android/jni_generat...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/267893002/diff/20001/base/android/jni_generat...
base/android/jni_generator/jni_generator.py:993: return ("as_jint(" + param.name
+ ")"
nit: s/"/'/

https://codereview.chromium.org/267893002/diff/20001/base/android/jni_int_wra...
File base/android/jni_int_wrapper.h (right):

https://codereview.chromium.org/267893002/diff/20001/base/android/jni_int_wra...
base/android/jni_int_wrapper.h:24: inline jint as_jint(JniIntWrapper wrapper) {
const& ?

[Anton, 2014-05-06 15:04:50]

On 2014/05/06 14:57:54, bulach wrote:
> lgtm, I like it a lot, thanks!!! 
> (and thanks to peter for the original insight as well :)
> 
> truly small nits below, and please let ross take a quick look too.
> 
>
https://codereview.chromium.org/267893002/diff/20001/base/android/jni_generat...
> File base/android/jni_generator/jni_generator.py (right):
> 
>
https://codereview.chromium.org/267893002/diff/20001/base/android/jni_generat...
> base/android/jni_generator/jni_generator.py:993: return ("as_jint(" +
param.name
> + ")"
> nit: s/"/'/

Done. At Patch Set 3.

>
https://codereview.chromium.org/267893002/diff/20001/base/android/jni_int_wra...
> File base/android/jni_int_wrapper.h (right):
> 
>
https://codereview.chromium.org/267893002/diff/20001/base/android/jni_int_wra...
> base/android/jni_int_wrapper.h:24: inline jint as_jint(JniIntWrapper wrapper)
{
> const& ?

JniIntWrapper is an int at this point!

[rmcilroy, 2014-05-06 17:16:03]

On 2014/05/06 13:02:48, Anton wrote:
> On 2014/05/02 17:28:35, rmcilroy wrote:
> > On 2014/05/02 17:21:21, rmcilroy wrote:
> > > On 2014/05/02 16:55:56, bulach wrote:
> > > > very nice, thanks anton! some small nits, and a question / suggestion to
> > make
> > > > this debug / release only, let me know your thoughts!
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > > > File base/android/jni_generator/jni_generator.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > > > base/android/jni_generator/jni_generator.py:666: // Copyright (c) 2014
The
> > > > Chromium Authors. All rights reserved.
> > > > probably better to leave this as is?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_generator/j...
> > > > base/android/jni_generator/jni_generator.py:993: return param.name +
> > > ".asJint()"
> > > > if param.datatype == 'int' else param.name
> > > > how about we make the jni_generator take a debug option, and we'd only
> > > generate
> > > > this code for debug?
> > > > we'd need a specific test for whatever other mode we don't use as
> default...
> > 
> > > > this extra indirection is really great, thanks for doing that! however,
I
> > > think
> > > > that for such the mistakes at buildtime, it'll not be worth it in the
> final
> > > > product, wdyt?
> > > > 
> > > > nit: single-quote and c_style, s/".asJint()"/'.as_jint()'/
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper.h
> > > > File base/android/jni_int_wrapper.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > > > base/android/jni_int_wrapper.h:16: // jint that could be consider
> > > anti-patterns,
> > > > such as from size_t.
> > > > I think we could do with a 
> > > > #ifdef NDEBUG 
> > > > // Don't add any indirection on release mode.
> > > > typedef jint JniIntWrapper;
> > > > #else
> > > > // On debug mode, use the extra indirection to capture
> > > > // the type-narrowing above.
> > > > #endi
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > > > base/android/jni_int_wrapper.h:18: class jni_int_wrapper {
> > > > nit: should be JniIntWrapper
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/267893002/diff/1/base/android/jni_int_wrapper...
> > > > base/android/jni_int_wrapper.h:24: jint asJint() const { return i_; }
> > > > nit: and here, as_jint()
> > > 
> > > Nice! Thanks Anton.
> > > 
> > > Just wondering, will this catch incorrect return values too? E.g.:
> > > 
> > >   jint_wrapper someJNIMethod();
> > >   ...
> > >   NativeObject* obj = reinterpret_cast<intptr_t>(someJNIMethod());
> > > 
> > > It would be great if we could catch these too though I guess it would be
> more
> > > tricky.
> > 
> > p.s., I realise the above isn't a lose of precision, but it implies that
> > precision has been lost somewhere if the intention was that someJNIMethod
> > returns a native pointer.
> 
> I don't think that there is a problem in the back path. The example above
> isn't compiling code, perhaps it should read:
> 
>    jint someJNIMethod();
>    ...
>    NativeObject* obj = reinterpret_cast<NativeObject*>(someJNIMethod());
> 
> Here the compiler will give you an error because reinterpret_cast<> requires
> that
> the objects are the same size. So I think that an error here requires multiple
> mistakes in combination before you get passed the compiler.

That makes sense, thanks for the explanation.  Sounds good.

[Anton, 2014-05-06 17:17:20]

The CQ bit was checked by anton@chromium.org

[commit-bot: I haz the power, 2014-05-06 17:18:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/anton@chromium.org/267893002/40001

[commit-bot: I haz the power, 2014-05-07 09:35:20]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 14:14:19]

Change committed as 268741