[debugger] Enable reading embedded ppdb by thaystg · Pull Request #18895 · mono/mono

thaystg · 2020-02-17T18:04:08Z

To fix the issue of enable reading embedded pdb everywhere the problem was that when the roslyn tests are compiling sources it creates embedded pdb with source file "", and before enable reading the embedded pdb we don't read this information, and now we read and our StackTrace corlib doesn't expect a source file "", it's okay if it's NULL but not "".
I could fix in corlib or in mono, I prefer to fix on mono because I don't know if there is any other place in corlib that doesn't expect source file "".

…lot. To fix the issue of enabling embedded pdb everywhere the problem was that when the roslyn tests are compiling sources it creates embedded pdb with source file "", and before enable reading the embedded pdb we don't read this information, and now we read and our StackTrace file written in our corlib doesn't expect a source file "", it's okay if it's NULL but not "". I could fix in corlib or in mono, I prefer to fix on mono because I don't know if there is any other place that doesn't expect source file "".

mono/metadata/debug-mono-ppdb.c

jaykrell · 2020-02-17T19:44:49Z

mcs/build/config-default.make


 TEST_HARNESS = $(topdir)/class/lib/$(PROFILE_DIRECTORY)/$(PARENT_PROFILE)nunit-lite-console.exe
-PLATFORM_DEBUG_FLAGS = /debug:portable
+PLATFORM_DEBUG_FLAGS = /debug:embedded


Be sure to test both/all, not delete one test to add another.

Also, can these settings be combined -- portable and embedded?

Ah, embedded implies portable.

I removed this change, I will do this in another PR.

jaykrell · 2020-02-17T19:44:57Z

mcs/class/Mono.Debugger.Soft/Makefile


 $(test_output_dir)/dtest-excfilter.exe: Test/dtest-excfilter.il | $(test_output_dir)
-	$(ILASM) -out:$@ /exe /debug Test/dtest-excfilter.il
+	$(ILASM) -out:$@ /exe /debug:embedded Test/dtest-excfilter.il


Be sure to test both/all, not delete one test to add another.

I removed this change, I will do this in another PR.

jaykrell · 2020-02-17T19:45:52Z

mono/metadata/debug-mono-ppdb.c


-// Temporarily disabled to unblock Roslyn
-#if HOST_WIN32 || HOST_WASM
+#if HOST_WIN32 || HAVE_SYS_ZLIB


Are embedded pdbs always compressed?

I think yes

Don't you also need the HOST_WASM here? I thought wasm doesn't have a system zlib?

Changing the way to test strlen because of performance

thaystg · 2020-02-18T01:16:25Z

@monojenkins build failed

jaykrell · 2020-02-18T12:29:08Z

mono/metadata/debug-mono-ppdb.c


 	location = g_new0 (MonoDebugSourceLocation, 1);
-	location->source_file = docname;
+	if (docname != NULL && docname[0] != 0)


cosmetic: if (docname && docname [0])

or if (!docname || docname [0])
because replacing null with null is an identity, but maybe that is too subtle.

thaystg · 2020-02-18T15:09:42Z

@monojenkins build Linux AArch64

CoffeeFlux

Thanks for sorting this out. Figured it would be something silly I missed :(

Might still be worth bringing up with the Roslyn team to see if that's intended behavior? This fix should go in regardless though, given it works in CoreCLR.

CoffeeFlux · 2020-02-18T16:27:04Z

mono/metadata/debug-mono-ppdb.c


-// Temporarily disabled to unblock Roslyn
-#if HOST_WIN32 || HOST_WASM
+#if HOST_WIN32 || HAVE_SYS_ZLIB


Don't you also need the HOST_WASM here? I thought wasm doesn't have a system zlib?

lambdageek · 2020-02-18T17:28:51Z

This is related to #17209

thaystg · 2020-02-18T20:44:56Z

@monojenkins build failed

thaystg · 2020-02-19T19:16:52Z

@monojenkins build Linux ARMv5

Therzok · 2020-02-26T19:19:05Z

Is this getting backported to any current release?

thaystg · 2020-02-26T19:46:38Z

@monojenkins backport 2020-02

monojenkins · 2020-02-26T19:47:45Z

@thaystg backporting to 2020-02 failed, the patch results in conflicts:

Applying: Running the debugger tests using embedded pdb as this will be used a lot. To fix the issue of enabling embedded pdb everywhere the problem was that when the roslyn tests are compiling sources it creates embedded pdb with source file "", and before enable reading the embedded pdb we don't read this information, and now we read and our StackTrace file written in our corlib doesn't expect a source file "", it's okay if it's NULL but not "". I could fix in corlib or in mono, I prefer to fix on mono because I don't know if there is any other place that doesn't expect source file "".
Applying: Removing comment
Using index info to reconstruct a base tree...
M	mono/metadata/debug-mono-ppdb.c
Falling back to patching base and 3-way merge...
Auto-merging mono/metadata/debug-mono-ppdb.c
CONFLICT (content): Merge conflict in mono/metadata/debug-mono-ppdb.c
error: Failed to merge in the changes.
Patch failed at 0002 Removing comment

Please backport manually!

Backport #18895

thaystg requested review from akoeplinger, lambdageek, marek-safar and vargaz as code owners February 17, 2020 18:04

thaystg added 2 commits February 17, 2020 15:10

Merge branch 'master' into thays_test_embeddedpdb_sdb

06888bf

Removing comment

47444a4

monojenkins mentioned this pull request Feb 17, 2020

[debugger] Enable reading embedded ppdb dotnet/runtime#32447

Merged

jaykrell reviewed Feb 17, 2020

View reviewed changes

mono/metadata/debug-mono-ppdb.c Outdated Show resolved Hide resolved

jaykrell reviewed Feb 17, 2020

View reviewed changes

thaystg added 2 commits February 17, 2020 18:47

Removing changing tests to embedded, will do this in another commit

defea19

Changing the way to test strlen because of performance

Fixing typo

c0889f7

thaystg requested a review from jaykrell February 18, 2020 12:18

jaykrell reviewed Feb 18, 2020

View reviewed changes

Cosmetic change.

a114a2e

jaykrell approved these changes Feb 18, 2020

View reviewed changes

CoffeeFlux approved these changes Feb 18, 2020

View reviewed changes

Do not remove WASM.

eaf5619

Fixing netcore build

dbab899

thaystg merged commit dd5926c into mono:master Feb 20, 2020

thaystg mentioned this pull request Feb 26, 2020

[2020-02][debugger] Enable reading embedded ppdb #19049

Merged

monojenkins mentioned this pull request Feb 26, 2020

[2019-12] [debugger] Enable reading embedded ppdb #19050

Merged

akoeplinger pushed a commit that referenced this pull request Feb 27, 2020

[2020-02][debugger] Enable reading embedded ppdb (#19049)

2b7e10e

Backport #18895

lambdageek mentioned this pull request Feb 27, 2020

Enabling system zlib breaks appdomain remoting #17209

Closed

thaystg mentioned this pull request Jan 5, 2022

[release/6.0-maui] Add back HAVE_SYS_ZLIB support dotnet/runtime#63359

Merged

Conversation

thaystg commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaystg commented Feb 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaykrell Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaystg commented Feb 18, 2020

Uh oh!

CoffeeFlux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdageek commented Feb 18, 2020

Uh oh!

thaystg commented Feb 18, 2020

Uh oh!

thaystg commented Feb 19, 2020

Uh oh!

Therzok commented Feb 26, 2020

Uh oh!

thaystg commented Feb 26, 2020

Uh oh!

monojenkins commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thaystg commented Feb 17, 2020 •

edited

Loading

jaykrell Feb 18, 2020 •

edited

Loading