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.

Sign up for GitHub

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

Jump to bottom

Reflection.Emit does not reliably allow use of CreateSpan<T> with non 8 byte aligned data #62314

Closed
davidwrighton opened this issue Dec 3, 2021 · 1 comment · Fixed by #63430
Closed

Reflection.Emit does not reliably allow use of CreateSpan<T> with non 8 byte aligned data #62314

davidwrighton opened this issue Dec 3, 2021 · 1 comment · Fixed by #63430
Labels
area-System.Reflection.Emit
Milestone
7.0.0

Comments

@davidwrighton
Copy link
Member

Description

The emitter for FieldRVA data used in Reflection.Emit does not 8 byte align field RVA fields when the data may have 8 byte alignment requiring data. This interferes with compatibility with the CreateSpan<T> feature. See #60948.

Reproduction Steps

Use reflection emit to allocate RVA static fields. They will not be 8 byte aligned.

See

runtime/src/coreclr/vm/commodule.cpp

Line 509 in c87e932

IfFailThrow( pGen->GetSectionBlock(pReflectionModule->m_sdataSection, length, sizeof(DWORD), (void**) &pvBlob) );

where the data is DWORD aligned (4 bytes), not ever 8 byte aligned.

Expected behavior

If the data potentially contains an 8 byte sized value, the data should be 8 byte aligned.

Actual behavior

The data will only be 4 byte aligned as only 4 byte alignment is requested o fthe ICeeGenInternal api.

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 3, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@davidwrighton davidwrighton added this to the 7.0.0 milestone Dec 3, 2021
@davidwrighton davidwrighton mentioned this issue Jan 6, 2022
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 6, 2022
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Jan 11, 2022
davidwrighton added a commit that referenced this issue Jan 27, 2022
@davidwrighton
Align rva fields in reflection emit (#63430)
014b084 
In support of CreateSpan (#60948), improve alignment for RVA static pre-initialized fields to align memory blocks which may contain long, ulong, or double primitive arrays on 8 byte boundaries.

Mono fix is more involved
- Fix Ref-Emit generated RVA statics
 - Set the HasFieldRVA bit. NOTE: earlier code that attempts to set the bit doesn't actually do that as the FieldRVA bit is in  FieldAttributes.ReservedMask which is masked away in the FieldBuilder constructor
 - Fix the Swizzle lookup. We should not be using the 8 byte swizzle in the final else clause.
- Enhance ref-emitted field to allow for use with CreateSpan
  - Ref-emitted fields should specify a pack size appropriate for minimum alignment of the CreateSpan targetted data
  - Respect the packing_size specified so that RVA static fields generated for CreateSpan can be properly aligned

Fixes #62314
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-System.Reflection.Emit
Projects
None yet
Milestone
7.0.0
Development

Successfully merging a pull request may close this issue.

Align rva fields in reflection emit davidwrighton/runtime
2 participants
@jeffschwMSFT @davidwrighton