Portable PDB support for ilasm #37702

ivanpovazan · 2020-06-10T12:50:14Z

This is my take on adding portable PDB support to ilasm.

I understand that this PR is quite large, which is against the contribution guidelines, but adding portable PDB support requires a lot of changes.

Even though the work is not complete, after talking to @noahfalk I have decided to create this draft pull request to start the discussion and get some feedback on the work that has been done so far. Thus I would really appreciate any comments/suggestions/corrections in order to finalize this feature.

The work is based on the thread discussion (#5051) and mainly follows the idea of creating a portable PDB metadata writer based on extensions to IMetaDataEmit interface. It is still WIP, but the current version can generate portable PDB, which can be loaded by VS debugger and I have successfully managed to debug, step, observe local variables of C# programs by round tripping them: compiling -> disassembling (ildasm) -> assembling(ilasm).

Since portable PDB standard seems to be oriented more towards higher level programming languages, I have tried to define minimum viable set of features that are applicable to ilasm, which are currently supported:

Portable PDB CodeView debug directory entry
Document table
MethodDebugInformation table
Local scope table
Local variable table

Please let me know if some other features are necessary or required, and how would those be expressed in ilasm.

- Currently this just makes a distinction between classic PDB format (not supported on CoreCLR) and portable one (to be added)

…d for portable PDB generation - The idea is to have 2 emitters in ilasm, 1 for system metadata and 1 for pdb metadata

…d for pdb metadata generation

This change extends the IMetaDataEmit2 interface

StateMachineMethod and CustomDebugInformation tables are left out

Extending IMetaDataEmit2 interface

…generation - This can probably be done in a better (more clever) way, check if/why/when this is necessary

Extending IMetaDataEmit2 interface

dnfadmin · 2020-06-10T12:50:27Z

All CLA requirements met.

Fixes build failures under gcc -Wcomment -Werror

davidwrighton

I like the ideas here, and the implementation generally seems reasonable. I have a few concerns though, that I'd like to make sure are handled before we could accept such a change. At

All of the changes in the MD directory need to be under some #ifdef. We could enable that for building ilasm, and probably ildasm, as ildasm should probably be able to parse these things. However, the size of the coreclr binary must not be affected by these changes.
I'd like to avoid changes to in the public headers Cor.h and Corhdr.h. I'd prefer that all new portable pdb handling in the native api is held in a new header or two.
COM rules on interface changing must be respected. This is a minor detail, and easy to fix.
We should support both embedded and non-embedded portable pdbs.
This will need some amount of testing.

davidwrighton · 2020-06-10T17:24:37Z

src/coreclr/src/inc/cor.h

@@ -613,6 +631,41 @@ DECLARE_INTERFACE_(IMetaDataEmit2, IMetaDataEmit)

    STDMETHOD(ResetENCLog)() PURE;          // S_OK or error.

+    STDMETHOD(GetReferencedTypeSysTables)(  // S_OK or error.


These are COM interfaces, which must never change. You will need to create a new COM interface to hold these methods.

Thank you for your insightful feedback.
I am currently fixing some build issues, but will look into introducing new COM interfaces right after that.

@davidwrighton could you please elaborate on how would these new COM interfaces be generated?
I was thinking of creating ppdb.idl file which would define IMetaDataEmit3 interface inheriting from IMetaDataEmit2, but I failed to find an .idl file defining IMetaDataEmit2

These COM interface headers were written by hand. I would recommend following the pattern in cor.h to make another one.

For instance,

// {7DD2F810-CAD1-419F-9964-82912CAC3CD4} EXTERN_GUID(IID_IMetaDataEmit3, 0x7DD2F810, 0xCAD1, 0x419F, 0x99, 0x64, 0x82, 0x91, 0x2C, 0xAC, 0x3C, 0xD4); //--- #undef INTERFACE #define INTERFACE IMetaDataEmit3 DECLARE_INTERFACE_(IMetaDataEmit3, IMetaDataEmit2) { // Insert new api's here! };

Note, that I have generated a new guid by using the uuidgen tool. Each guid is associated represents a unique interface.

@davidwrighton could you please check the changes from my last 3 commits?

Based on your previous comments, I have done the following:

Introduced 2 new COM interfaces: IMetaDataEmit3 and IMetaDataDispenserEx2 found in .\md\inc\portablepdbmdi.h

Removed all changes from cor.h and corhdr.h

Introduced new preprocessor directive FEATURE_METADATA_EMIT_PORT_PDB which now surrounds all the changes related to portable PDB metadata generation

Adjusted make files to generate portable PDB metadata support only for ilasm (keeping coreclr intact)

Let me know what do you think about this, and what will be the next steps.

noahfalk · 2020-06-11T06:04:48Z

Hi @ivanpovazan - thanks so much for your efforts here! It might be another day or two before I can dig into this properly.
heads up @tmat and @MichalStrehovsky, you guys might be interested in taking a look at this as well?

src/coreclr/src/md/enc/pdbheap.cpp

noahfalk · 2020-06-13T03:14:56Z

@janvorli and @davidwrighton - I've had plenty of work continue to come in and I see both of you already reviewed. Let me know if you think it would valuable for me to be a 3rd reviewer here.

…tion All changes related to portable PDB metadata generation should be controlled with preprocessor directive FEATURE_METADATA_EMIT_PORT_PDB

…DATA_EMIT_PORT_PDB preprocessor directive

davidwrighton · 2020-06-26T18:54:37Z

@ivanpovazan I've looked through the change, and I believe it is good enough to checkin, and there are no more meaningful issues to address before checkin with regards to changes to the product code. I am however a bit concerned that it might regress some existing ILasm functionality as we have very few tests for ilasm correctness at this time.

@BruceForstall do you have any ideas on how to regresssion test this? I know we've done some ildasm/ilasm roundtripping testing in the past. Is that a reasonable thing to run?

src/coreclr/src/ilasm/CMakeLists.txt

src/coreclr/src/ilasm/assembler.h

src/coreclr/src/ilasm/main.cpp

BruceForstall · 2020-06-26T22:07:51Z

src/coreclr/src/ilasm/portable_pdb.cpp

+    memset(m_pdbStream.typeSystemTableRows, 0, sizeof(ULONG) * TBL_COUNT);
+    time_t now;
+    time(&now);
+    m_pdbStream.id.pdbTimeStamp = (ULONG)now;


Do we really want the timestamp in the generated PDB file? That makes the generated file not binary equivalent on equivalent source rebuild. I thought we're trying to get away from timestamps in generated files because of this problem.

BruceForstall · 2020-06-26T22:08:23Z

src/coreclr/src/ilasm/portable_pdb.cpp

+    time_t now;
+    time(&now);
+    m_pdbStream.id.pdbTimeStamp = (ULONG)now;
+    hr = CoCreateGuid(&m_pdbStream.id.pdbGuid);


Maybe the same this applies to this GUID.

ivanpovazan · 2020-07-02T23:09:57Z

@davidwrighton @BruceForstall my last commit includes some functional testing related to portable PDB metadata generation for ilasm.

It is true that testing would be even more complete when ildasm supports portable PDB, but I believe I covered significant part of what has been added for portable PDB support.

What I think would be valuable for you (or someone who will perform manual debugger tests) can be found at:
https://github.com/ivanpovazan/runtime/tree/ilasm_portable_pdb/src/coreclr/tests/src/ilasm/PortablePdb/Resources
where I have added a short description of how you can use one of the test files to verify debugging C# source with a debugger.

…olling portable PDB metadata support

…r full names

ivanpovazan · 2020-07-06T10:30:36Z

I have also fixed the naming issues reported by @BruceForstall.
Can I mark this as 'Ready for review' so this work can be treated as done?

davidwrighton · 2020-07-07T22:30:02Z

Please feel free to mark this as ready for review. I think the testing strategy here is good. I'm not sure the tests are in the right place, but I don't have a better place for them quite yet.

ivanpovazan · 2020-07-08T07:52:31Z

@davidwrighton thank you, I have marked it as ready for review but there is still a change request open.

noahfalk · 2020-07-08T09:57:10Z

PDB timestamp and GUID: I was just following the specification of debug directory and portable PDB format.

Let me know what do you think about this.

This looks fine to me. I didn't notice anything in the portable pdb spec that said the 20 byte id needed to be formed from a GUID and a timestamp, but that behavior matches SRM's implementation when it is operating in non-deterministic mode.

ivanpovazan · 2020-07-08T10:05:48Z

@noahfalk yes you are right, GUID and timestamp requirement do not come from portable PDB spec, that is why I linked the spec of debug directory as well, which states:

Version Major=any, Minor=0x504d of the data format has the same structure as above. The Age shall be 1. The format of the .pdb file that this PE/COFF file was built with is Portable PDB. The Major version specified in the entry indicates the version of the Portable PDB format. Together 16B of the Guid concatenated with 4B of the TimeDateStamp field of the entry form a PDB ID that should be used to match the PE/COFF image with the associated PDB (instead of Guid and Age). Matching PDB ID is stored in the #Pdb stream of the .pdb file.

noahfalk · 2020-07-08T11:17:12Z

that is why I linked the spec of debug directory as well...

I have failed at reading it appears, sorry about that : )

noahfalk

This looks really good to me, thanks for your all your hard work here! Just one comment inline about the verification macros I'd like to fix up.

noahfalk · 2020-07-08T10:26:48Z

src/coreclr/src/md/enc/liteweightstgdbrw.cpp

@@ -554,6 +563,24 @@ HRESULT CLiteWeightStgdbRW::GetSaveSize(// S_OK or error.
        }
    }

+#ifdef FEATURE_METADATA_EMIT_PORTABLE_PDB
+    // [IMPORTANT]:
+    // Apparently, string pool must exist in the portable PDB metadata in order to be recognized


@gregg-miskelly @tmat - fyi for you guys. This workaround seems fine to me but let us know if you have any concerns.

src/coreclr/src/md/inc/VerifyLayouts.inc

…library variations except for mdcompiler_wks

…part of test suite

noahfalk

LGTM!

ivanpovazan · 2020-07-10T13:47:58Z

Any idea how I could track down the unsuccessful checks?

.packages\microsoft.dotnet.helix.sdk\5.0.0-beta.20330.3\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets#L76
.packages\microsoft.dotnet.helix.sdk\5.0.0-beta.20330.3\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(76,5): error : (NETCORE_ENGINEERING_TELEMETRY=Test) Work item e58d5f52-74bf-4657-85be-7893ea3587d9/System.Collections.Immutable in job e58d5f52-74bf-4657-85be-7893ea3587d9 has failed.
Failure log: https://helix.dot.net/api/2019-06-17/jobs/e58d5f52-74bf-4657-85be-7893ea3587d9/workitems/System.Collections.Immutable/console

BruceForstall · 2020-07-10T19:12:31Z

@ivanpovazan It appears your change hit the CI in a period of particularly poor reliability. My suggestion is to rebase and repush your change to trigger a new set of runs. (I've not had good luck simply re-triggering from the UI, or close/re-open to attempt to retrigger in cases like this.)

ivanpovazan · 2020-07-10T19:26:55Z

Thank you for the explanation, I will try to retrigger the build

davidwrighton · 2020-07-14T05:16:19Z

I've manually verified that debugging works, and this scheme for testing looks to be good too. Thank you @ivanpovazan for all your hard work.

Ivan Povazan added 11 commits June 10, 2020 14:08

[WIP] Adding new command line argument to specify desired PDB format

e9a267a

- Currently this just makes a distinction between classic PDB format (not supported on CoreCLR) and portable one (to be added)

[WIP] Extending IMetaDataDispenserEx to create a metadata emitter use…

a3653ba

…d for portable PDB generation - The idea is to have 2 emitters in ilasm, 1 for system metadata and 1 for pdb metadata

[WIP] Adding PortablePdbWritter class as a wrapper around emitter use…

0798ac4

…d for pdb metadata generation

[WIP] Adding methods for setting PDB file name and saving PDB file

632f0e9

[WIP] Updating CreateDebugDirectory method to support portable PDB

0dd5f9c

[WIP] Adding PdbStream generation

890d8e4

This change extends the IMetaDataEmit2 interface

[WIP] Adding portable PDB table definitions

34861c6

StateMachineMethod and CustomDebugInformation tables are left out

[WIP] Adding the logic for defining documents

9c4ef5a

Extending IMetaDataEmit2 interface

[WIP] Adding the logic for defining sequence points

da35491

Extending IMetaDataEmit2 interface

[WIP] Make sure String heap is never empty for portable PDB metadata …

7b728e9

…generation - This can probably be done in a better (more clever) way, check if/why/when this is necessary

[WIP] Adding the logic for defining local scopes and local variables

bbe7976

Extending IMetaDataEmit2 interface

Dotnet-GitSync-Bot added the area-VM-coreclr label Jun 10, 2020

jkotas added area-ILTools-coreclr and removed area-VM-coreclr labels Jun 10, 2020

[WIP] Removing line continuation in comments

c86c8c4

Fixes build failures under gcc -Wcomment -Werror

davidwrighton requested changes Jun 10, 2020

View reviewed changes

Ivan Povazan added 3 commits June 11, 2020 16:20

[WIP] Linux build failure fix

466b846

[WIP] Another attempt to fix build issues for different systems

f51d26b

[WIP] Proper use of IfFailGo macro for checked build

8a9b16d

janvorli reviewed Jun 12, 2020

View reviewed changes

src/coreclr/src/md/enc/pdbheap.cpp Outdated Show resolved Hide resolved

Ivan Povazan added 5 commits June 14, 2020 12:13

[WIP] Removing inappropriate use of IfFailGo macro

b99bdf5

[WIP] Introducing new COM interfaces for portable PDB metadata genera…

453d332

…tion All changes related to portable PDB metadata generation should be controlled with preprocessor directive FEATURE_METADATA_EMIT_PORT_PDB

[WIP] Surrounding all portable PDB metadata changes with FEATURE_META…

69e20d6

…DATA_EMIT_PORT_PDB preprocessor directive

[WIP] Enabling only ilasm to use portable PDB metadata generation

5ff964e

[WIP] Fix for UNIX paths when defining documents

586e52d

BruceForstall reviewed Jun 26, 2020

View reviewed changes

Ivan Povazan added 3 commits July 6, 2020 11:31

[WIP] Spelling out PORTABLE correctly in preprocessor directive contr…

a8c2649

…olling portable PDB metadata support

[WIP] Typo: Naming PortablePdbWriter instead of PortablePdbWritter

9c9b3f5

[WIP] Clarifying command line parameters for pdb format by using thei…

a164156

…r full names

ivanpovazan marked this pull request as ready for review July 8, 2020 07:40

noahfalk reviewed Jul 8, 2020

View reviewed changes

Ivan Povazan and others added 4 commits July 8, 2020 14:53

[WIP] Removing dependency on layout verification from all mdcompiler …

89c8519

…library variations except for mdcompiler_wks

Merge branch 'master' into ilasm_portable_pdb

ecc437a

[WIP] Simplifying the makefile for building mdcompiler_wks library

c92c8ec

[WIP] Moving ilasm portable PDB tests to a new location to make them …

703266f

…part of test suite

noahfalk approved these changes Jul 10, 2020

View reviewed changes

Merge branch 'master' into ilasm_portable_pdb

c348528

ivanpovazan requested a review from davidwrighton July 10, 2020 21:54

Merge branch 'master' into ilasm_portable_pdb

5791a51

davidwrighton approved these changes Jul 14, 2020

View reviewed changes

davidwrighton merged commit 99bbb19 into dotnet:master Jul 14, 2020

ivanpovazan mentioned this pull request Jul 14, 2020

PDB support for ilasm #5051

Closed

gbalykov mentioned this pull request Jul 28, 2020

Linux x86 build error #40003

Open

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

ivanpovazan deleted the ilasm_portable_pdb branch December 19, 2020 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portable PDB support for ilasm #37702

Portable PDB support for ilasm #37702

ivanpovazan commented Jun 10, 2020 •

edited

Loading

dnfadmin commented Jun 10, 2020 •

edited

Loading

davidwrighton left a comment

davidwrighton Jun 10, 2020

ivanpovazan Jun 11, 2020

ivanpovazan Jun 13, 2020 •

edited

Loading

davidwrighton Jun 16, 2020

ivanpovazan Jun 17, 2020

noahfalk commented Jun 11, 2020

noahfalk commented Jun 13, 2020

davidwrighton commented Jun 26, 2020

BruceForstall Jun 26, 2020

BruceForstall Jun 26, 2020

ivanpovazan commented Jul 2, 2020 •

edited

Loading

ivanpovazan commented Jul 6, 2020

davidwrighton commented Jul 7, 2020

ivanpovazan commented Jul 8, 2020

noahfalk commented Jul 8, 2020

ivanpovazan commented Jul 8, 2020

noahfalk commented Jul 8, 2020

noahfalk left a comment

noahfalk Jul 8, 2020

noahfalk left a comment

ivanpovazan commented Jul 10, 2020

BruceForstall commented Jul 10, 2020

ivanpovazan commented Jul 10, 2020

davidwrighton commented Jul 14, 2020

		@@ -613,6 +631,41 @@ DECLARE_INTERFACE_(IMetaDataEmit2, IMetaDataEmit)

		STDMETHOD(ResetENCLog)() PURE; // S_OK or error.

		STDMETHOD(GetReferencedTypeSysTables)( // S_OK or error.

Portable PDB support for ilasm #37702

Portable PDB support for ilasm #37702

Conversation

ivanpovazan commented Jun 10, 2020 • edited Loading

dnfadmin commented Jun 10, 2020 • edited Loading

davidwrighton left a comment

Choose a reason for hiding this comment

davidwrighton Jun 10, 2020

Choose a reason for hiding this comment

ivanpovazan Jun 11, 2020

Choose a reason for hiding this comment

ivanpovazan Jun 13, 2020 • edited Loading

Choose a reason for hiding this comment

davidwrighton Jun 16, 2020

Choose a reason for hiding this comment

ivanpovazan Jun 17, 2020

Choose a reason for hiding this comment

noahfalk commented Jun 11, 2020

noahfalk commented Jun 13, 2020

davidwrighton commented Jun 26, 2020

BruceForstall Jun 26, 2020

Choose a reason for hiding this comment

BruceForstall Jun 26, 2020

Choose a reason for hiding this comment

ivanpovazan commented Jul 2, 2020 • edited Loading

ivanpovazan commented Jul 6, 2020

davidwrighton commented Jul 7, 2020

ivanpovazan commented Jul 8, 2020

noahfalk commented Jul 8, 2020

ivanpovazan commented Jul 8, 2020

noahfalk commented Jul 8, 2020

noahfalk left a comment

Choose a reason for hiding this comment

noahfalk Jul 8, 2020

Choose a reason for hiding this comment

noahfalk left a comment

Choose a reason for hiding this comment

ivanpovazan commented Jul 10, 2020

BruceForstall commented Jul 10, 2020

ivanpovazan commented Jul 10, 2020

davidwrighton commented Jul 14, 2020

ivanpovazan commented Jun 10, 2020 •

edited

Loading

dnfadmin commented Jun 10, 2020 •

edited

Loading

ivanpovazan Jun 13, 2020 •

edited

Loading

ivanpovazan commented Jul 2, 2020 •

edited

Loading