You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2018/03/29 22:27:05 UTC
Warnings from the bindings
Philip Martin <ph...@codematters.co.uk> writes:
> +1 to release
The swig-pl tests produce warnings for:
Warning: unable to close filehandle $out_fh properly: Bad file descriptor at /home/pm/sw/subversion/sign/build/subversion/bindings/swig/perl/native/blib/lib/SVN/Core.pm line 58.
Core.pm:58 is a call to apr_terminate() so perhaps pool cleanup is
attempting to close a file that has been already closed.
The JavaHL tests produce warnings for:
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod
but it's not clear to me which bit of source code is responsible. There
are also warnings for:
WARNING: JNI local refs: 33, exceeds capacity: 32
at org.apache.subversion.javahl.util.PropLib.unparseExternals(Native Method)
at org.apache.subversion.javahl.SVNUtil.unparseExternals(SVNUtil.java:790)
--
Philip
Re: Warnings from the bindings
Posted by Branko Čibej <br...@apache.org>.
On 07.04.2018 21:20, Philip Martin wrote:
> Branko Čibej <br...@apache.org> writes:
>
>> I'll take another look. At the very least this should be using the local
>> frame manager object, which is exception-safe, unlike explicitly pushing
>> and popping the frame.
> ImmutableList::for_each iterates and constructs arbitrary types T. The
> problem occurs when T is ExternalItem as the constructor generates local
> refs from the get_string_field and get_revision_field calls. It's
> possible that we should modify ExternalItem to avoid doing this, but
> for_each should also work with arbitrary types and so for_each should
> use a local frame. I've comitted the for_each change.
Looks good, thanks!
-- Brane
Re: Warnings from the bindings
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@apache.org> writes:
> I'll take another look. At the very least this should be using the local
> frame manager object, which is exception-safe, unlike explicitly pushing
> and popping the frame.
ImmutableList::for_each iterates and constructs arbitrary types T. The
problem occurs when T is ExternalItem as the constructor generates local
refs from the get_string_field and get_revision_field calls. It's
possible that we should modify ExternalItem to avoid doing this, but
for_each should also work with arbitrary types and so for_each should
use a local frame. I've comitted the for_each change.
--
Philip
Re: Warnings from the bindings
Posted by Branko Čibej <br...@apache.org>.
On 07.04.2018 00:25, Philip Martin wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>> Iterator iter(get_iterator());
>> while (iter.has_next())
>> + {
>> + m_env.PushLocalFrame(10);
>> function(T(m_env, NativeT(iter.next())));
>> + m_env.PopLocalFrame();
>> + }
>> return function;
> Rewriting as:
>
> while (iter.has_next())
> {
> NativeT nt(iter.next());
> m_env.PushLocalFrame(10);
> T t(m_env, nt);
> function(t);
> m_env.PopLocalFrame();
> }
>
> also removes the warning, but if I move the T construction before the
> PushLocalframe then the warning returns. T is JavaHL:ExternalItem.
I'll take another look. At the very least this should be using the local
frame manager object, which is exception-safe, unlike explicitly pushing
and popping the frame.
-- Brane
Re: Warnings from the bindings
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> Iterator iter(get_iterator());
> while (iter.has_next())
> + {
> + m_env.PushLocalFrame(10);
> function(T(m_env, NativeT(iter.next())));
> + m_env.PopLocalFrame();
> + }
> return function;
Rewriting as:
while (iter.has_next())
{
NativeT nt(iter.next());
m_env.PushLocalFrame(10);
T t(m_env, nt);
function(t);
m_env.PopLocalFrame();
}
also removes the warning, but if I move the T construction before the
PushLocalframe then the warning returns. T is JavaHL:ExternalItem.
--
Philip
Re: Warnings from the bindings
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> Branko Čibej <br...@apache.org> writes:
>
>> And I don't see these, either. I checked the code and we explicitly use
>> a JNI local frame in the core loop of unparseExternals. Try as I might,
>> I can't see where all those local references could be coming from.
>
> ImmutableList::for_each appears to be the problem. The following ugly
> hack is not correct but does resolve the warning:
That is more complicated than necessary. iter.has_next is not the
problem, just the function call. Probably this is still fixing the
symptom rather than the underlying problem:
Index: subversion/bindings/javahl/native/jniwrapper/jni_list.hpp
===================================================================
--- subversion/bindings/javahl/native/jniwrapper/jni_list.hpp (revision 1828477)
+++ subversion/bindings/javahl/native/jniwrapper/jni_list.hpp (working copy)
@@ -173,7 +173,11 @@ class ImmutableList : public BaseImmutableList
{
Iterator iter(get_iterator());
while (iter.has_next())
+ {
+ m_env.PushLocalFrame(10);
function(T(m_env, NativeT(iter.next())));
+ m_env.PopLocalFrame();
+ }
return function;
}
};
--
Philip
Re: Warnings from the bindings
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@apache.org> writes:
> And I don't see these, either. I checked the code and we explicitly use
> a JNI local frame in the core loop of unparseExternals. Try as I might,
> I can't see where all those local references could be coming from.
ImmutableList::for_each appears to be the problem. The following ugly
hack is not correct but does resolve the warning:
Index: subversion/bindings/javahl/native/jniwrapper/jni_list.hpp
===================================================================
--- subversion/bindings/javahl/native/jniwrapper/jni_list.hpp (revision 1828477)
+++ subversion/bindings/javahl/native/jniwrapper/jni_list.hpp (working copy)
@@ -172,8 +172,18 @@ class ImmutableList : public BaseImmutableList
F for_each(F function) const
{
Iterator iter(get_iterator());
- while (iter.has_next())
+ while (true)
+ {
+ m_env.PushLocalFrame(10);
+ bool has_next = iter.has_next();
+ if (!has_next)
+ {
+ m_env.PopLocalFrame();
+ break;
+ }
function(T(m_env, NativeT(iter.next())));
+ m_env.PopLocalFrame();
+ }
return function;
}
};
--
Philip
Re: Warnings from the bindings
Posted by Branko Čibej <br...@apache.org>.
On 03.04.2018 13:28, Stefan Fuhrmann wrote:
> On 30.03.2018 00:27, Philip Martin wrote:
>
>> WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod
> Seen these in alpha2 but not in rc2. Java8 has
> been updated since then.
I don't see this on OSX with 1.8.0_45-b14.
>> but it's not clear to me which bit of source code is responsible. There
>> are also warnings for:
>>
>> WARNING: JNI local refs: 33, exceeds capacity: 32
>> at org.apache.subversion.javahl.util.PropLib.unparseExternals(Native Method)
>> at org.apache.subversion.javahl.SVNUtil.unparseExternals(SVNUtil.java:790)
> Those are a known issue. There used to be 10-ish
> such warnings and now they are down to 3. I vaguely
> recall that there had been an actual fix and we are
> now seeing the remaining unpatched occurrences.
And I don't see these, either. I checked the code and we explicitly use
a JNI local frame in the core loop of unparseExternals. Try as I might,
I can't see where all those local references could be coming from.
-- Brane
Re: Warnings from the bindings
Posted by Stefan Fuhrmann <st...@apache.org>.
On 30.03.2018 00:27, Philip Martin wrote:
> Philip Martin<ph...@codematters.co.uk> writes:
>
>> +1 to release
> The swig-pl tests produce warnings for:
>
> Warning: unable to close filehandle $out_fh properly: Bad file descriptor at /home/pm/sw/subversion/sign/build/subversion/bindings/swig/perl/native/blib/lib/SVN/Core.pm line 58.
FWIW, I see the same on Ubuntu 16.04.4 and
they have been present in alpha2 already.
> Core.pm:58 is a call to apr_terminate() so perhaps pool cleanup is
> attempting to close a file that has been already closed.
>
> The JavaHL tests produce warnings for:
>
> WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod
Seen these in alpha2 but not in rc2. Java8 has
been updated since then.
> but it's not clear to me which bit of source code is responsible. There
> are also warnings for:
>
> WARNING: JNI local refs: 33, exceeds capacity: 32
> at org.apache.subversion.javahl.util.PropLib.unparseExternals(Native Method)
> at org.apache.subversion.javahl.SVNUtil.unparseExternals(SVNUtil.java:790)
Those are a known issue. There used to be 10-ish
such warnings and now they are down to 3. I vaguely
recall that there had been an actual fix and we are
now seeing the remaining unpatched occurrences.
-- Stefan^2.
Re: Warnings from the bindings
Posted by Philip Martin <ph...@codematters.co.uk>.
Stefan Sperling <st...@apache.org> writes:
> On Fri, Mar 30, 2018 at 09:29:47AM +0200, Branko Čibej wrote:
>> I've never seen either of these warnings and I don't see what could've
>> changed on the 1.10.x branch to make them suddenly start appearing.
>
> The Perl one happens on OpenBSD as well. It is not new. I don't exactly
> know when it started but I believe it started before 1.10 was branched.
Yes, I didn't mean to imply that any of these are new. It's possible
the JavaHL ones started when I switched from Java 8 to Java 9.
--
Philip
Re: Warnings from the bindings
Posted by Stefan Sperling <st...@apache.org>.
On Fri, Mar 30, 2018 at 09:29:47AM +0200, Branko Čibej wrote:
> On 30.03.2018 00:27, Philip Martin wrote:
> > Philip Martin <ph...@codematters.co.uk> writes:
> >
> >> +1 to release
> > The swig-pl tests produce warnings for:
> >
> > Warning: unable to close filehandle $out_fh properly: Bad file descriptor at /home/pm/sw/subversion/sign/build/subversion/bindings/swig/perl/native/blib/lib/SVN/Core.pm line 58.
> >
> > Core.pm:58 is a call to apr_terminate() so perhaps pool cleanup is
> > attempting to close a file that has been already closed.
> >
> > The JavaHL tests produce warnings for:
> >
> > WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod
> >
> > but it's not clear to me which bit of source code is responsible. There
> > are also warnings for:
> >
> > WARNING: JNI local refs: 33, exceeds capacity: 32
> > at org.apache.subversion.javahl.util.PropLib.unparseExternals(Native Method)
> > at org.apache.subversion.javahl.SVNUtil.unparseExternals(SVNUtil.java:790)
>
> I've never seen either of these warnings and I don't see what could've
> changed on the 1.10.x branch to make them suddenly start appearing.
The Perl one happens on OpenBSD as well. It is not new. I don't exactly
know when it started but I believe it started before 1.10 was branched.
https://ci.apache.org/builders/svn-bb-openbsd/builds/62/steps/Test%20bindings/logs/testlog-swig-pl
JavaHL doesn't even build on OpenBSD anymore these days, so no idea.
(I don't think anybody would need JavaHL on this platform.)
Re: Warnings from the bindings
Posted by James McCoy <ja...@jamessan.com>.
On Fri, Mar 30, 2018 at 09:29:47AM +0200, Branko Čibej wrote:
> On 30.03.2018 00:27, Philip Martin wrote:
> > Philip Martin <ph...@codematters.co.uk> writes:
> >
> >> +1 to release
> > The swig-pl tests produce warnings for:
> >
> > Warning: unable to close filehandle $out_fh properly: Bad file descriptor at /home/pm/sw/subversion/sign/build/subversion/bindings/swig/perl/native/blib/lib/SVN/Core.pm line 58.
> >
> > Core.pm:58 is a call to apr_terminate() so perhaps pool cleanup is
> > attempting to close a file that has been already closed.
> >
> > The JavaHL tests produce warnings for:
> >
> > WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod
> >
> > but it's not clear to me which bit of source code is responsible. There
> > are also warnings for:
> >
> > WARNING: JNI local refs: 33, exceeds capacity: 32
> > at org.apache.subversion.javahl.util.PropLib.unparseExternals(Native Method)
> > at org.apache.subversion.javahl.SVNUtil.unparseExternals(SVNUtil.java:790)
>
> I've never seen either of these warnings and I don't see what could've
> changed on the 1.10.x branch to make them suddenly start appearing.
They're not new. They've been happening for the past 2 years in Debian.
It looks like they popped up when we started building with Java 8.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: Warnings from the bindings
Posted by Branko Čibej <br...@apache.org>.
On 30.03.2018 00:27, Philip Martin wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>> +1 to release
> The swig-pl tests produce warnings for:
>
> Warning: unable to close filehandle $out_fh properly: Bad file descriptor at /home/pm/sw/subversion/sign/build/subversion/bindings/swig/perl/native/blib/lib/SVN/Core.pm line 58.
>
> Core.pm:58 is a call to apr_terminate() so perhaps pool cleanup is
> attempting to close a file that has been already closed.
>
> The JavaHL tests produce warnings for:
>
> WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethod
>
> but it's not clear to me which bit of source code is responsible. There
> are also warnings for:
>
> WARNING: JNI local refs: 33, exceeds capacity: 32
> at org.apache.subversion.javahl.util.PropLib.unparseExternals(Native Method)
> at org.apache.subversion.javahl.SVNUtil.unparseExternals(SVNUtil.java:790)
I've never seen either of these warnings and I don't see what could've
changed on the 1.10.x branch to make them suddenly start appearing.
-- Brane
Re: Warnings from the bindings
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> WARNING in native method: JNI call made without checking exceptions
> when required to from CallStaticObjectMethod
>
> but it's not clear to me which bit of source code is responsible.
This might be a false alarm. From
https://github.com/dropbox/djinni/issues/152#issuecomment-157220326
if I do
echo 'public class Foo { public static void main(String[] args) throws Exception { } }' > Foo.java
javac Foo.java
java -Xcheck:jni Foo
I see the same warning as above.
--
Philip