Add internal instrumentation API for switch expression arm's expression #43430

gafter · 2020-04-16T23:29:43Z

gafter · 2020-04-16T23:40:48Z

@dotnet/roslyn-compiler @dotnet/roslyn-analysis Could you please have a look at this?

AlekseyTs · 2020-04-19T20:37:13Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_SwitchExpression.cs

-                        // Should go through this._localRewriter._instrumenter; see https://github.com/dotnet/roslyn/issues/42810
-                        loweredValue = new BoundSequencePointExpression(arm.Value.Syntax, loweredValue, loweredValue.Type);
-                    }
+                        loweredValue = this._localRewriter._instrumenter.InstrumentSwitchExpressionArmExpression(arm.Value, loweredValue, _factory);


loweredValue = this._localRewriter._instrumenter.InstrumentSwitchExpressionArmExpression(arm.Value, loweredValue, _factory); [](start = 24, length = 124)

It is not clear why instrumentation would be linked to generation of sequence points. #Closed

If you prefer I will rename GenerateSequencePoints to GenerateInstrumentation. #Resolved

AlekseyTs · 2020-04-19T20:38:21Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_SwitchExpression.cs

@@ -92,10 +92,7 @@ private BoundExpression LowerSwitchExpression(BoundConvertedSwitchExpression nod
                    sectionBuilder.Add(_factory.Label(arm.Label));
                    var loweredValue = _localRewriter.VisitExpression(arm.Value);
                    if (GenerateSequencePoints)
-                    {


{ [](start = 20, length = 1)

If we are going to keep the if at the end, consider keeping braces as well. #Closed

All single-line if statements in this source file are without braces. I prefer to maintain that consistency.

In reply to: 410986252 [](ancestors = 410986252)

AlekseyTs · 2020-04-19T20:42:54Z

Done with review pass (iteration 1). I would expect to see some tests verifying that instrumentation is happening as expected even when no debug information is generated. #WontFix

gafter · 2020-04-20T17:53:11Z

As you can see from the implementation in Instrumenter.cs, we do not currently instrument any parts of a switch expression. See Instrumenter.InstrumentSwitchWhenClauseConditionalGotoBody and Instrumenter.InstrumentSwitchExpressionArmExpression. The purpose of this PR is to go through those hooks when producing instrumentation such as debugging output or instrumentation. Since we do not produce instrumentation, there is no observable change to the external behavior of the compiler due to this PR.

In reply to: 616222134 [](ancestors = 616222134)

gafter · 2020-04-20T18:10:16Z

@AlekseyTs I think I've responded to all of your comments.

@agocke @RikkiGibson Could you please review this?

…into patterns3-42810

AlekseyTs · 2020-04-21T17:31:19Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.DecisionDagRewriter.cs

@@ -904,7 +904,7 @@ private void LowerWhenClause(BoundWhenDecisionDagNode whenClause)

                    // We hide the jump back into the decision dag, as it is not logically part of the when clause
                    BoundStatement jump = _factory.Goto(GetDagNodeLabel(whenFalse));
-                    sectionBuilder.Add(GenerateSequencePoints ? _factory.HiddenSequencePoint(jump) : jump);
+                    sectionBuilder.Add(GenerateInstrumentation ? _factory.HiddenSequencePoint(jump) : jump);


GenerateInstrumentation [](start = 39, length = 23)

It is not clear why generation of hidden sequence points is conditioned on instrumentation #Resolved

Because we produce sequence points whenever we use the instrumentation API and only then. Hidden sequence points are needed here when we are producing sequence points. (When not producing a pdb, the hidden sequence points are dropped on the floor later along with the sequence points). It is only because this code is shared between contexts in which we are producing sequence points (e.g. the switch statement) and those in which are are not (e.g. the is-pattern expression) that we need this conditional.

In reply to: 412352452 [](ancestors = 412352452)

AlekseyTs · 2020-04-21T17:44:26Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_PatternSwitchStatement.cs

@@ -26,7 +26,7 @@ private sealed class SwitchStatementLocalRewriter : BaseSwitchLocalRewriter
            /// </summary>
            private readonly Dictionary<SyntaxNode, LabelSymbol> _sectionLabels = PooledDictionary<SyntaxNode, LabelSymbol>.GetInstance();

-            protected override bool GenerateSequencePoints => true;
+            protected override bool GenerateInstrumentation => true;


true [](start = 63, length = 4)

Unconditional instrumentation feels suspicious #Closed

All statements are instrumented today. For example, this long appears (without any condition) in LocalRewriter.VisitBlock:

BoundStatement? prologue = _instrumenter.CreateBlockPrologue(node, out synthesizedLocal);

Normally we simply call the instrumentation API when lowering a statement. Since this overrides a method in code that is shared between processing expressions (e.g. the is-pattern expression) and statmements (e.g. the switch statement) we use this to communicate which kind of context we are in.

In reply to: 412362045 [](ancestors = 412362045)

Normally we instrument conditionally, based on LocalRewriter.Instrument value. Even the LocalRewriter.VisitBlock that you use as an example bypasses instrumentation when LocalRewriter.Instrument is false (see an if statement 5 lines above). I expect at least that value to be always taken into account.

In reply to: 412452424 [](ancestors = 412452424,412362045)

AlekseyTs · 2020-04-21T17:50:43Z

The API got hooked into the product, therefore we should find a way to test it. It is quite possible that the easiest way to do that is to implement instrumentation in DynamicAnalysisInjector. Alternatively we can unhook the API, keep this as just an introduction of the API. And hook it into the product once we are comfortable with its shape and ready to test its usage proper.

In reply to: 616714458 [](ancestors = 616714458,616222134)

AlekseyTs · 2020-04-21T17:53:12Z

Done with review pass (iteration 3)

gafter · 2020-04-21T19:56:25Z

It is tested. We go through this API to produce the sequence points, and there are many existing tests that demonstrate the presence of the sequence points produced through this API.

In reply to: 617316315 [](ancestors = 617316315,616714458,616222134)

AlekseyTs · 2020-04-21T20:12:22Z

It is tested. We go through this API to produce the sequence points, and there are many existing tests that demonstrate the presence of the sequence points produced through this API.

The change adds zero tests. I have doubts about conditions under which the instrumentation is invoked. The only way to eliminate them is through tests. I don't think we can rely on existing tests around debug information (sequence points) because I am specifically concerned about scenarios where we instrument, but do not generate debug information and vise versa.

In reply to: 617380907 [](ancestors = 617380907,617316315,616714458,616222134)

gafter · 2020-04-21T20:29:59Z

This is exactly the same as InstrumentSwitchStatementExpression, InstrumentCatchClauseFilter and others which you added but without the same kind of tests that you now seem to think are required. How did you eliminate your doubts then?

In reply to: 617388877 [](ancestors = 617388877,617380907,617316315,616714458,616222134)

AlekseyTs · 2020-04-21T20:49:50Z

This is exactly the same as InstrumentSwitchStatementExpression, InstrumentCatchClauseFilter and others which you added but without the same kind of tests that you now seem to think are required. How did you eliminate your doubts then?

I assume the scenarios are tested under umbrella of dynamic analysis tests. I also had more context at the time, given that the feature was under active development and I was working on it. Right now, I am reviewing the code, the code that is either new, or significantly modified since then. The code that appears to use some different logic around invoking instrumentation. All I have to go from - is my intuition, which might or might not be wrong. If you are so against adding tests, I suggest a compromise - let's remove usage of the new API from the product and start using it when we can easily test its actual effect under different conditions, or add a PROTOTYPE comment in the right place to catch up on testing later.

In reply to: 617397889 [](ancestors = 617397889,617388877,617380907,617316315,616714458,616222134)

gafter · 2020-04-21T20:54:54Z

No, the new API is used to produce debugging support for these contexts, just like for InstrumentSwitchStatementExpression, InstrumentCatchClauseFilter and others. There are many tests that confirm that the debugging information is present where the new API is called. Adding a PROTOTYPE comment will not facilitate merging into master this week. I am confident that the testing of the use of the APIs is more than sufficient.

In reply to: 617406776 [](ancestors = 617406776,617397889,617388877,617380907,617316315,616714458,616222134)

AlekseyTs · 2020-04-21T21:00:06Z

No, the new API is used to produce debugging support for these contexts, just like for InstrumentSwitchStatementExpression, InstrumentCatchClauseFilter and others. There are many tests that confirm that the debugging information is present where the new API is called. Adding a PROTOTYPE comment will not facilitate merging into master this week. I am confident that the testing of the use of the APIs is more than sufficient.

Since using or not using the API makes no difference. If we unhook that one call, there would be no observable behavior change and we can merge to master.

In reply to: 617409044 [](ancestors = 617409044,617406776,617397889,617388877,617380907,617316315,616714458,616222134)

AlekseyTs · 2020-04-21T21:02:31Z

To be honest, I don't understand why you don't want to implement dynamic analysis for this case, we should have examples how to instrument expressions and I don't think this one is special in any way.

In reply to: 617411518 [](ancestors = 617411518,617409044,617406776,617397889,617388877,617380907,617316315,616714458,616222134)

… statements or in expression lambdas. Though neither of these conditions is possible today.

gafter · 2020-04-21T21:25:46Z

I do not intend to design the experience of instrumentation for expressions. I intend to work with the team responsible for that to help them design the experience they intend.

Removing the invocation of the new API would indeed break a bunch of tests. That's how we know it works.

In reply to: 617412554 [](ancestors = 617412554,617411518,617409044,617406776,617397889,617388877,617380907,617316315,616714458,616222134)

AlekseyTs · 2020-04-21T21:40:58Z

Removing the invocation of the new API would indeed break a bunch of tests. That's how we know it works.

I guess I wasn't clear enough. I meant to restore loweredValue = new BoundSequencePointExpression(arm.Value.Syntax, loweredValue, loweredValue.Type); back and remove the loweredValue = this._localRewriter._instrumenter.InstrumentSwitchExpressionArmExpression(arm.Value, loweredValue, _factory); in the LowerSwitchExpression method.That change is completely unobservable.

In reply to: 617422758 [](ancestors = 617422758,617412554,617411518,617409044,617406776,617397889,617388877,617380907,617316315,616714458,616222134)

AlekseyTs · 2020-04-21T21:45:56Z

I intend to work with the team responsible for that to help them design the experience they intend.

What is the plan here? At what point that work is going to start? Is it really necessary to merge the API into master before the shape of the API is finalized and, at least, proven to work through implementing dynamic analysis (at least as a prototype)? #Resolved

gafter · 2020-04-21T21:59:32Z

We only produce a BoundSequencePointExpression through the instrumentation APIs.

In reply to: 617429142 [](ancestors = 617429142,617422758,617412554,617411518,617409044,617406776,617397889,617388877,617380907,617316315,616714458,616222134)

gafter · 2020-04-21T22:34:24Z

Since we use the instrumentation API to produce all sequence points for breakpoint locations, and since you commented on a previous PR that it was necessary to do so, yes, it is necessary. There is an issue (#43544) tracking the dynamic instrumentation work, and I do not yet have insight into that team's plans or designs for when or how they intend to proceed. I am fine with this API changing in the future in the unlikely event it is found to need changes for dynamic analysis; this shape of the API is sufficient for producing debugging breakpoints.

In reply to: 617431028 [](ancestors = 617431028)

AlekseyTs · 2020-04-21T22:45:52Z

There is an issue (#43544) tracking the dynamic instrumentation work

Thank you for opening the issue to track completion of the work. This address my final concerns regarding this PR. #Resolved

AlekseyTs

LGTM (iteration 5)

…into patterns3-42810

…to patterns3-42810

…into patterns3-42810

(It used to depend on calling a virtual property `GenerateInstrumentation` implemented in a subtype)

gafter · 2020-04-24T15:12:54Z

Reviewers: I have added an iteration to fix an order-of-initialization bug. Please let me know if you have any comments on that.

AlekseyTs · 2020-04-24T18:03:54Z

Reviewers: I have added an iteration to fix an order-of-initialization bug. Please let me know if you have any comments on that.

I do not see anything obviously wrong in the 10th commit

ghost

Auto-approval

…into patterns3-42810

Add internal instrumentation API for switch expression arm's expression

5469157

Fixes dotnet#42810

gafter added Area-Compilers New Language Feature - Pattern Matching Pattern Matching labels Apr 16, 2020

gafter added this to the Compiler.Net5 milestone Apr 16, 2020

gafter requested review from agocke, RikkiGibson and a team April 16, 2020 23:29

gafter self-assigned this Apr 16, 2020

gafter added Area-Interactive Interactive-DynamicAnalysis labels Apr 16, 2020

AlekseyTs reviewed Apr 19, 2020

View reviewed changes

Rename GenerateSequencePoints to GenerateInstrumentation

5f72ae8

agocke approved these changes Apr 20, 2020

View reviewed changes

Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn …

a31caa6

…into patterns3-42810

AlekseyTs reviewed Apr 21, 2020

View reviewed changes

Do not generate switch statement instrumentation for synthetic switch…

305734c

… statements or in expression lambdas. Though neither of these conditions is possible today.

gafter mentioned this pull request Apr 21, 2020

Dynamic analysis for switch expressions. #43544

Open

Add mention of new pattern forms in test plan.

8d9ba66

AlekseyTs approved these changes Apr 21, 2020

View reviewed changes

Neal Gafter added 5 commits April 21, 2020 17:10

Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn …

4d380c5

…into patterns3-42810

Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn …

b21dd7d

…into patterns3-42810

Merge branch 'patterns3-42810' of https://github.com/gafter/roslyn in…

ddfa6a8

…to patterns3-42810

Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn …

db9d300

…into patterns3-42810

Resolve an initialization order bug in PatternLocalRewriter constructor

cc5b09b

(It used to depend on calling a virtual property `GenerateInstrumentation` implemented in a subtype)

gafter mentioned this pull request Apr 24, 2020

Fix several issues with sequence points and switch expressions #43471

Merged

gafter added the auto-merge label Apr 24, 2020

ghost approved these changes Apr 24, 2020

View reviewed changes

Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn …

9b5cbb0

…into patterns3-42810

gafter closed this Apr 27, 2020

gafter reopened this Apr 27, 2020

ghost merged commit 26d509b into dotnet:features/patterns3 Apr 27, 2020

gafter mentioned this pull request Apr 27, 2020

Instrumentation for switch expressions #42810

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal instrumentation API for switch expression arm's expression #43430

Add internal instrumentation API for switch expression arm's expression #43430

gafter commented Apr 16, 2020

gafter commented Apr 16, 2020

AlekseyTs Apr 19, 2020 •

edited

Loading

gafter Apr 20, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

gafter Apr 20, 2020

AlekseyTs commented Apr 19, 2020 •

edited by gafter

Loading

gafter commented Apr 20, 2020

gafter commented Apr 20, 2020

AlekseyTs Apr 21, 2020 •

edited by gafter

Loading

gafter Apr 21, 2020 •

edited

Loading

AlekseyTs Apr 21, 2020 •

edited

Loading

gafter Apr 21, 2020

AlekseyTs Apr 21, 2020

AlekseyTs commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020

gafter commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020 •

edited

Loading

gafter commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited

Loading

gafter commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020

gafter commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020 •

edited by gafter

Loading

gafter commented Apr 21, 2020

gafter commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited by gafter

Loading

AlekseyTs left a comment

gafter commented Apr 24, 2020

AlekseyTs commented Apr 24, 2020

ghost left a comment

Add internal instrumentation API for switch expression arm's expression #43430

Add internal instrumentation API for switch expression arm's expression #43430

Conversation

gafter commented Apr 16, 2020

gafter commented Apr 16, 2020

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

gafter Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

gafter Apr 20, 2020

Choose a reason for hiding this comment

AlekseyTs commented Apr 19, 2020 • edited by gafter Loading

gafter commented Apr 20, 2020

gafter commented Apr 20, 2020

AlekseyTs Apr 21, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

gafter Apr 21, 2020

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2020

Choose a reason for hiding this comment

AlekseyTs commented Apr 21, 2020 • edited Loading

AlekseyTs commented Apr 21, 2020

gafter commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020 • edited Loading

gafter commented Apr 21, 2020 • edited Loading

AlekseyTs commented Apr 21, 2020 • edited Loading

gafter commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020

gafter commented Apr 21, 2020 • edited Loading

AlekseyTs commented Apr 21, 2020

AlekseyTs commented Apr 21, 2020 • edited by gafter Loading

gafter commented Apr 21, 2020

gafter commented Apr 21, 2020 • edited Loading

AlekseyTs commented Apr 21, 2020 • edited by gafter Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

gafter commented Apr 24, 2020

AlekseyTs commented Apr 24, 2020

ghost left a comment

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 •

edited

Loading

gafter Apr 20, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs commented Apr 19, 2020 •

edited by gafter

Loading

AlekseyTs Apr 21, 2020 •

edited by gafter

Loading

gafter Apr 21, 2020 •

edited

Loading

AlekseyTs Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited

Loading

gafter commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited

Loading

gafter commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited by gafter

Loading

gafter commented Apr 21, 2020 •

edited

Loading

AlekseyTs commented Apr 21, 2020 •

edited by gafter

Loading