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

Implement support for Embedded Portable PDB #12711

Merged
merged 2 commits into from
Jul 29, 2016
Merged

Implement support for Embedded Portable PDB #12711

merged 2 commits into from
Jul 29, 2016

Conversation

tmat
Copy link
Member

@tmat tmat commented Jul 25, 2016

Implements #12390 and #12753

@tmat tmat added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 25, 2016
@tmat tmat removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 25, 2016
@tmat
Copy link
Member Author

tmat commented Jul 25, 2016

@dotnet/roslyn-compiler Please review.

@tmat
Copy link
Member Author

tmat commented Jul 26, 2016

@jaredpar @marek-safar FYI

@tmat
Copy link
Member Author

tmat commented Jul 26, 2016

@jaredpar I have not added /debug:windows. I thought about it and concluded that it'd be mostly adding noise -- I don't think anyone would really use it. If you feel strong about it I can certainly follow up and add it. We could also wait for customers to ask for it.

@jaredpar
Copy link
Member

I've gone back and forth on this. At this point I think we're fine not implementing /debug:windows now. We can keep it in our back pocket should a scenario come along where this is needed.

@@ -8394,11 +8420,13 @@ private void VerifyQuotedInvalid<T>(string name, string value, T expected, Func<
[Fact]
Copy link
Member

@jaredpar jaredpar Jul 26, 2016

Choose a reason for hiding this comment

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

Can we add a test which validates that on Linux we are choosing the correct PDB format? Either use ConditionalFact or run on both OS and verify that on Linux we use Portable, and Windows PDB

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see where you added those.

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 test does validate that on Linux/Mac we use Portable PDB and otherwise Windows PDB. ConditionalFact does the same check of PathUtilities.IsUnixLikePlatform value as the line below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is the only check we need. I didn't test this but I believe it'll still not work because COM support in Mono on Windows is not really working.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's unfortunate. #12753 states that /debug:full is OS specific, but not runtime specific. Does Mono not support COM at all? The interop we need is very simple (no COM registration needed). We P/Invoke native dll that directly returns COM object, which we then call methods on.

@tmat
Copy link
Member Author

tmat commented Jul 26, 2016

@jaredpar Keeping it in a back pocket sounds good to me.

@jaredpar
Copy link
Member

CC @dotnet/roslyn-compiler now that this is out for official review.

👍 from me.

@tmat
Copy link
Member Author

tmat commented Jul 28, 2016

@dotnet/roslyn-compiler Ping.

@jcouv
Copy link
Member

jcouv commented Jul 29, 2016

LGTM
Sorry I missed the design presentation on the topic, but what tool will consume the embedded PDB?

@tmat
Copy link
Member Author

tmat commented Jul 29, 2016

All the tools that consume standalone portable PDBs - the debugger, the CLR stack trace library, dotnet cli test tool, etc.

@tmat tmat merged commit 10412d8 into dotnet:master Jul 29, 2016
@tmat tmat deleted the EmbedPdb branch August 16, 2017 19:53
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.

5 participants