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

Big-endian fixes: various problems in ilasm #55349

Merged
merged 1 commit into from
Jul 22, 2021
Merged

Big-endian fixes: various problems in ilasm #55349

merged 1 commit into from
Jul 22, 2021

Conversation

uweigand
Copy link
Contributor

@uweigand uweigand commented Jul 8, 2021

While debugging problems running the roslyn test suite, I noticed a few more big-endian problems in ilasm:

  • Byte-swap property and parameter default values of string type
    Default values of string type are swapped to little-endian by the parser (fieldInit). This has to be done there since "bytearray" values (which are provided by the user in little-endian format) are also encoded as string type, so we won't be able to tell the two cases apart later. However, the MD layer expects default values of all types, including strings, to be in native byte order (they're swapped again on big-endian hosts in CMiniMdBase::SwapConstant). In order to fix this mismatch, the default values of fields are already swapped yet another time by ilasm code in Assembler::EmitField. However, that same problem exists for default values of parameters or properties. This patch adds an equivalent swap operation there.

  • Prepare custom attribute blobs in little-endian byte order
    Custom attribute blobs (as opposed to default values) are expected to be provided in little-endian order by the MD layer. This already works for attributes specifies as byte arrays. But for attributes specified as structured values, values of all types except strings are currently provided in native order by the parser -- this is because serInit shares most of its implementation with fieldInit above. To fix this, I've added a new helper AppendFieldToCustomBlob that copies a typed value over to the custom attribute blob, byte-swapping as necessary. There are also a couple of places where ilasm synthesizes attribute blobs; some of those had a few byte swaps missing as well (see AsmMan::EmitDebuggableAttribute and the PermissionDecl constructor).

  • Fix byte order of ELEMENT_TYPE_TYPEDEF typespec blobs and of VTable blobs
    These two are unrelated to the above, just another set of instances where byte swaps were missing.

* Byte-swap property and parameter default values of string type

* Prepare custom attribute blobs in little-endian byte order

* Fix byte order of ELEMENT_TYPE_TYPEDEF typespec blobs

* Fix byte order of VTable blobs
@uweigand
Copy link
Contributor Author

Hi @BruceForstall , can you have a look?

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@BruceForstall BruceForstall self-requested a review July 21, 2021 00:47
@BruceForstall
Copy link
Member

/azp run runtime-coreclr ilasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

@BruceForstall BruceForstall merged commit cc7a8ef into dotnet:main Jul 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
@uweigand uweigand deleted the bigendian-ilasm branch September 21, 2021 18:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ILTools-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants