United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-6472714 crash compiling com.sun.jndi.ldap.sasl.LdapSasl::saslBind
JDK-6472714 : crash compiling com.sun.jndi.ldap.sasl.LdapSasl::saslBind

Details
Type:
Bug
Submit Date:
2006-09-19
Status:
Resolved
Updated Date:
2010-05-08
Project Name:
JDK
Resolved Date:
2006-11-14
Component:
hotspot
OS:
other
Sub-Component:
compiler
CPU:
generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
5.0u8
Fixed Versions:
hs10 (b03)

Related Reports
Backport:
Backport:
Backport:
Relates:

Sub Tasks

Description
Problem is observed on 1.5.0.08.FCS

Encountered a compiler crash, while compiling following method:
com.sun.jndi.ldap.sasl.LdapSasl::saslBind(Lcom/sun/jndi/ldap/LdapClient;
Lcom/sun/jndi/ldap/Connection;Ljava/lang/String;Ljava/lang/String;
Ljava/lang/Object;Ljava/lang/String;Ljava/util/Hashtable;
[Ljavax/naming/ldap/Control;)Lcom/sun/jndi/ldap/LdapResult; (437 bytes)

Source Code Looks like:
------------------------
     74     public static LdapResult saslBind(LdapClient clnt,
Connection conn,
     75         String server, String dn, Object pw,
     76         String authMech, Hashtable env, Control[] bindCtls)
     77         throws IOException, NamingException {
     78
     79         SaslClient saslClnt = null;
     80         boolean cleanupHandler = false;
     81
     82         // Use supplied callback handler or create default
     83         CallbackHandler cbh =
     84             (env != null) ?
(CallbackHandler)env.get(SASL_CALLBACK) : null;
     85         if (cbh == null) {
     86             cbh = new DefaultCallbackHandler(dn, pw,
(String)env.get(SASL_REALM));
     87             cleanupHandler = true;
     88         }
     89
     90         // Prepare parameters for creating SASL client
     91         String authzId = (env != null) ?
(String)env.get(SASL_AUTHZ_ID) : null;
     92         String[] mechs = getSaslMechanismNames(authMech);
     93   . . . . .
     94   Continues on  . . . .
     95   . . . . .

Inlining Information:
---------------------
      @ 49   com.sun.jndi.ldap.sasl.DefaultCallbackHandler::<init>
inline (hot)
        @ 1   java.lang.Object::<init>  inline (hot)
        @ 26   java.lang.String::toCharArray  inline (hot)
          @ 15   java.lang.String::getChars0  inline (hot)
  Inlining intrinsic static void java.lang.System.arraycopy(jobject,
jint, jobject, jint, jint)
        @ 82   java.lang.String::<init>  inline (hot)
          @ 6   java.lang.String::<init>  inline (hot)
            @ 1   java.lang.Object::<init>  inline (hot)
            @ 22   java.lang.String::checkBounds  inline (hot)
  Inlining intrinsic virtual jint java.lang.String.compareTo(jobject)
  Inlining intrinsic virtual jint java.lang.String.compareTo(jobject)
            @ 89   java.lang.StringCoding::decode  hot method too big
        @ 90   java.lang.String::toCharArray  inline (hot)
          @ 15   java.lang.String::getChars0  inline (hot)
  Inlining intrinsic static void java.lang.System.arraycopy(jobject,
jint, jobject, jint, jint)
      @ 80   com.sun.jndi.ldap.sasl.LdapSasl::getSaslMechanismNames
inline (hot)
. . . . . .
Continues on  . . . .
. . . . . . .


Note: We were able to see this level of inlining, because of our internal
enhancement enabled through option XX:+OptimisticInlining. It purpose
is to
Force inlining call-sites regardless its call profile. If you are
interested,
we can share the source changes. But I have provided the calling sequence,
for reference also.

Problem is observed during the optimizations of if-block, from line
85 to 88 in the Java source. I have attached a hand drawn picture of
the IR after parsing.

A Very Low-Level And Detailed Background:
-----------------------------------------
In the graph RegionNode (#124) is like you are at line no 85 in
Java Source. I noticed, that the CheckCastPPNode (# 117) is getting
input from CastPP, i.e. we are feeding a non-null value. Hence
effectively, we may say that, if the value of env is not null,
then we are feeding a non-null value for cbh. As incase,
return value of CallDynamicJava( # 48) was null we will hit
an uncommon-trap. This looked like was part of an optimization
that can be disabled using option XX-UncommonNullCast. By
default UncommonNullCast is enabled. The sub-graph between
IfNode (# 72) and RegionNode (# 117) was generated by
GraphKit::gen_checkcast().

Now, after looking at the usage of CheckCastPP and RegionNode
(#124), We can say that, control will never reach IfNode (#132)
if the return value of CallDynamicJava(#48) is null
(due to uncommon trap). In other words, IfNode (# 132) was
only reachable from IfTrue(#45), which implies that value of
env has to be null.

This became more clear after first pass of PhaseIdealLoop.
I noticed that the IfNode (# 132), that is control dependent
on Region (124) was split-up. In Fig-2, I have this sub-graph
created, that reflects the changes made after if-splitting.
After attempting to split-up IfNode( #132), the new If nodes
produced, no longer are getting inputs from BoolNode, rather
are being seeded with a constant-one and constant-zero,
respectively. In this figure, we see that the second merge
point of RegionNode( #20834) is a dead path, and that is
because of the following input order.
IfNode( Bool( Cmp ( Phi ( NULL, (CheckCastPP (CastPP)) ), NULL) ) )
Because of CastPP, the type of CheckCastPP will be NOTNULL

All that said, it becomes more evident that the receiver of
CallDynamicJava (#194) will be null and we will always go through
the exception path and hence fall-through path will be dead.

Gist Of the problem:
---------------------
During PhaseIdealLoop, and particularly during if-splitting, we
added couple of new nodes in the IGVN-worklist, which includes
RegionNode (# 20834), PhiNode(#20840), etc. At end of
PhaseIdealLoop, when we process the IGVN worklist, we recognized,
that _in[2] of Region(#20834) was dead, and hence we also change
the _in[2] of the PhiNode(# 20840) to TOP. This Phi will again
be transformed to a ConPNode, which is equivalent to NULL. This
changed the inputs of CallDynamicJava(#194) and it got placed
onto the worklist, and was left without any further transformation.
At this point, we have identified a potential dead path, but
is left un-noticed. Ideally, To me it seems more reasonable,
to do the best effort, to cleanup all the graph, during the
same IGVN pass, based on all the information we have gathered
so far. CatchNode::Value function does the trick of recognizing
such CallNodes and if the receiver is null, it appropriately
sets the type of its projection, but we never get to process
the associated CachNode during IGVN, and the whole sub-graph
under the fall-through projection remained in the IR without
being detected as dead, during the currently executed pass
of IGVN.


What happened next was:
------------------------
Next point when this dead path is recognized will be during
PhaseCCP. In this phase as we seem to do a global top-down
traversal and evaluate the types of each reachable node. Any
node that was not reachable during PhaseCCP::analyze() is
considered dead. Later on during PhaseCCP transformation
(traversing bottom-up) we start to remove/subsume any dead
node as we encounter. In order to remove dead nodes we seem
to make use of function remove_globally_dead_node(). I
think we could still have managed to remove this dead sub-graph.
In this function we check if after disconnecting input edge
of a "dead" node, there is no use of "in" node, we consider
"in" to be also dead. But we seem to take no action, if
in->outcount() != 0 and the type is TOP. In my case, "dead"
node was a CFG node and while looking at the nodes merging at
this dead-CFG-node, one of the input edges were comming from a
If-projection i.e. IfFalseNode, (which is also a loop exit point).
If the other If-projection i.e. IfTrueNode is a backedge, we will
have a situation where we will end up removing the exit path of
the loop, but the LoopEndNode which is also known to be dead, will
encounter no action. And hence are left with a subgraph, rather
a loop region, with no exit path.


During my investigation, I noticed this un-deleted loop
first. I made small change, to fix to ensure that the
un-reachible graph is properly deleted. I made a small change,
but later on considered that as a workaround.

What I did was:
Instead of leaving this case un-noticed, I push this "in"
node on to the IGVN worklist. (Ref: See my
workaround ahead)
Here was the change I made to see the impact.

void PhaseIterGVN::remove_globally_dead_node( Node *dead ) {
  assert(dead != C->root(), "killing root, eh?");
  if (dead->is_top())  return;
  NOT_PRODUCT( set_progress(); )
  // Remove from iterative worklist
  _worklist.remove(dead);
  // Remove from hash table
  _table.hash_delete( dead );
  // Smash all inputs to 'dead', isolating him completely
  for( uint i = 0; i < dead->req(); i++ ) {
    Node *in = dead->in(i);
    if( in ) {                 // Points to something?
      dead->set_req(i,NULL);  // Kill the edge
      if (in->outcnt() == 0 && in != C->top()) {// Made input go dead?
        remove_dead_node(in); // Recursively remove
#ifdef HP
      // Ref: JAGag01006 / D98
      // Without this, a potential dead path merging into the current dead
      // node may not be identified.
      } else if (in->outcnt() != 0 && type_or_null(in) == Type::TOP) {
        _worklist.push(in);
#endif
      } else if (in->outcnt() <= 2 && dead->is_Phi()) {
        if( in->Opcode() == Op_Region )
          _worklist.push(in);
        else if( in->is_Store() ) {
          DUIterator_Fast imax, i = in->fast_outs(imax);
          _worklist.push(in->fast_out(i));
          i++;
          if(in->outcnt() == 2) {
            _worklist.push(in->fast_out(i));
            i++;
          }
          assert(!(i < imax), "sanity");
        }
      }
    }
  }

  // Aggressively kill globally dead uses
  // (Cannot use DUIterator_Last because of the indefinite number
  // of edge deletions per loop trip.)
  while (dead->outcnt() > 0) {
    remove_globally_dead_node(dead->raw_out(0));
  }
}

Remarks
-------
The change I made, will not remove the node rightaway. Instead I
consider it a dead-candidate, and push it for IGVN processeing.
since the analysis is monotonic, once a fixed-point is reached,
any further attempt to transform a node should be a NOP.
That said, I also think, that this still looked like a workaround
to some uncaught problem. Infact that was indeed the case, as I
had already left a dead fall-through-path undetected during an
earlier IGVN pass.

                                    

Comments
EVALUATION

I think, this problem is the same as one described by Cliff in 5030922.
                                     
2006-09-27
SUGGESTED FIX

Here is the fix based on Cliff's and John's suggestions described in 5030922.
This changes are for 1.5.0_11 (the same as 1.5.0_08) sources.

*** /tmp/vertlCEAKyaO.T	Wed Sep 27 11:16:30 2006
--- /export/home2/work/2142125/src/share/vm/opto/phaseX.cpp	Wed Sep 27 11:19:17 2006
***************
*** 1118,1125 ****
      Node* use = n->fast_out(i); // Get use
  
      if( use->is_Multi() ||      // Multi-definer?  Push projs on worklist
!         use->is_Store() )       // Enable store/load same address
        add_users_to_worklist0(use); 
  
      if( use->is_Cmp() ) {       // Enable CMP/BOOL optimization
        add_users_to_worklist(use); // Put Bool on worklist
--- 1118,1146 ----
      Node* use = n->fast_out(i); // Get use
  
      if( use->is_Multi() ||      // Multi-definer?  Push projs on worklist
!         use->is_Store() ) {     // Enable store/load same address
        add_users_to_worklist0(use); 
+       // If we changed the reciever type to a call, we need to revisit
+       // the Catch following the call.  It's looking for a non-NULL
+       // receiver to know when to enable the regular fall-through path
+       // in addition to the NullPtrException path
+       CallNode *call = use->is_Call();
+       if (call != NULL && call->is_CallDynamicJava() && n == call->in(TypeFunc::Parms)) {
+         for (DUIterator_Fast i2max, i2 = call->fast_outs(i2max); i2 < i2max; i2++) {
+           ProjNode* p = call->fast_out(i2)->is_Proj();
+           if (p != NULL && p->_con == TypeFunc::Control) {
+             for (DUIterator_Fast i3max, i3 = p->fast_outs(i3max); i3 < i3max; i3++) {
+               Node* c = p->fast_out(i3);
+               if (c->is_Catch()) {
+                 assert(c->in(1) != NULL && c->in(1)->is_Proj(), "sanity");
+                 assert(c->in(1)->in(0) == use, "sanity");
+                 _worklist.push(c);  // Push on worklist
+               }
+             }
+           }
+         }
+       }
+     }
  
      if( use->is_Cmp() ) {       // Enable CMP/BOOL optimization
        add_users_to_worklist(use); // Put Bool on worklist
                                     
2006-09-27
EVALUATION

Here is John's comment from 5030922 (I modified it):

A change to NULL in a call's receiver can cause a change to a 
CatchNode's type, thus making a CatchProjNode go dead.  The logic to do 
this is in CatchNode::Value.  Thus, the CatchNode must be put onto the 
work list if the receiver of a dynamic call changes (at least, to null). 
  Note that extra-long series of links that CatchNode::Value traverses 
in order to examine the receiver:

   Catch -> Proj#1 -> DynamicCall -> Con#NULL

When the receiver changes, then its DynamicCall user gets put on the 
worklist.  At that point the following code runs (from phaseX.cpp):

     if( use->is_Multi() ||      // Multi-definer?  Push projs on worklist
         use->is_Store() )       // Enable store/load same address
       add_users_to_worklist0(use);

Since the use is a DynamicCall, it is also a MultiNode, and so its 
userse (which are all projections) get stuck on the worklist too.  This 
means the Proj#1 gets on.  However, because (I think) of a slight design 
weakness in the graph schema, that Proj#1 is a placeholder, whose type 
does not represent all the distinct states that are interesting to the 
optimizer.  The Catch node which uses it is not placed on the worklist, 
and the type of the Proj#1 (I_O projection) does not change, so nothing 
further happens to clue the Catch node that something has changed that 
it wants to hear about.
                                     
2006-10-02
SUGGESTED FIX

The fix is to put the following Catch node onto the IGVN worklist in
add_users_to_worklist() when the receiver of a CallDynamicJava
is changing.

Webrev:                 http://analemma.sfbay.sun.com/net/prt-archiver.sfbay/data/archived_workspaces/main/c2_baseline/2006/20061003105310.kvn.6472714/workspace/webrevs/webrev-2006.10.03/index.html
                                     
2006-10-03



Hardware and Software, Engineered to Work Together