Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Can Method::DoesTryHandlerPointToOffset ever return true? #757

Open
3 of 24 tasks
SteveGilham opened this issue Aug 29, 2017 · 3 comments
Open
3 of 24 tasks

Can Method::DoesTryHandlerPointToOffset ever return true? #757

SteveGilham opened this issue Aug 29, 2017 · 3 comments
Labels

Comments

@SteveGilham
Copy link
Contributor

My Framework

  • .NET 2
  • .NET 3.5
  • .NET 4
  • .NET 4.5
  • .NET 4.6
  • .NET 4.6.1
  • .NET 4.6.2
  • .NET Core 1.0.0
  • Something else .net 4.7

My Environment

  • Windows 7 or below (not truly supported due to EOL)
  • Windows 8
  • Windows 8.1
  • Windows 10 (1703 Home)
  • Windows 10 IoT Core
  • Windows Server 2012
  • Windows Server 2012 R2
  • Windows Server 2016

I have already...

  • repeated the problem using the latest stable release of OpenCover.
  • reviewed the usage guide and usage document.
  • have looked at the opencover output xml file in an attempt to resolve the issue.
  • reviewed the current issues to check that the issue isn't already known.

My issue is related to (check only those which apply):

  • no coverage being recorded
  • 32 or 64 bit support
  • feature request

Expected Behavior

I can write a unit test based on the sample code in issue #663 to exercise what is currently the drop-through branch of Method::InsertInstructionsAtOriginalOffset -- having some real live IL for which the test condition, to wit

(i->m_handlerType == COR_ILEXCEPTION_CLAUSE_NONE
	&& i->m_handlerStart->m_offset == offset
	&& i->m_handlerStart->m_operand == CEE_THROW)

is true for some ExceptionHandler *i -- though the use of m_operand looked suspicious from the start.

Actual Behavior

Building the sample code from issue #663 in release configuration and then unpacking with ILDASM, I created the following stub test

TEST_F(InstrumentationTest, CanInsertInstructionAtExceptionHandler)
{
	BYTE data[] = {
		0x00, 0x00, 0x00, 0x00,
		0x00, 0x00, 0x00, 0x00,
		0x00, 0x00, 0x00, 0x00,
		0x1F, 0x2A,						// ldc.i4.s   42
		0x73, 0x0A, 0x00, 0x00, 0x0F,                  // newobj     instance void[mscorlib]System.Decimal::.ctor(int32)
		0x0A,							// stloc.0
		0xDE, 0x01,						// leave.s    IL_000b
		0x7A,							// throw
		0x06,							// ldloc.0
		0x2A,							// ret

		0x00, 0x00, 0x00,// align
		0x01, 0x0C, 0x00, 0x00,

		// HEX: 00 00 00 00 0A 0A 00 01 12 00 00 01
		0x00, 0x00, 0x00, 0x00, 0x0A, 0x0A, 0x00, 0x01, 0x12, 0x00, 0x00, 0x01
	};

	auto pHeader = reinterpret_cast<IMAGE_COR_ILMETHOD_FAT*>(data);
	pHeader->Flags = CorILMethod_FatFormat | CorILMethod_MoreSects;
	pHeader->CodeSize = static_cast<DWORD>(13);
	pHeader->Size = 3;

	Method instrument(reinterpret_cast<IMAGE_COR_ILMETHOD*>(data));

	InstructionList instructions;
	instructions.push_back(new Instruction(CEE_NOP, 0));

	instrument.InsertInstructionsAtOriginalOffset(10, instructions);

	// TODO -- Assert that the new code comes after offset 10, which is unchanged
}

Debugging through this I see that, as I quite suspected, I have one exception handler record with i->m_handlerStart->m_operation == CEE_THROW but i->m_handlerStart->m_operand == 0; and Method::DoesTryHandlerPointToOffset returns false again.

Steps to reproduce the problem:

As above. I expect this to be a simple typo, probably induced by "helpful" autocompletion.

SteveGilham added a commit to SteveGilham/opencover that referenced this issue Aug 29, 2017
@SteveGilham
Copy link
Contributor Author

SteveGilham commented Aug 29, 2017

At the moment, I'm just looking for a sanity check here. If there were a Slack channel (which I am told does send alerts to the phone app) rather than a dusty and dying gitter, I would have asked there.

@sawilde
Copy link
Member

sawilde commented Aug 29, 2017

I did look at slack and I use it all the time but it is invite only AFAIK so it isn't very community friendly - I may have another look and see if we can make the process better

@SteveGilham
Copy link
Contributor Author

Back on topic --

Having pondered it overnight, it didn't seem to make sense to put/leave the instrumentation after a throw, so I put it to the test, by running the example from OpenCoverOptimizedCodeBug.zip in issue #663 with the same C# compiler and NUnit version but OpenCover from

the results were, as expected, identical and successful.

I suspect the answer to my question that forms the topic title is that it would do so only by coincidence, and in any case that it shouldn't.


For the record, the IL generated by the 14.0 compiler with the given settings is slightly different to that quoted above (which was standard release build in VS2017 v15.3.2)

.method public hidebysig static valuetype [mscorlib]System.Decimal 
        GetAnswer() cil managed
// SIG: 00 00 11 19
{
  // Method begins at RVA 0x2050
  // Code size       13 (0xd)
  .maxstack  1
  .locals init ([0] valuetype [mscorlib]System.Decimal V_0)
  .try
  {
    IL_0000:  /* 1F   | 2A               */ ldc.i4.s   42
    IL_0002:  /* 73   | (0A)000004       */ newobj     instance void [mscorlib]System.Decimal::.ctor(int32)
    IL_0007:  /* 0A   |                  */ stloc.0
    IL_0008:  /* DE   | 01               */ leave.s    IL_000b
  }  // end .try
  catch [mscorlib]System.Exception 
  {
    IL_000a:  /* 7A   |                  */ throw
  }  // end handler
  // HEX: 00 00 00 00 0A 0A 00 01 07 00 00 01
  IL_000b:  /* 06   |                  */ ldloc.0
  IL_000c:  /* 2A   |                  */ ret
} // end of method Thing::GetAnswer

with a different handle for the Decimal object and 0x07 rather than 0x12 in the 9th byte of the handler, but identical m_handerStart when that IL is debugged through in the nascent unit-test above.

@sawilde sawilde added the medium label Jan 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants