Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler source info into DebugInfo #1235

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Compiler source info into DebugInfo #1235

wants to merge 17 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Nov 8, 2024

Alternative to #1229
Related to neo-project/proposals#184
Require #1240

This introduce a new property, sequence-points-v2, inside the method in debugInfo:

sequence-points remains as before, not changed:

{
  "sequence-points": [
    "20[0]12:20-12:21",
    "21[0]12:24-12:25",
    "22[0]12:20-12:25",
    "23[0]12:20-12:25",
    "25[0]12:28-12:35",
    "32[0]12:20-12:35",
    "33[0]12:20-12:35",
    "35[0]12:13-12:36",
    "37[0]13:9-13:10"
  ]
}

sequence-points-v2 was included in json format, with more information:

{
  "sequence-points-v2": {
    "20": {
      "source": {
        "document": 0,
        "location": "12:20-12:21"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "21": {
      "source": {
        "document": 0,
        "location": "12:24-12:25"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "22": {
      "source": {
        "document": 0,
        "location": "12:20-12:25"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "23": {
      "source": {
        "document": 0,
        "location": "12:20-12:25"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "25": {
      "source": {
        "document": 0,
        "location": "12:28-12:35"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "32": {
      "source": {
        "document": 0,
        "location": "12:20-12:35"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "33": {
      "source": {
        "document": 0,
        "location": "12:20-12:35"
      },
      "compiler": {
        "file": "Expression.cs",
        "line": 36,
        "method": "ConvertExpression"
      }
    },
    "35": {
      "source": {
        "document": 0,
        "location": "12:13-12:36"
      },
      "compiler": {
        "file": "ReturnStatement.cs",
        "line": 42,
        "method": "ConvertReturnStatement"
      }
    },
    "37": {
      "source": {
        "document": 0,
        "location": "13:9-13:10"
      },
      "compiler": {
        "file": "BlockStatement.cs",
        "line": 52,
        "method": "ConvertBlockStatement"
      }
    }
  }
}

Note: My bad, it was pushed to main and already reverted...

@shargon shargon requested a review from Jim8y November 8, 2024 17:05
@shargon shargon changed the title Compiler source info into DebugInfo Compiler source info into DebugInfo Nov 8, 2024
@shargon shargon marked this pull request as ready for review November 8, 2024 17:07
@shargon shargon requested a review from Hecate2 November 8, 2024 17:30
@shargon shargon added the blocked label Nov 8, 2024
@shargon shargon mentioned this pull request Nov 8, 2024
Hecate2
Hecate2 previously approved these changes Nov 9, 2024
Copy link
Contributor

@Hecate2 Hecate2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need to modify sequence v2 in my optimizer, inneo-devpack-dotnet/src/Neo.Compiler.CSharp/Optimizer/AssetBuilder/DebugInfoBuilder.cs

@Hecate2
Copy link
Contributor

Hecate2 commented Nov 9, 2024

And there is a problem that, after optimization, a single Instruction may come from multiple different lines of contract source codes, and seq v2 using key-value is unable to represent them. Yet this will not be a problem for my debuggers or optimizers.

@shargon
Copy link
Member Author

shargon commented Nov 9, 2024

And there is a problem that, after optimization, a single Instruction may come from multiple different lines of contract source codes, and seq v2 using key-value is unable to represent them. Yet this will not be a problem for my debuggers or optimizers.

I will change it to an array

@roman-khimov
Copy link

This introduce a new property, sequence-points-v2, inside the method in debugInfo:

Hey, how about NEP-19? We have a number of compilers and we can't do these changes that easy. And I'm still not sure what's the real problem we're trying to solve, looks like this data is purely an internal compiler thing. I'd not expect it to be a part of the debug info since the details of codegenerator are not really relevant there and compiler should be debugged in some other way. It can be a separate info file if really needed, but debug info is expected to be NEP-19-compliant unless we have any real reasons to change that standard.

@shargon
Copy link
Member Author

shargon commented Nov 9, 2024

This introduce a new property, sequence-points-v2, inside the method in debugInfo:

Hey, how about NEP-19? We have a number of compilers and we can't do these changes that easy. And I'm still not sure what's the real problem we're trying to solve, looks like this data is purely an internal compiler thing. I'd not expect it to be a part of the debug info since the details of codegenerator are not really relevant there and compiler should be debugged in some other way. It can be a separate info file if really needed, but debug info is expected to be NEP-19-compliant unless we have any real reasons to change that standard.

Is full compatible with previous version, just more information inside the json for easily trace the compiler

@roman-khimov
Copy link

Yeah, it's an extension, but every tool that expects a proper NEP-19 document can fail with it. Every developer that see it has to guess what this all is about (NEP-19 things are documented). To me it's either we have and follow some standards or not.

@shargon
Copy link
Member Author

shargon commented Nov 9, 2024

Yeah, it's an extension, but every tool that expects a proper NEP-19 document can fail with it. Every developer that see it has to guess what this all is about (NEP-19 things are documented). To me it's either we have and follow some standards or not.

If you strictly follow NEP 17, couldn't you add other methods beyond the standard?
It's about complying with the standard, if it has extra information or methods it doesn't affect it at all.

/// Script: DAtUb2tlblN5bWJvbEA=
/// 00 : OpCode.PUSHDATA1 546F6B656E53796D626F6C [8 datoshi]
/// 0D : OpCode.RET [0 datoshi]
/// </remarks>
[DisplayName("symbol")]
public abstract string? Symbol();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed with the Abi export

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't relate the DebugInfo to the ABI, so, this information will only work with others compilers if they attach the abi information to the NEP19 file

// Create sequence-points-v2

var v2 = new JObject();
v2["optimization"] = CompilationOptions.OptimizationType.None.ToString().ToLowerInvariant();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hecate2

TODO: it will require a change in the optimization later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Value of ["optimization"] is not changed in optimization.

@shargon shargon mentioned this pull request Nov 11, 2024
@Hecate2
Copy link
Contributor

Hecate2 commented Nov 12, 2024

#1237 ready for merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants