WI #2039 Compute cyclic call using transitive closure.

[mayanje]

What have been done

We noticed that all cyclic perform call chains were not detected by the curent implementation: a -> b -> c -> a was not detected.
Thus as we know all direct calls, that is to say we know: a -> b, b -> c and c -> a
The idea here is to compute the transitive closure of all known direct calls.
So we compute the transitive closure of the direct call relation which is know as a transitive relation: a->b and b->c implies a->c
During the transitive closure computation, we compute the call chains of cycles in parallel.

[fm-117]

I disagree with this change, sentences is an accumulator list given by the caller of AccumulateSentencesThrough. The purpose of the method is to fill this list so it does not make sense to allow a null reference for this arg.

[mayanje]

done in 9bf7eb1

[fm-117]

Naming: what is a 'CALL relation' ? This is not a good naming choice because it looks like it refers to the Cobol CALL instruction but here we are dealing with PERFORMs only.

What is the first element in the tuple stored for each procedure in the dictionary ? If it is the caller of the said procedure then there is a problem because there can be multiple PERFORMs calling the same procedure in the whole program. Also, the second item of the tuple should be a list of PerformProcedure (not Node).

I think this method can be greatly simplified. Instead of doing the whole procedure resolution process again (finding the procedures targeted in a perform and the associated sentences) we could simply attach the resolved procedures and the list of sentences directly into BasicBlockForNodeGroup instance, at resolve time in ResolvePendingPERFORMProcedure. And then go over the list of groups to build the 'CALL relation' dictionary. The current procedure can also be attached to a Sentence instance very easily at build time so no need to compute it again.

[mayanje]

done in 9bf7eb1

[fm-117]

Naming: AddToRelation, first parameter should be called simply procedure to avoid conflict with calleeProcedure and second parameter can be removed as the local function ccan capture the callee parameter.

[mayanje]

do,e in c5f891a

[fm-117]

This is an Add operation so better use callRelation.Add(caller, callees);

[mayanje]

done in c5f891a

[fm-117]

This is a HashSet, simply call Add.

[mayanje]

don in c5f891a

[fm-117]

This part (Line 1253 to 1262) can now be removed ?

[mayanje]

Yes done in d80af72

[fm-117]

Not needed, store the current procedure inside each Sentence instance at build time.

[mayanje]

done in 9bf7eb1

[fm-117]

Could you explain the algorithm ? What does the N dictionary contain ? What does "an entry point of a calculated component" mean ?

[mayanje]

Ok this algorithm is not my invention, it is based on J. Eve, R. Kurki-Suonio transitive closure calculation algorithm, you can google on it, there are many adaptation. I implemented also an adaptation for gathering perform chain path.
I added few comments on it in 9bf7eb1

[fm-117]

Do we need it ? What about this much simpler algorithm ?

        public static Dictionary<N, HashSet<N>> TransitiveClosure<N>(Dictionary<N, HashSet<N>> graph)
        {
            var transitiveClosure = new Dictionary<N, HashSet<N>>();
            var root = graph.FirstOrDefault().Key;
            if (root != null)
            {
                ComputeDirectAndIndirectSuccessors(root);
            }
            return transitiveClosure;

            HashSet<N> ComputeDirectAndIndirectSuccessors(N node)
            {
                if (transitiveClosure.TryGetValue(node, out var result))
                {
                    return result;
                }

                var directSuccessors = graph[node];
                result = new HashSet<N>(directSuccessors);
                transitiveClosure.Add(node, result);

                foreach (var directSuccessor in directSuccessors)
                {
                    foreach (var indirectSuccessor in ComputeDirectAndIndirectSuccessors(directSuccessor))
                    {
                        result.Add(indirectSuccessor);
                    }
                }

                return result;
            }
        }

[mayanje]

Ok here is the url of the Eve paper: http://i.stanford.edu/pub/cstr/reports/cs/tr/75/508/CS-TR-75-508.pdf
It seems to me that the algorithm that you suggest is in the best case in O(n^2) and in the worst case in O(n^4) because it can implies 4 foreach loops.
In the paper it is say that computing transitive closure is in the best case be in O(n^2) and in the worst case in O(n^3), and it is said in the paper that the algorithm in the paper usually works in O(n) to O(n^2) where n is the number of vertices.

And I don't see in your algorithm where to compute perform call path chain.

[fm-117]

Yes it was an attempt at writing a simpler algorithm to help understanding the transitive closure. Anyway my code does not work properly for cyclic graphs so we can forget about it.

However the main question still remains, do we need to compute the full closure ? Our goal is to gather cyclic chains but not necessarily building the transitive closure of the perform call graph.

[mayanje]

My answer is yes we need to compute the full transitive closure, it is this computation that discovers Strongly connected components (that is to say cyclic chains).

[fm-117]

You can remove private setters, they are redundant

[fm-117]

naming, this is more like GetPerformTarget since we do not compute the target each time the method is called.

[mayanje]

done in 4b969b9

[fm-117]

Java indent style

[mayanje]

done in 4b969b9

[fm-117]

Typo: PerformTarget class

[mayanje]

done in 4b969b9

[fm-117]

This remark goes for all the return null; instructions in this method. We should actually store the null value inside the dictionary to prevent attempting resolving the target multiple times.
Even better: we should create a special instance Unresolved with empty sentences and procedures to use whenever a PerformProcedure cannot have its target properly resolved. This way the method would always return a non-null value.

[mayanje]

I would say this an abnormal situation it should never happened, let's take it in account with an assert.

[mayanje]

In fact calling an inexisting procedure is a semantic error, so no need to assert here, and simply return null here without storing such error value.

[fm-117]

It is a semantic error but the CFG builder may run on semantically wrong programs. CFG building happens during the Node phase so at this point we may have unresolved procedures. Storing a null PerformTarget in the cache for such cases is required to avoid trying the resolution multiple times. The null-object pattern is useful for the callers of the method which won't have to bother with testing for null value but it is optional.

[mayanje]

It is not a good practice to store null values for semantic error reason.

[fm-117]

No need to track the current procedure using the n index. The Procedure property of each Sentence object can (and should) be set at construction time. See my previous remark in this PR to see how to do it.

[mayanje]

done in 4b969b9

[fm-117]

Java indent style ?

[mayanje]

done in 4b969b9

[fm-117]

Should be read-only, set in constructor

[mayanje]

It is set in PerformTarget

[mayanje]

right done in 4b969b9

[fm-117]

Strange the file is generated in French now ?

[mayanje]

Maybe it's because I have a french version of VS2019

[fm-117]

List<Node> can be turned into List<PerformProcedure> if i recall correctly.

[mayanje]

I would like, but it's because of ControlFlowGraph<N, D>.AddRecursivePerform(PerformProcedure perform, List recursiveJump)

[smedilol]

This last version of the code without Tuple is much clearer.
Can you use more meaningful name for variables?
Like procedure or at least proc instead of a for the name of the procedure.
Same remarks for variables : d, fa, b, nb, ...

[mayanje]

Done in 35d0734

[smedilol]

With my previous remarks on meaningful variable name, you could inline variable:

                            if (Visited(b)== 0)

[mayanje]

Done in 35d0734

[fm-117]

It is a semantic error but the CFG builder may run on semantically wrong programs. CFG building happens during the Node phase so at this point we may have unresolved procedures. Storing a null PerformTarget in the cache for such cases is required to avoid trying the resolution multiple times. The null-object pattern is useful for the callers of the method which won't have to bother with testing for null value but it is optional.

[fm-117]

As we have now a new mechanism to detect recursive PERFORMs, we can remove the RecursivityGroupSet property from BasicBlockForNodeGroup class. I've checked the code, it is now written only, never read.

[mayanje]

Done in 35d0734

[fm-117]

There is no need to check for the existence of the perform node entry in the dictionary now, we can call Add each time. The second parameter should be renamed also (but I can't find a proper name yet, it is not the cycle, it's all the perform reached through the one passed as first arg).

[mayanje]

Done in 35d0734

[fm-117]

Now that I read it again, sentence.Procedure should never be null since we have a default root section.

[mayanje]

Yep, there is a bug in presence of DECLARATIVE:
#2081

[fm-117]

Parameter is not used, remove it please.

[mayanje]

Done in 35d0734

[fm-117]

The null value should be stored here also.

[mayanje]

done in 49d4035

[fm-117]

And here too

[mayanje]

done in 49d4035

[smedilol]

The inheritance from Dictionary disturbs me.
You are not extending the concept of Dictionary here but rather you want to use a Dictionary to store some information.

[mayanje]

It is not an extension of the concept of Dictionary because Generic Type Variables <TKey, TValue> of the Dictionary base class are instantiated with <Procedure, PerformCallChain>types.

[smedilol]

Does it have to be in the class ControlFlowGraphBuilder ?
It seems to me that the transitive closure mechanism could be put aside the ControlFlowGraphBuilder.
I mean it can be called by ControlFlowGraphBuilder, but the mechanism could also be called later. Once the graph is fully calculated.

[mayanje]

I think yes because only ControlFlowGraphBuilder knows how to discover direct perfrom calls. Once the graph is fully calculated direct perform calls context will be difficult to reproduce without retraversing the graph and analyzing each block to look for performs and perform targets..

[smedilol]

Suggested change

	foreach (var sentence in procedure)
	{
	sentences.Add(sentence);
	}
	sentences.AddRange(procedure);

[mayanje]

Yes done in 99deb05

[smedilol]

If I understand correctly a BasicBlockForNodeGroup is only used for Perform.
Such block always contains only 1 instruction which is the PerformProcedure.
If this is correct, it'd be easier to understand this by creating a getter in BasicBlockForNodeGroup to retrieve the PerformProcedure.
This way, you don't have to understand how BasicBlockForNodeGroup is implemented to retrieve the PerformProcedure.

So this code in this class would become:

Suggested change

	AddDirectCallPerformRelation(p, currentProcedure, (PerformProcedure)group0.Instructions.Last.Value, performCallRelation);
	AddDirectCallPerformRelation(p, currentProcedure, block.PerformProcedure, performCallRelation);

[mayanje]

Yes but this is a specialized knowledge whereas BasicBlockForNodeGroup is for a general purpose.

[smedilol]

It works but it seems we don't realy use the full capabilities of the CFG here. There is no use of SuccessorEdges.

Plus this algorithm only focus on cyclic perform call.
Why don't we try to detect all cyclic code? Like this:

        procedure division.
        a.
            go to b.
        b.
            perform a.

There is also PR #2050 currently in progress with almost the same algorithm.
Why not use the DFS in this other PR to detect every cycle ?
If our internal quality tools only want to report perform cycle as error, then we can filter the result.
And we can report other cycles (cycles not only with perform) as warning as a bonus for developers.

[mayanje]

Yes this will be an improvement because IBM compiler reports these warnings in such situation:

12  IGYCB7309-W   There may be a loop from the "PERFORM" statement at "PERFORM (line 12.1)" to itself. 
12  IGYCB7310-W   The "PERFORM" statement at "PERFORM (line 12.1)" cannot reach its exit.

But it seems that qualis does not report such situation also.

But our parser report a CFG error like this:
--- Diagnostics ---
Line 12[13,21] <37, Warning, General> - Warning: Statement ' go to b' located at line 10, column 13 prevents this PERFORM statement to reach its exit.

Ok for such situation I agree, Abstract Interpretation is welcome to be very similar with IBM Compiler.

[smedilol]

Isn't possible to report the full perform chain ?
Like perform a, Line 5 > perform b, Line 9 > perform c, Line 12 > perform a, Line 15?

With the current diagnostics, the Cobol developer have to manualy group error message together to understand what the cycle is.

[mayanje]

In fact this the current way of reporting, but the Perform Call Chain contains all information to do that.