Re: Patch: Remove exception catching from normal path in Arrays.equals.

[Andrew Haley <aph at redhat dot com>]

Mohan Embar writes:
 > Hi Andrew,
 > 
 > > >  > While we're on this subject, there are myriad places (natClass.cc,
 > > >  > natRuntime.cc, natArray.cc, natConstructor.cc, natField.cc, natMethod.cc,
 > > >  > natResourceBundle.cc) where gnu.gcj.StackTrace is being used
 > > >  > like this (example from natArray.cc):
 > > >  > 
 > > >  > try
 > > >  > {
 > > >  >   for (int i = 1; !caller; i++)
 > > >  >     {
 > > >  >       caller = t->classAt (i);
 > > >  >     }
 > > >  >   caller_loader = caller->getClassLoaderInternal();
 > > >  > }
 > > >  > catch (::java::lang::ArrayIndexOutOfBoundsException *e)
 > > >  > {
 > > >  > }
 > > >  
 > > > Note that the catch clause will not be invoked unless there is a bug in gcj.
 > >
 > >... so the last part should perhaps read
 > >
 > >catch (::java::lang::ArrayIndexOutOfBoundsException *e)
 > >{
 > >  throw new RuntimeError (e);
 > >}
 > 
 > ...with Ranjit's initial cross-configury patch and also for cygwin gcj,
 > the backtrace() function was non-existent and stacktraces where therefore
 > broken. The ArrayIndexOutOfBoundsException was eaten alive, causing
 > null pointer problems down the road. Throwing a RuntimeError would be
 > better.
 > 
 > I'm probably belaboring something not worth belaboring, but it seems
 > the right thing to do to refactor this into a helper function in
 > StackTrace.

True.  I guess any time you see code repeated so much it deserves to
be encapsulated in a method.

 > In this way, we can implement the correct behavior only once. (And
 > I agree that findCallerClass() is better than
 > tryTofindFirstNonNullClassStartingAtIndex() :). However, this may
 > not be worth our time given that it's a fringe case and you said
 > this code is going to be revised.

Yes.  This is critical to access control, and we need to be very
careful.

 > Let me know if you want me to submit a patch. Otherwise, I'll walk
 > away from this.

Sure, go ahead.  I'm a bit nervous about the throw new RuntimeError
part because some broken libgcj targets still don't have working
backtrace.  Perhaps it needs to be conditional on HAVE_BACKTRACE.

Andrew.