WI #1750 Add DFA support #1751

mayanje · 2020-07-30T13:09:20Z

According to an internal discussion, this PR is created on a branch which will be the base branch for DFA integration discussions, reports and requests.

TypeCobol.Analysis/Util/Antlr4_license.txt

TypeCobol.Analysis/Report/ZCallPgmReport.cs

…260_CFG_DFA_SemanticDomainLite_IntegrateAsAnalyzer # Conflicts: # TypeCobol.Analysis.Test/CfgTestUtils.cs # TypeCobol.Analysis.Test/TypeCobol.Analysis.Test.csproj

fm-117

Here are my suggestions for improvement

The WithDfa mode for CFG building is the same as the Extended mode, the only difference is on the generic arguments for the block type. We could decouple CFG building from DFA by removing the need for typing the block data (i.e. stored as object, DFA builder would have to cast when reading)
general remarks
- remove locks
- restrict visibilities where possible to avoid misuses
- remove unused code
- some test code may be factorized
Some reports collect results during the Node phase, others may work on a CompilationUnit after parsing has completed so it is confusing on how to use the IReport interface. Maybe create 2 sorts of reports (one is tied to the parsing process, the other directly uses the results) ?
@smedilol also suggested that the process of determining how variables are affected by each statement (currently in ZCallPgmReport.ComputeUsePointDefPaths method) should be moved into a separate class for future reuse.

See other following remarks.

TypeCobol.Analysis.Test/BasicCfgProgramTest.cs

TypeCobol.Analysis.Test/CfgDfaBuildingTests.cs

TypeCobol.Analysis.Test/CfgTestUtils.cs

fm-117 · 2020-08-19T12:17:17Z

TypeCobol.Analysis.Test/DfaGenAndKillSetTest.cs

+            Assert.IsNotNull(dfaBuilder.Cfg.AllBlocks[1].Data.Gen);            
+            Assert.AreEqual("{0, 2}", dfaBuilder.Cfg.AllBlocks[1].Data.Gen.ToString());
+            //MOVE 2 TO I
+            Assert.IsTrue(dfaBuilder.Cfg.AllBlocks[1].Data.Gen.Get(0));


Isn't it redundant to test the set bits after having already checked the ToString result ?

I aggree, but it's just because I follow the same kind of logic: checking instructions.

TypeCobol.Analysis.Test/TypeCobol.Analysis.Test.csproj

fm-117 · 2020-08-20T08:31:30Z

TypeCobol.Analysis/Report/ZCallPgmReport.cs

+        /// <summary>
+        /// Visitor to Collect all level 88 symbols.
+        /// </summary>
+        class Level88SymbolCollector : AbstractSymbolAndTypeVisitor<Dictionary<Symbol, Symbol>, Dictionary<Symbol, Symbol>>


I'm a bit lost here... ParentSymbol is never updated so this class will associate the given ParentSymbol to any level 88 found under it. But can't we simply use Owner of the level 88 to get this info ?

We are collecting level 88 symbols and we associate to each such symbol its parent, accumulator variable "symbol", is reused for all such symbols having different parents.

Ok now I see that the collector works for every 88-levels found under initial parent symbol (and not only those located directly under it).

fm-117 · 2020-08-20T08:59:11Z

TypeCobol.Analysis/Report/ZCallPgmReport.cs

+        private void OnDefPoint(DataFlowGraphBuilder<Node, Symbol> dfaBuilder, DfaDefPoint<Node, Symbol> dp)
+        {
+            VariableSymbol sym = (VariableSymbol)dp.Variable;
+            Symbol parent = null;
+            if (sym.Level == 88 && sym.Value != null && Level88SymbolParentSymbolMap != null && Level88SymbolParentSymbolMap.TryGetValue(sym, out parent))
+            {//Now say that it is the parent variable which is the target of the definition.
+                dp.Variable = parent;
+                dp.UserData = sym;
+            }
+        }


In fact this is true in every Cobol program, a DEF on a 88 level is in fact a DEF on its parent variable so I think this logic should be moved to DefaultDataFlowGraphBuilder

No this is a hack here, to make DFA think that when there is a level 88 variable assignment, it is it's parent variable which is assigned, DFA must track by the CALL the parent variable value changing and not the level 88 children variables.

I agree, all I say is that this logic should be moved to DefaultDataFlowGraphBuilder otherwise all reports or class that would use the DFA results will have to perform this hack on their own, it must be factorized.

Hum... I think it is a contextual logic, not sure that factorizinf such logic, will lead to a generic pattern.

TypeCobol.Analysis/Report/ZCallPgmReport.cs

TypeCobol.Analysis/Util/BitSet.cs

fm-117 · 2020-08-20T15:03:23Z

TypeCobol.Analysis.Test/DfaCallPgmReport.cs

+    /// Dfa test for call module report
+    /// </summary>
+    [TestClass]
+    public class DfaCallPgmReport


These 2 tests are duplicate with the 2 in CLI, should we keep them ?

Yes, because theses are for CLI using the -cfg WithDfa and -zcr options in the command line arguments.

On one hand the CLI tests are more complete because they also cover the new options, on the other hand the tests in TypeCobol.Analysis are easier to debug because they run in the same process...
I'm ok to keep them both but as they are duplicates, we'll have to keep in mind that changes made on one have to be copied on the other.

Yes but anyway they still to be different tests.

fm-117 · 2020-08-21T07:33:23Z

TypeCobol.Analysis/Dfa/DfaDefPoint.cs

+    /// </summary>
+    /// <typeparam name="I">Instruction generic Type</typeparam>
+    /// <typeparam name="V">Used Variable generic Type</typeparam>
+    public class DfaDefPoint<I, V>


I think that all properties of this class should be readonly except UserData. This is compatible with the current usages except for the hack on SET statements for 88 levels (see my remark in ZCallPgmReport.cs)

Ok for "UserData" variable, "Variable" is used by the ZCallPgmReport hack, other variables can be made internal set.
This is not the purpose here , but we can image that we can use the same hack to detect parent variable indirectly targeted by their REDEFINES or RENAMES variables.

fm-117 · 2020-08-21T07:43:13Z

TypeCobol.Analysis/Dfa/DfaUsePoint.cs

+    /// </summary>
+    /// <typeparam name="I">Instruction generic Type</typeparam>
+    /// <typeparam name="V">Used Variable generic Type</typeparam>
+    public class DfaUsePoint<I, V>


Same remark as for DEF points, I'll leave it as it is now until we decide for the DEF points properties accessibility.

Same answer as for DEF.

…ariable

1260 dfa minor refactorings

…anges)

…iption

… events

fm-117 · 2020-11-09T11:14:10Z

Replaced by #1802.

WI #1750 Add DFA support

2d49141

trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Jul 30, 2020

mayanje requested review from fm-117, rooksdo and smedilol July 30, 2020 13:09

mayanje assigned mayanje and fm-117 Jul 30, 2020

smedilol suggested changes Aug 14, 2020

View reviewed changes

TypeCobol.Analysis/Util/Antlr4_license.txt Show resolved Hide resolved

TypeCobol.Analysis/Report/ZCallPgmReport.cs Outdated Show resolved Hide resolved

TypeCobol.Analysis/Report/ZCallPgmReport.cs Outdated Show resolved Hide resolved

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Aug 14, 2020

Merge branch '1260_CFG_SemanticDomainLite_IntegrateAsAnalyzer' into 1…

77db331

…260_CFG_DFA_SemanticDomainLite_IntegrateAsAnalyzer # Conflicts: # TypeCobol.Analysis.Test/CfgTestUtils.cs # TypeCobol.Analysis.Test/TypeCobol.Analysis.Test.csproj

trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Aug 17, 2020

fm-117 requested changes Aug 20, 2020

View reviewed changes

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Aug 20, 2020

fm-117 reviewed Aug 20, 2020

View reviewed changes

fm-117 added 2 commits August 20, 2020 18:18

WI #1750 Minor changes

761259b

WI #1750 Remove StorageArea visitor, use node dictionaries instead

9a6f503

fm-117 reviewed Aug 21, 2020

View reviewed changes

WI #1750 Code cleanup

6797e27

fm-117 changed the base branch from 1260_CFG_SemanticDomainLite_IntegrateAsAnalyzer to 1260_CFG_SemanticDomainLite August 21, 2020 09:01

fm-117 and others added 5 commits August 24, 2020 18:26

WI #1275 Add ValueOrigin class to compute possible value paths of a V…

cae2b9d

…ariable

WI #1275 Adapt comment for ValueOrigin.ComputeFrom method

96382f9

WI #1275 Improve path description for condition variables

e1a89d8

WI #1272 Optimize predecessors lists building

e7bc3cc

Merge pull request #1761 from TypeCobolTeam/1260_DFA_MinorRefactorings

b03ed3a

1260 dfa minor refactorings

trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Sep 2, 2020

fm-117 and others added 8 commits September 2, 2020 11:33

WI #1750 Add BitSet Xor unit test

4cc372e

WI #1750 Swap actual and expected for BitSet tests

6bb4325

WI #1766 changing internal to public.

3458bbe

WI #1750 Use proper analyzer identifier in CLI

d7983f4

WI #1750 Make all inheritors of SetStatement public (+ minor style ch…

fe613e2

…anges)

WI #1750 Use VariableSymbol instead of Symbol for DFA variables descr…

9aff694

…iption

WI #1750 Handle uninitialized variables in AsPaths method

dc22561

WI #1750 Use standard .NET framework convention for UsePoint/DefPoint…

6dc95b9

… events

fm-117 requested review from fm-117 and smedilol October 21, 2020 09:15

fm-117 assigned mayanje and unassigned mayanje Oct 21, 2020

fm-117 merged commit ed9e15e into 1260_CFG_SemanticDomainLite Nov 9, 2020

trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Nov 9, 2020

fm-117 mentioned this pull request Nov 9, 2020

WI #1750 Add DFA support [NEW] #1802

Closed

This was referenced Dec 31, 2020

Integrate DFA in CFG_SemanticDomainLite #1750

Closed

Dataflow analysis #627

Closed

mayanje mentioned this pull request Jan 28, 2021

V1.5 to Master #1873

Merged

WI #1750 Add DFA support #1751

WI #1750 Add DFA support #1751

Uh oh!

Conversation

mayanje commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fm-117 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayanje Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fm-117 Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fm-117 commented Nov 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mayanje commented Jul 30, 2020 •

edited

Loading

fm-117 left a comment •

edited

Loading

mayanje Aug 26, 2020 •

edited

Loading

fm-117 Aug 20, 2020 •

edited

Loading