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

Field offset not being set when offset is 0 on explicit layout types. #105795

Closed
TrueLunacy opened this issue Aug 1, 2024 · 4 comments · Fixed by #105894
Closed

Field offset not being set when offset is 0 on explicit layout types. #105795

TrueLunacy opened this issue Aug 1, 2024 · 4 comments · Fixed by #105894
Labels
area-System.Reflection.Emit bug help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@TrueLunacy
Copy link
Contributor

TrueLunacy commented Aug 1, 2024

Description

Field offsets aren't being set in the IL for types with an offset of zero. This causes the types to fail to load at runtime.

The cause, as far as I can tell, seems to be from lines 641 in the WriteFields function in ModuleBuilderImpl.cs

if (field._offset > 0 && (typeBuilder.Attributes & TypeAttributes.ExplicitLayout) != 0)
{
    AddFieldLayout(handle, field._offset);
}

Reproduction Steps

using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.Loader;

PersistedAssemblyBuilder ab = new PersistedAssemblyBuilder(new AssemblyName("MyAssembly"), typeof(object).Assembly);
ModuleBuilder mob = ab.DefineDynamicModule("MyModule");

TypeBuilder exp = mob.DefineType("ExplicitLayoutType", TypeAttributes.Public | TypeAttributes.Class | TypeAttributes.ExplicitLayout, typeof(ValueType), 8);
var f1 = exp.DefineField("first", typeof(int), FieldAttributes.Public);
f1.SetOffset(0);
var f2 = exp.DefineField("second", typeof(int), FieldAttributes.Public);
f2.SetOffset(4);
exp.CreateType();

TypeBuilder tb = mob.DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class);
tb.DefineField("structfield", exp, FieldAttributes.Public | FieldAttributes.Static);

MethodBuilder meb = tb.DefineMethod("SumMethod", MethodAttributes.Public | MethodAttributes.Static,
                                                        typeof(int), new Type[] { typeof(int), typeof(int) });
ILGenerator il = meb.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldarg_1);
il.Emit(OpCodes.Add);
il.Emit(OpCodes.Ret);

tb.CreateType();

using var stream = new MemoryStream();
ab.Save(stream); 
stream.Seek(0, SeekOrigin.Begin);
Assembly assembly = AssemblyLoadContext.Default.LoadFromStream(stream);
MethodInfo method = assembly.GetType("MyType").GetMethod("SumMethod");
Console.WriteLine(method.Invoke(null, new object[] { 5, 10 }));

Expected behavior

Type loads, program prints 15.

Actual behavior

Unhandled exception. System.TypeLoadException: Could not load type 'ExplicitLayoutType' from assembly 'MyAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' because field 'first' was not given an explicit offset.
   at System.Reflection.RuntimeAssembly.GetTypeCore(QCallAssembly assembly, String typeName, ReadOnlySpan`1 nestedTypeNames, Int32 nestedTypeNamesLength, ObjectHandleOnStack retType)
   at System.Reflection.RuntimeAssembly.GetTypeCore(String typeName, ReadOnlySpan`1 nestedTypeNames, Boolean throwOnError, Boolean ignoreCase)
   at System.Reflection.TypeNameResolver.GetType(String escapedTypeName, ReadOnlySpan`1 nestedTypeNames, TypeName parsedName)
   at System.Reflection.TypeNameResolver.GetSimpleType(TypeName typeName)
   at System.Reflection.TypeNameResolver.Resolve(TypeName typeName)
   at System.Reflection.TypeNameResolver.GetType(String typeName, Boolean throwOnError, Boolean ignoreCase, Assembly topLevelAssembly)
   at Program.<Main>$(String[] args) in Program.cs:line 32

Regression?

No response

Known Workarounds

No response

Configuration

Arch Linux x64, dotnet 9.0.100-rc.1.24381.3

Other information

No response

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Aug 1, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

@buyaa-n buyaa-n added this to the 9.0.0 milestone Aug 1, 2024
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Aug 2, 2024
@buyaa-n buyaa-n added the bug label Aug 2, 2024
@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 2, 2024

The cause, as far as I can tell, seems to be from lines 641 in the WriteFields function in ModuleBuilderImpl.cs

if (field._offset > 0 && (typeBuilder.Attributes & TypeAttributes.ExplicitLayout) != 0)
{
    AddFieldLayout(handle, field._offset);
}

Thank you for reporting this @TrueLunacy! Looks you found the root cause, would you like to offer a fix (with a test repro)

@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Aug 2, 2024
TrueLunacy added a commit to TrueLunacy/dotnet-runtime that referenced this issue Aug 2, 2024
PersistedAssemblyBuilder didn't write field offset correctly when field offset
was 0. This fixes that, and adds a test to ensure the behaviour is working.

Fixes dotnet#105795
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 2, 2024
@TrueLunacy
Copy link
Contributor Author

I've made a pull request with the change and a test for it, but I'm not 100% if that's enough or I've done it right. Hopefully I have!

@buyaa-n buyaa-n closed this as completed in 1c0ce30 Aug 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection.Emit bug help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants