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

DebugInfoBuilder for sequence point v2 #1237

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

Hecate2
Copy link
Contributor

@Hecate2 Hecate2 commented Nov 9, 2024

For #1235 (review) . Not used for master, but for PR 1235.
(But works for master as well)

@Hecate2 Hecate2 force-pushed the compilerinfo-optimize-seq-v2 branch from 18a6f6a to 70d0c0d Compare November 9, 2024 03:30
@Hecate2
Copy link
Contributor Author

Hecate2 commented Nov 9, 2024

If seq v2 uses array instead of object, NEED CHANGES

@shargon
Copy link
Member

shargon commented Nov 9, 2024

If seq v2 uses array instead of object, NEED CHANGES

I think we can use object, if optimized: false, and if optimized:true it can be an array

@Hecate2
Copy link
Contributor Author

Hecate2 commented Nov 9, 2024

If seq v2 uses array instead of object, NEED CHANGES

I think we can use object, if optimized: false, and if optimized:true it can be an array

OK I can handle it

@@ -304,7 +304,7 @@ public JObject CreateDebugInformation(string folder = "")
// Create sequence-points-v2

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

Choose a reason for hiding this comment

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

If the method was not optimized, it will be good to avoid the change, maybe some methods are not optimized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally it can be expensive to check whether a method is optimized or not. I need to visit all the Instructions that may be covered by the method.

Copy link
Member

Choose a reason for hiding this comment

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

If we check the previous sequencePoints with the current one and are different, it was optimized, isn't it?

parsedMethod.Offset = abiMethod.Offset; // It can be optimized, we avoid to check the offset
if (!parsedMethod.Equals(abiMethod)) continue;

if (method["abi"] is JObject && int.Parse(method["abi"]!["offset"]!.AsString()) == abiMethod.Offset)
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use the parsed version, with offset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I revert this? I do not really understand what to do here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, only removing the offset = line should work

else
newSequencePointsV2[writeAddr] = ((JArray)newSequencePointsV2[writeAddr]!).Concat(oldContentArray).ToArray();
}
method["sequence-points-v2"] = newSequencePointsV2;
Copy link
Member

@shargon shargon Nov 12, 2024

Choose a reason for hiding this comment

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

if there is only one entry it should be good to preserve the object, also, in what cases will be more (inline methods?)? it was checked that if it's the same information, don't duplicate it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there is only one entry it should be good to preserve the object, also, in what cases will be more (inline methods?)? it was checked that if it's the same information, don't duplicate it?

For example, mutiple RET instructions may be compressed to 1 RET.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there is only one entry it should be good to preserve the object, also, in what cases will be more (inline methods?)? it was checked that if it's the same information, don't duplicate it?

Something like JToken.DeepEquals comparer is needed for a HashSet, if we need to eliminate the duplicates. Not easy.

@shargon shargon merged commit fc2bab9 into compilerinfo Nov 14, 2024
4 checks passed
@shargon shargon deleted the compilerinfo-optimize-seq-v2 branch November 14, 2024 08:16
Jim8y pushed a commit that referenced this pull request Nov 14, 2024
* Compiler info

* don't change logic

* use sequence-points-v2

* Optimization

* Fix dissasembler

* Use abi

* clean

* Optimization type

* Use debug type

* Remove V1 method detection, doesn't work without abi

* rename

* use TryParse

* Fix artifact

* Fix artifacts with Abi identification

* DebugInfoBuilder for sequence point v2 (#1237)

* DebugInfoBuilder for sequence point v2

* v2["optimization"]

* handle method["abi"]["offset"] in debugInfo

* apply code review

* Fix merge

* Fix merge

---------

Co-authored-by: Hecate2 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants