Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Changes for issue #10595. Handle assembly references through alias in the correct way.#10878

Closed
rartemev wants to merge 3 commits into
dotnet:masterfrom
rartemev:ilasm_reference_aliasing_fix
Closed

Changes for issue #10595. Handle assembly references through alias in the correct way.#10878
rartemev wants to merge 3 commits into
dotnet:masterfrom
rartemev:ilasm_reference_aliasing_fix

Conversation

@rartemev
Copy link
Copy Markdown

@rartemev rartemev commented Apr 11, 2017 •
edited
Loading

Changes for issue #10595. Handle assembly references through alias in the correct way.

ildasm output for sample from issue description.

//  Microsoft (R) .NET Framework IL Disassembler.  Version 4.5.22220.0

// Metadata version: v4.0.22220
.assembly extern System.Runtime
{
  .ver 0:0:0:0
}
.assembly TestAssembly
{
  .ver 0:0:0:0
}
.module test1.dll
// MVID: {FA8E4A92-EB5D-4875-9A46-218805862D57}
.imagebase 0x00400000
.file alignment 0x00000200
.stackreserve 0x00100000
.subsystem 0x0003       // WINDOWS_CUI
.corflags 0x00000001    //  ILONLY
// Image base: 0x00000226F5820000

// ================== GLOBAL METHODS =========================

.method privatescope static void  Main$PST06000001() cil managed
{
  .entrypoint
  // Code size       7 (0x7)
  .maxstack  8
  IL_0000:  newobj     instance void [System.Runtime]System.Object::.ctor()
  IL_0005:  pop
  IL_0006:  ret
} // end of global method Main


// =============================================================

// *********** DISASSEMBLY COMPLETE ***********************

@rartemev
Copy link
Copy Markdown
Author

rartemev commented Apr 11, 2017

@mletterle @RussKeldorph Please review

@mletterle
Copy link
Copy Markdown

mletterle commented Apr 11, 2017

@dotnet-bot test

@mletterle
Copy link
Copy Markdown

mletterle commented Apr 11, 2017

I like it! Looks good to me.

@RussKeldorph
Copy link
Copy Markdown

RussKeldorph commented Apr 11, 2017

@dotnet/jit-contrib

