Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Fix getClassAlignmentRequirement for arm 32-bit architectures#3671

Merged
jkotas merged 1 commit intodotnet:masterfrom
BredPet:master
May 22, 2017
Merged

Fix getClassAlignmentRequirement for arm 32-bit architectures#3671
jkotas merged 1 commit intodotnet:masterfrom
BredPet:master

Conversation

@BredPet
Copy link
Contributor

@BredPet BredPet commented May 22, 2017 •
edited
Loading

This change need due to crash on 32-bit ARM architectures.

Backtrace:

Unhandled Exception: System.NotImplementedException: getClassAlignmentRequirement
at Internal.JitInterface.CorInfoImpl.getClassAlignmentRequirement(CORINFO_CLASS_STRUCT_* cls, Boolean fDoubleAlignHint)
at Internal.JitInterface.CorInfoImpl._getClassAlignmentRequirement(IntPtr thisHandle, IntPtr* ppException, CORINFO_CLASS_STRUCT_* cls, Boolean fDoubleAlignHint)
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at Internal.JitInterface.CorInfoImpl.CompileMethod(IMethodCodeNode methodCodeNodeNeedingCode, MethodIL methodIL)
at ILCompiler.RyuJitCompilation.ComputeDependencyNodeDependencies(List`1 obj)
at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeDependencies(List`1 deferredStaticDependencies)
at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes()
at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.get_MarkedNodeList()
at ILCompiler.RyuJitCompilation.CompileInternal(String outputFile, ObjectDumper dumper)
at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String outputFile, ObjectDumper dumper)
at ILCompiler.Program.Run(String[] args)
at ILCompiler.Program.Main(String[] args)

Signed-off-by: UIDL YGEN uidlygen@yandex.ru

@dnfclas
Copy link

dnfclas commented May 22, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@BredPet
Copy link
Contributor Author

BredPet commented May 22, 2017

@jkotas please review

jkotas
jkotas suggested changes May 22, 2017
View reviewed changes
src/JitInterface/src/CorInfoImpl.cs Outdated
private uint getClassAlignmentRequirement(CORINFO_CLASS_STRUCT_* cls, bool fDoubleAlignHint)
{ throw new NotImplementedException("getClassAlignmentRequirement"); }
{
if (_compilation.TypeSystemContext.Target.Architecture == TargetArchitecture.ARM ||
Copy link
Member

@jkotas jkotas May 22, 2017

Choose a reason for hiding this comment

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

This should not be special cased for ARM. The implementation should work everywhere.

Copy link
Contributor Author

@BredPet BredPet May 22, 2017

Choose a reason for hiding this comment

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

I have the same opinion, I just test only on ARM. Therefore, I decided not to change the usual logic.
But if you think that this change need and can be extended to other architectures, I think it will be better.

src/JitInterface/src/CorInfoImpl.cs Outdated
_compilation.TypeSystemContext.Target.Architecture == TargetArchitecture.ARMEL)
{
// Default alignment is sizeof(void*)
uint result = (uint)sizeof(void*);
Copy link
Member

@jkotas jkotas May 22, 2017

Choose a reason for hiding this comment

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

sizeof(void*) is the size of the pointer of the host where the compiler is running. Instead, this should be Target.PointerSize - size of the pointer on the target.

Copy link
Contributor Author

@BredPet BredPet May 22, 2017

Choose a reason for hiding this comment

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

Ok

src/JitInterface/src/CorInfoImpl.cs Outdated
// Default alignment is sizeof(void*)
uint result = (uint)sizeof(void*);
var type = HandleToObject(cls);
MetadataType typeDef = type.GetTypeDefinition() as MetadataType;
Copy link
Member

@jkotas jkotas May 22, 2017

Choose a reason for hiding this comment

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

The alignment lives on DefType, so you should be just casting to DefType here - without the rest. Also, the fallback to pointer-size alignment should not happen, and it is probably incorrect if it happens. It may better to just let it throw exception so that we noticed. Taking these two together makes the implementation:

return ((DefType)type).InstanceByteAlignment.AsInt;

Copy link
Contributor Author

@BredPet BredPet May 22, 2017

Choose a reason for hiding this comment

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

Ok, thanks

@BredPet
Fix getClassAlignmentRequirement for arm 32-bit architectures
90457e8 
Signed-off-by: UIDL YGEN <uidlygen@yandex.ru>
@BredPet
Copy link
Contributor Author

BredPet commented May 22, 2017

@Dmitri-Botcharnikov
@sergign60
@jkotas please review again

jkotas
jkotas approved these changes May 22, 2017
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkotas jkotas merged commit 744d42e into dotnet:master May 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jkotas jkotas jkotas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BredPet @dnfclas @jkotas