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

Add option to dump debug info to jit-diffs #166

Merged
merged 2 commits into from
Sep 6, 2018

Conversation

AndyAyersMS
Copy link
Member

Off by default. Enable via --debuginfo.

Info is dumped after the method (like unwind and eh) and will show up
as file diffs.

Off by default. Enable via --debuginfo.

Info is dumped after the method (like unwind and eh) and will show up
as file diffs.
@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

One bug.

Making it opt-in seems ok, but unlikely anyone will use it. We should make it the default it if behaves well with diffing (doesn't create unnecessary diffs with small changes). The reason GCInfo is not default is our only way to dump GC info now requires annotating instructions with code offsets, which means adding or removing an instruction causes diffs in all subsequent lines. If we changed our GC info dump to be interleaved with instructions (e.g., +rax +V01 -rcx), then we could (and should!) make GC info dumping the default as well.

@@ -362,6 +366,7 @@ private class DisasmEngine
_assemblyInfoList = assemblyInfoList;

this.doGCDump = config.DumpGCInfo;
this.doDebugDump = config.DumpGCInfo;
Copy link
Member

Choose a reason for hiding this comment

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

cut-and-paste error

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will fix.

@AndyAyersMS
Copy link
Member Author

Easiest thing is to make it off by default in the sub-tools but on by default in jit-diffs.

There can be cascading diffs like you mention for GC. But hopefully not too painful?

We could do the same kind of interleaved display for IL offsets and variable scopes that you suggest we do for GC info. Seems like it might be worth a try...

@BruceForstall
Copy link
Member

Ok, well if there might be cascading diffs, I would leave it as-is, and have a different work item (or items) to eliminate those, that would then change the defaults here.

@AndyAyersMS AndyAyersMS merged commit b74d29f into dotnet:master Sep 6, 2018
@AndyAyersMS AndyAyersMS deleted the DebugInfo branch September 6, 2018 03:54
@AndyAyersMS
Copy link
Member Author

Opened dotnet/coreclr#19850 for the intermingled approach.

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