RussKeldorph
RussKeldorph reviewed Apr 11, 2017
View reviewed changes
Comment thread src/ilasm/assembler.cpp Outdated
}
else // needs to be resolved
if(!m_fIsMscorlib) tkResScope = GetBaseAsmRef();
else { // needs to be resolved
Copy link
Copy Markdown

@RussKeldorph RussKeldorph Apr 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please match bracing style (opening { on next line). Applies to next line as well.

Copy link
Copy Markdown
Author

@rartemev rartemev Apr 11, 2017

Choose a reason for hiding this comment

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

Done

@rartemev rartemev force-pushed the ilasm_reference_aliasing_fix branch from de5ce37 to 2bbeec1 Compare April 11, 2017 20:56
RussKeldorph
RussKeldorph reviewed Apr 11, 2017
View reviewed changes
Comment thread src/ilasm/assembler.cpp Outdated
if(!m_fIsMscorlib) tkResScope = GetBaseAsmRef();
else
{ // needs to be resolved
if (TypeFromToken(tkResScope) == mdtAssemblyRef && RidFromToken(tkResScope) > 0)
Copy link
Copy Markdown

@RussKeldorph RussKeldorph Apr 11, 2017

Choose a reason for hiding this comment

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

Should the second && subexpression be !IsNilToken(tkResScope)? Inverse below.

Copy link
Copy Markdown
Author

@rartemev rartemev Apr 11, 2017

Choose a reason for hiding this comment

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

Fixed

RussKeldorph
RussKeldorph reviewed Apr 11, 2017
View reviewed changes
Comment thread src/ilasm/assembler.cpp
if (TypeFromToken(tkResScope) == mdtAssemblyRef && RidFromToken(tkResScope) > 0)
{
// considering the aliased case '[' dottedName ']' slashedName
AsmManAssembly* assembly = m_pManifest->m_AsmRefLst.PEEK(RidFromToken(tkResScope) - 1);
Copy link
Copy Markdown

@RussKeldorph RussKeldorph Apr 11, 2017

Choose a reason for hiding this comment

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

I'm sorry I'm not terribly familiar with ILAsm, so please forgive me if these are naive questions:

  1. Is the RID guaranteed to be in m_AsmRefLst, i.e. do you need to check the return value of PEEK before passing it to GetAsmRef?
  2. Is it possible to have multiple indirections through aliasing such that you might need to iterate until you find the non-alias?

Copy link
Copy Markdown
Author

@rartemev rartemev Apr 11, 2017

Choose a reason for hiding this comment

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

  1. Sure, the first condition "TypeFromToken(tkResScope) == mdtAssemblyRef" guarantees this fact.
  2. I do not know. I am not familiar with ILAsm either. If there is someone who knows please help us.

Copy link
Copy Markdown

@RussKeldorph RussKeldorph Apr 18, 2017

Choose a reason for hiding this comment

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

Regarding #2, can you attempt to construct a case with multiple levels of aliases and see what you get? If the syntax doesn't allow you to create an alias of SomethingElse (from original repro code), then I think you may be ok and this change looks good to me.

Copy link
Copy Markdown
Author

@rartemev rartemev Apr 19, 2017

Choose a reason for hiding this comment

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

Updated

@rartemev rartemev force-pushed the ilasm_reference_aliasing_fix branch 2 times, most recently from 876e998 to 1bd8b36 Compare April 11, 2017 22:52
@danmoseley
Copy link
Copy Markdown
Member

danmoseley commented Apr 12, 2017

@dotnet-bot test Tizen armel Cross Debug Build (biuld break)
@dotnet-bot test Tizen armel Cross Release Build (biuld break)

@rartemev rartemev force-pushed the ilasm_reference_aliasing_fix branch 2 times, most recently from 331c11d to 1609ac5 Compare April 18, 2017 23:08
RussKeldorph
RussKeldorph reviewed Apr 19, 2017
View reviewed changes
Comment thread src/ilasm/assembler.cpp Outdated
if(!m_fIsMscorlib) tkResScope = GetBaseAsmRef();
else
{ // needs to be resolved
if ((TypeFromToken(tkResScope) == mdtAssemblyRef) && !IsNilToken(tkResScope))
Copy link
Copy Markdown

@RussKeldorph RussKeldorph Apr 19, 2017

Choose a reason for hiding this comment

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

Ok, great. Two more things:

  1. please match the prevailing style of the file where there is no space between if/while and following (. I hate this style, but it's important to match it. We should probably add these ilasm and ildasm to the list of things covered by jit-format, but that's for another change.
  2. please add a regression test somewhere under tests\src\JIT\Regression (unless you can find a better place). You should be able to copy another .il test and paste your source into it.

Copy link
Copy Markdown
Author

@rartemev rartemev Apr 19, 2017

Choose a reason for hiding this comment

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

  1. Fixed
  2. Added regression test into tests\src\Regression\coreclr

@RussKeldorph RussKeldorph mentioned this pull request Apr 19, 2017
Merged
@rartemev rartemev force-pushed the ilasm_reference_aliasing_fix branch from 1609ac5 to f83f353 Compare April 19, 2017 23:46
@rartemev
Copy link
Copy Markdown
Author

rartemev commented Apr 20, 2017

@dotnet-bot test Tizen armel Cross Release Build

@RussKeldorph
Copy link
Copy Markdown

RussKeldorph commented Apr 20, 2017

@dotnet-bot test Windows_NT x64 Debug Build and Test

@rartemev rartemev force-pushed the ilasm_reference_aliasing_fix branch 2 times, most recently from 751c0d8 to 2dd50f9 Compare April 24, 2017 20:41
@rartemev rartemev force-pushed the ilasm_reference_aliasing_fix branch from 2dd50f9 to d78a14c Compare April 24, 2017 20:43
@rartemev rartemev closed this May 9, 2017
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@RussKeldorph RussKeldorph RussKeldorph approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@rartemev @mletterle @RussKeldorph @danmoseley @karelz @dnfclas