Many tests failed with Verify_FieldOffset 'ExtendedSocketException._endPoint' Field offset 128!=136(actual) || baseOffset 128!=136(actual) #49982
Comments
VincentBu
added
arch-arm64
os-linux
dotnet-issue-labeler
bot
added
area-crossgen2-coreclr
untriaged
|
cc @trylek @dotnet/crossgen-contrib. Possibly due to optimizations which were recently enabled? |
|
This is likely caused by @trylek's change to make our cg2 testing more similar to how we are shipping preview3. Looks to be a small bug in the type layout engine of crossgen2. |
|
(Note that this isn't likely newly introduced, just newly found) |
I hit this while creating a regression test for the issue dotnet#49982. In the initial composite R2R design I proposed slightly different organization of the header tables when the composite image only comprises a single file; later on I removed this special casing as it was confusing and making the format more complicated but apparently I forgot to fix this runtime bit. When there's a composite image with only one component in it, we weren't properly initializing the component header table and so we were unable to resolve any R2R methods for the composite image. Thanks Tomas
I hit this while creating a regression test for the issue #49982. In the initial composite R2R design I proposed slightly different organization of the header tables when the composite image only comprises a single file; later on I removed this special casing as it was confusing and making the format more complicated but apparently I forgot to fix this runtime bit. When there's a composite image with only one component in it, we weren't properly initializing the component header table and so we were unable to resolve any R2R methods for the composite image. Thanks Tomas
|
I finally understand the underlying problem and I believe it exposes a pre-existing debt from the time of composite implementation we need to figure out how to address: the discrepancy is basically between these two lines of code: and runtime/src/coreclr/vm/methodtablebuilder.cpp Line 11059 in 5533388 The Crossgen2 version basically states that anything outside of the current version bubble is "some other big version bubble" which is true for composite framework but not for single-file framework with large bubble turned off. To support the fully general case we'd probably need to have a way of specifying version bubble boundaries for reference assemblies in Crossgen2. At the very least we'd probably need a boolean flag stating whether the outside of the current version bubble is one bubble or not but that won't support hierarchical version bubbles (e.g. ASP.NET composite on top of framework composite). I actually think it shouldn't be rocket science to implement even the general case by using a state machine - perhaps a special option, something like While the implementation on the Crossgen2 side is mostly trivial in all the proposed cases, I'm reluctant to dive into it before I hear what @davidwrighton has to say on the subject. |
|
That's very interesting. I am reluctant to try to specify the shape of the various bubbles, but instead would prefer to make the system a bit more conservative. In general the point of not being in the current version bubble is that the shape of other assemblies version bubbles should be immaterial. In general, if we are able to know the shape of the other assembly version bubble, we should probably be able to include that code in the current version bubble. |
|
Test Failed tests: Error message: |
|
Hello David! Thanks for your feedback. I agree that specifying the shape of external version bubbles is unfortunate and fundamentally undermines the R2R resiliency principles. Let me first describe what exactly happens in this particular case to better demonstrate the underlying problem. The problem is basically SocketException deriving from Win32Exception deriving from Exception via ExternalException and SystemException. Exception by itself has 128 (0x80) bytes (including the method table pointer) on x64. ExternalException and SystemException don't add any data, it's only Win32Exception which adds the native 32-bit HRESULT and that misaligns the class size to 132 (0x84). At runtime, methodtablebuilder for SocketException queries whether base class alignment is needed between SocketException in System.Net.Internals and Win32Exception in System.ComponentModel and that subsequently causes the discrepancy. Today, base alignment is considered to be needed when the framework assemblies are compiled one by one; when the framework is compiled in composite or large version bubble mode, CoreCLR runtime decides that the alignment is not needed. If, in accordance with your feedback, we don't want to specify this framework build detail in every single app with a class deriving from SocketException, I believe we either need to remove the distinction or defer its resolution to the runtime. For "removing the distinction", aligning the base class always would presumably incur memory consumption regression (some class instances would become larger). For dynamic resolution, we would need to make sure base class size information is accessed via helpers; in fact I was under the impression that R2R already implements these helpers - maybe we're just doing something wrong in Thanks Tomas /cc @jkotas |
|
I agree that specifying the shape of external version bubbles would be very unfortunate. The field offset verification logic does not seem to handle this case correctly. Is the problem just in the field offset verification logic; or is the actual code also wrong? |
|
Thanks Jan for your comment. For now my super-simple regression test looks as follows: class Program
{
private class MockEndPoint : EndPoint
{
}
private sealed class ExtendedSocketException : SocketException
{
private readonly EndPoint? _endPoint;
public ExtendedSocketException(EndPoint? endPoint)
: base(0)
{
_endPoint = endPoint;
}
public bool EndPointEquals(EndPoint? endPoint)
{
return _endPoint == endPoint;
}
}
static bool TestExtendedSocketException()
{
EndPoint endPoint = new MockEndPoint();
ExtendedSocketException extendedSocketException = new ExtendedSocketException(endPoint);
Console.WriteLine("ExtendedSocketException: {0}", extendedSocketException.GetType());
return extendedSocketException.EndPointEquals(endPoint);
}
static int Main()
{
Console.WriteLine("Extended socket exception:");
return TestExtendedSocketException() ? 100 : 1;
}
}
The R2RDump disassembly for the bool Program.TestExtendedSocketException()
Handle: 0x06000001
Rid: 1
EntryPointRuntimeFunctionId: 0
Number of RuntimeFunctions: 1
Number of fixups: 4
TableIndex 4, Offset 0000: Program+MockEndPoint (TYPE_HANDLE)
TableIndex 4, Offset 0001: Program+ExtendedSocketException (TYPE_HANDLE)
TableIndex 4, Offset 0002: Program+ExtendedSocketException (FIELD_BASE_OFFSET)
TableIndex 5, Offset 0000: "ExtendedSocketException: {0}" (STRING_HANDLE)
bool Program.TestExtendedSocketException()
Id: 0
StartAddress: 0x000013A0
Size: 123 bytes
UnwindRVA: 0x00001270
Version: 1
Flags: 0x03 EHANDLER UHANDLER
SizeOfProlog: 0x0008
CountOfUnwindCodes: 5
FrameRegister: None
FrameOffset: 0x0
PersonalityRVA: 0x199C
UnwindCode[0]: CodeOffset 0x0008 FrameOffset 0x0000 NextOffset 0x0 Op 40
UnwindCode[2]: CodeOffset 0x0003 FrameOffset 0x0000 NextOffset 0x0 Op RBP(5)
UnwindCode[4]: CodeOffset 0x0001 FrameOffset 0x0000 NextOffset 0x0 Op RDI(7)
Debug Info
Bounds:
Native Offset: 0x0, Prolog, Source Types: StackEmpty
Native Offset: 0x8, IL Offset: 0x0001, Source Types: StackEmpty
Native Offset: 0x1A, IL Offset: 0x0007, Source Types: StackEmpty
Native Offset: 0x42, IL Offset: 0x000e, Source Types: StackEmpty
Native Offset: 0x68, IL Offset: 0x001f, Source Types: StackEmpty
Native Offset: 0x72, IL Offset: 0x0029, Source Types: StackEmpty
Native Offset: 0x72, Epilog, Source Types: StackEmpty
13a0: 57 push rdi
UWOP_PUSH_NONVOL RDI(7) 0
13a1: 56 push rsi
UWOP_PUSH_NONVOL RSI(6) 0
13a2: 55 push rbp
UWOP_PUSH_NONVOL RBP(5) 0
13a3: 53 push rbx
UWOP_PUSH_NONVOL RBX(3) 0
13a4: 48 83 ec 28 sub rsp, 40
UWOP_ALLOC_SMALL 40 0
13a8: ff 15 0a 0d 00 00 call qword ptr [0x20b8] // Program+MockEndPoint (NEW_OBJECT)
13ae: 48 8b f0 mov rsi, rax
13b1: 48 8b ce mov rcx, rsi
13b4: ff 15 0e 0d 00 00 call qword ptr [0x20c8] // void System.Net.EndPoint..ctor() (METHOD_ENTRY_REF_TOKEN)
13ba: ff 15 00 0d 00 00 call qword ptr [0x20c0] // Program+ExtendedSocketException (NEW_OBJECT)
13c0: 48 8b f8 mov rdi, rax
13c3: 48 8b cf mov rcx, rdi
13c6: 33 d2 xor edx, edx
13c8: ff 15 02 0d 00 00 call qword ptr [0x20d0] // void System.Net.Sockets.SocketException..ctor(int) (METHOD_ENTRY_REF_TOKEN)
13ce: 48 8b 1d 2b 0d 00 00 mov rbx, qword ptr [0x2100] // Program+ExtendedSocketException (FIELD_BASE_OFFSET)
13d5: 48 8d 0c 1f lea rcx, [rdi + rbx]
13d9: 48 8b d6 mov rdx, rsi
13dc: ff 15 be 0c 00 00 call qword ptr [0x20a0] // WRITE_BARRIER (HELPER)
13e2: 48 8b 0d 1f 0d 00 00 mov rcx, qword ptr [0x2108] // "ExtendedSocketException: {0}" (STRING_HANDLE)
13e9: 48 8b 29 mov rbp, qword ptr [rcx]
13ec: 48 8b cf mov rcx, rdi
13ef: 4c 8d 1d 82 0c 00 00 lea r11, [0x2078] // System.Type System.Exception.GetType() (VIRTUAL_ENTRY)
13f6: ff 15 7c 0c 00 00 call qword ptr [0x2078] // System.Type System.Exception.GetType() (VIRTUAL_ENTRY)
13fc: 48 8b d0 mov rdx, rax
13ff: 48 8b cd mov rcx, rbp
1402: ff 15 d8 0c 00 00 call qword ptr [0x20e0] // void System.Console.WriteLine(string, object) (METHOD_ENTRY_REF_TOKEN)
1408: 48 39 34 1f cmp qword ptr [rdi + rbx], rsi
140c: 0f 94 c0 sete al
140f: 0f b6 c0 movzx eax, al
1412: 48 83 c4 28 add rsp, 40
1416: 5b pop rbx
1417: 5d pop rbp
1418: 5e pop rsi
1419: 5f pop rdi
141a: c3 ret
According to the disassembly I believe the codegen is correct and the field layout verification is "overzealous" in the sense that in these cases it should probably only verify the relative field offset to the base. I'll investigate in more detail how exactly we're producing these fixups and whether I might be able to adjust the logic as appropriate. I suspect that a "rough-hewn" solution is to disable field layout checks in this case, a more precise solution would probably require either creating another fixup or defining some special form of the existing type / field verification fixup (e.g. by specifying base class size = 0) that would only cater for the relative differences and ignore the superclass - in accordance with your feedback from previous issues of this type I believe we shouldn't be querying for / verifying the layouts in version bubbles external to the current compilation. |
|
@trylek yes, that sounds like a good approach. In the meantime, if this is going to take you a while to enable, we should just disable generation of the field layout checks for the test binaries so that we can get our test system off of the ground. |
ghost
added
the
in-pr
|
Failed again in runtime-coreclr crossgen2 20210405.2 Failed test: Error message: |
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
|
Failed again in runtime-coreclr outerloop 20210616.5 Failed test: Error message: |
|
@VincentBu This issue is closed. Could you please open a new one? |
Opened a new one here 54316 , thanks for reminding |
Run: runtime-coreclr crossgen2 20210320.1
One of error message:
The text was updated successfully, but these errors were encountered: