Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Absent alignment requirements for the CONTEXT struct #1044

Open
HoShiMin opened this issue Aug 7, 2022 · 25 comments
Open

Absent alignment requirements for the CONTEXT struct #1044

HoShiMin opened this issue Aug 7, 2022 · 25 comments
Assignees
Labels
bug Something isn't working rust Critical for Rust adoption

Comments

@HoShiMin
Copy link

HoShiMin commented Aug 7, 2022

Which crate is this about?

windows, windows-sys

Crate version

No response

Summary

According to the definition in the Windows.h, the CONTEXT struct must be aligned by 16 bytes, but it is not neither in windows_rs, nor in windows_sys (it's just a #[repr(C)] here).
It causes an ERROR_NOACCESS errors in the GetThreadContext()/SetThreadContext() functions.

Toolchain version/configuration

No response

Reproducible example

No response

Crate manifest

No response

Expected behavior

No response

Actual behavior

No response

Additional comments

No response

@HoShiMin HoShiMin added the bug Something isn't working label Aug 7, 2022
@riverar
Copy link
Collaborator

riverar commented Aug 15, 2022

Agree, there's a higher level packing/architecture bug here in metadata, transferring to that repo.

Related:

@riverar riverar transferred this issue from microsoft/windows-rs Aug 15, 2022
@kennykerr
Copy link
Contributor

Any update on this? Just ran into this issue.

@kennykerr kennykerr added the rust Critical for Rust adoption label Nov 14, 2022
@HoShiMin
Copy link
Author

HoShiMin commented Nov 14, 2022

@kennykerr, it doesn't fixed yet as I know, but you could use a wrapper with the alignment requirement. Something like this:

#[repr(align(16))]
#[derive(Default)]
struct AlignedContext {
    ctx: CONTEXT
}

...

let mut ctx = AlignedContext::default();
...
unsafe { GetThreadContext(..., &mut ctx.ctx) };

That's all I could advise for now.

@kennykerr
Copy link
Contributor

This issue has been reported again: #1412

@kennykerr
Copy link
Contributor

Reported again: #1477

We've even had projects stop using windows-rs in part because this issue is not fixed: EmbarkStudios/crash-handling#62

@riverar
Copy link
Collaborator

riverar commented Feb 28, 2023

@mikebattista Just an update: I've been looking into this and haven't found an easy fix yet. (I already tried adding support for per-architecture xxx.manual.cs definitions.) We're going to have to do some deeper work/research to fix this one.

@mikebattista
Copy link
Contributor

@tannergooding is DECLSPEC_ALIGN something that ClangSharp can scrape? Is there a setting we need to enable? I don't see any representation of that currently in the generated C# output.

typedef struct DECLSPEC_ALIGN(16) DECLSPEC_NOINITALL _CONTEXT {

@mikebattista
Copy link
Contributor

I added generate-macro-bindings but in the log output I see the unsupported message below.

Warning (Line 231, Column 9 in C:/win32metadata/generation/WinSDK/RecompiledIdlHeaders/um/winnt.h): Function like macro definition records are not supported: 'DECLSPEC_ALIGN'. Generated bindings may be incomplete.

Is this on your radar to support?

In the absence of this, I guess we'll need to manually decorate structs with this macro at a per-architecture level.

@mikebattista
Copy link
Contributor

dotnet/ClangSharp#409 looks related.

@kennykerr
Copy link
Contributor

Note that packing and alignment are related but different. In MSVC, pack lowers the alignment of a structure while alignas raises the alignment. The trick is that these can generally not be combined so care must be taken for example not to declare a struct with pack that the contains a struct with alignas.

@mikebattista
Copy link
Contributor

What do you expect on the metadata here for CONTEXT?

Just [StructLayout(LayoutKind.Sequential, Pack = 16)] for x64 and ARM64? What about x86?

@tannergooding
Copy link
Member

tannergooding commented Mar 7, 2023

This can't be represented using [StructLayout] as Pack becomes the smaller of "natural packing" and "specified packing" for .NET.

It would need to be a distinct/separate attribute which is only respected by the languages that support specifying custom alignment/packing.

@tannergooding
Copy link
Member

Could maybe set Pack to it anyways; it just won't be respected in .NET and other languages will need to understand this difference when traversing the metadata

@kennykerr
Copy link
Contributor

I'm pretty sure we can't reuse pack. Just capture the alignment as a separate attribute and then have some build validation that ensures it's not mixed in with packing.

@kennykerr
Copy link
Contributor

At a minimum, can you read the __declspec(align(x)) attribute?

Good examples are the CONTEXT struct as well as the SLIST_ENTRY struct.

CONTEXT (x64): align: 16 (no pack)
CONTEXT (arm64): align: 16 (no pack)
CONTEXT (x86): pack: 4 (no align)

SLIST_ENTRY (x64): align: 16 (no pack)
SLIST_ENTRY (arm64): align: 16 (no pack)
SLIST_ENTRY (x86): pack: 4 (no align)

@kennykerr
Copy link
Contributor

Here's a good background on alignment and packing. https://doc.rust-lang.org/reference/type-layout.html

@mikebattista
Copy link
Contributor

Those are all tagged with DECLSPEC_ALIGN which ClangSharp doesn't scrape.

@tannergooding
Copy link
Member

In .NET, alignment impacts allocation addresses while packing impacts field offsets. Alignment is a superset of packing and so alignment=# implies pack=#. However, changing the pack doesn't necessarily impact alignment due to GC constraints. .NET has its own rules for understanding packing and its effectively min(UserSpecifiedPacking, NaturalPacking) and so while you can specify Pack=16, the runtime won't actually give you 16-byte packing if it doesn't have a field which is naturally packed as 16-bytes.

It's also not that ClangSharp doesn't scrape the attribute, it's that it ignores overalignment today because .NET can't respect it.

ClangSharp likely should surface a warning by default on such structs, specifying that it found an overaligned struct and specify what alignment it will be treated with instead. It should also emit some [NativeAlignment(#)] and it should emit this as part of the specified Pack information, even though it won't be respected as it will at least be correctly encoded in metadata then.

There is a separate issue in the above, however, that types such as CONTEXT and SLIST_ENTRY have very different layouts and sometimes even fields between x86, x64, Arm32, and Arm64. Such differences cannot be handled in a single C/C++ AST and require multiple separate passes plus some sort of merging/conflict resolution step to ensure that all the relevant type information exists.

While I can trivially update ClangSharp to emit something like NativeAlignment, encode the "correct" packing information, and emit a warning that the type is not "compatible" with .NET. The same cannot be said for handling types which differ on a per-architecture basis. The simplest thing there is likely to use --with-attribute or --with-remapping to emit the relevant "per-architecture" types or other information as needed. Identifying the impacted types (which can be transitive) likely requires running n different compilations and looking for file changes between each pass.

@kennykerr
Copy link
Contributor

The NativeAlignment attribute sounds perfect. The arch-specific differences shouldn't be an issue since win32metadata already runs the tooling once-per-arch.

@mikebattista
Copy link
Contributor

Yes we already handle multiple passes per architecture and then merging the results back. The problem is there's no record of DECLSPEC_ALIGN in the scraped output so that context is lost. If a NativeAlignment attribute were scraped, it should be persisted during our merge process.

@tannergooding
Copy link
Member

I'm posting here as well for visibility, as I've seen a wrong presumption on several different issues so far.

Packing always matters, even if the struct only has a single field. The following godbolt example gives a more concrete visualization of this.

That is the following code:

#include <Windows.h>
#include <pshpack4.h>

struct MINIDUMP_INCLUDE_MODULE_CALLBACK1 {
    ULONG64 BaseOfImage;
};

#include <poppack.h>

struct MINIDUMP_INCLUDE_MODULE_CALLBACK2 {
    ULONG64 BaseOfImage;
};

struct Test1 {
   BYTE x;
   MINIDUMP_INCLUDE_MODULE_CALLBACK1 y;
};

struct Test2 {
   BYTE x;
   MINIDUMP_INCLUDE_MODULE_CALLBACK2 y;
};

unsigned M1() { return offsetof(Test1, y); }

unsigned M2() { return offsetof(Test2, y); }

Gives you:

unsigned int M1(void) PROC ; M1
  mov eax, 4
  ret 0
unsigned int M1(void) ENDP ; M1

unsigned int M2(void) PROC ; M2
  mov eax, 8
  ret 0
unsigned int M2(void) ENDP ; M2

@kennykerr
Copy link
Contributor

As long as the metadata can capture both pack/align as described by MSVC I'll figure out how to make it work with Rust, I just can't make any headway until the metadata is actually complete and accurate.

@ghost
Copy link

ghost commented Oct 17, 2023

Hello. Are there any updates or this? I would like to add, thanks or all the amazing contributions. Respect.

@HoShiMin
Copy link
Author

So, is there light at the end of the tunnel?

@tannergooding
Copy link
Member

I've been pretty heads down on other work related to .NET 9 and haven't had a chance to update ClangSharp.

Contributions to dotnet/clangsharp are more than welcome to enable this scenario and should overall be fairly straightforward to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rust Critical for Rust adoption
Projects
None yet
Development

No branches or pull requests

6 participants