Skip to content

Fix unaligned memory access errors in TraceEvent - #1104

Merged
brianrob merged 2 commits into
microsoft:masterfrom
swift-kim:unaligned
Mar 3, 2020
Merged

Fix unaligned memory access errors in TraceEvent#1104
brianrob merged 2 commits into
microsoft:masterfrom
swift-kim:unaligned

Conversation

@swift-kim

@swift-kim swift-kim commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

This fixes unaligned read errors (SIGBUS) caused by unmanaged pointer operations on ARM platforms. The Marshal.Read*() operations are portable and have little performance impacts.

@sharwell

sharwell commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

The JIT output seems very different (reference):

public unsafe double M1(IntPtr pointer, int offset) {
    return *((double*)((byte*)pointer.ToPointer() + offset));
}
C.M1(IntPtr, Int32)
    L0000: vzeroupper
    L0003: movsxd rax, r8d
    L0006: vmovsd xmm0, qword [rdx+rax]
    L000b: ret
public unsafe double M2(IntPtr pointer, int offset) {
    var val = Marshal.ReadInt64(pointer, offset);
    return *(double*)&val;
}
C.M2(IntPtr, Int32)
    L0000: sub rsp, 0x28
    L0004: vzeroupper
    L0007: mov rcx, rdx
    L000a: mov edx, r8d
    L000d: call System.Runtime.InteropServices.Marshal.ReadInt64(IntPtr, Int32)
    L0012: mov [rsp+0x20], rax
    L0017: vmovsd xmm0, qword [rsp+0x20]
    L001d: add rsp, 0x28
    L0021: ret

@sharwell

sharwell commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

This option seems better:

public unsafe double M3(IntPtr pointer, int offset) {
    return Unsafe.ReadUnaligned<double>(IntPtr.Add(pointer, offset).ToPointer());
}
C.M3(IntPtr, Int32)
    L0000: vzeroupper
    L0003: movsxd rax, r8d
    L0006: add rax, rdx
    L0009: vmovsd xmm0, qword [rax]
    L000d: ret

@sharwell sharwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should use Unsafe.ReadUnaligned<T> to avoid the overhead of Marshal.Read*.

@swift-kim

Copy link
Copy Markdown
Contributor Author

@sharwell Thanks for the review and introducing a good tool.

  • I didn't actually consider Unsafe.* because it required an additional package dependency. If you don't mind, I'll add it.

  • I replaced IntPtr.Add(pointer, offset).ToPointer()(byte*)pointer.ToPointer() + offset for consistency with other methods. They are equivalent according to the IL compiler you referenced.

Comment thread src/TraceEvent/TraceEvent.csproj Outdated
Comment thread src/TraceEvent/TraceEvent.csproj Outdated

<!-- *** SourceLink Support *** -->
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0-beta-63127-02" PrivateAssets="All" />
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="4.5.2" />

@sharwell sharwell Feb 7, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@brianrob @adamsitnik Please review this added dependency. IMO it is a clear win.

Comment thread src/TraceEvent/TraceEvent.csproj Outdated
- System.Runtime.CompilerServices.Unsafe is added as a new dependency.

@sharwell sharwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Still needs approval from @brianrob .

@swift-kim

swift-kim commented Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

For reminder: @sharwell @brianrob, could you take a look?

@brianrob

brianrob commented Mar 3, 2020

Copy link
Copy Markdown
Member

@swift-kim, thanks for contributing this. This looks OK to me - just want to confirm that you've confirmed that the issue is fixed with the latest version of the PR, since you switched APIs?

@swift-kim

Copy link
Copy Markdown
Contributor Author

@brianrob Yes, I've confirmed dotnet-counters.dll now works in our production system. I'll report if I ever find any other issue. Thank you.

@brianrob
brianrob merged commit d125d41 into microsoft:master Mar 3, 2020
@brianrob

brianrob commented Mar 3, 2020

Copy link
Copy Markdown
Member

Thanks @swift-kim!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants