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

[RISC-V] Initial commit for libraries directory #90203

Merged
merged 2 commits into from
Sep 13, 2023
Merged

[RISC-V] Initial commit for libraries directory #90203

merged 2 commits into from
Sep 13, 2023

Conversation

t-mustafin
Copy link
Contributor

@t-mustafin t-mustafin commented Aug 8, 2023 •
edited
Loading

RISC-V API proposed in #73437.
Part of #84834.
cc @alpencolt @gbalykov @ashaurtaev @clamp03.

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Aug 8, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 8, 2023
This was referenced Aug 9, 2023
Closed
Closed
@t-mustafin t-mustafin marked this pull request as ready for review August 9, 2023 19:49
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Aug 10, 2023
@t-mustafin t-mustafin requested a review from am11 August 10, 2023 13:59
@am11 am11 added this to the 9.0.0 milestone Aug 10, 2023
@am11
Copy link
Member

am11 commented Aug 10, 2023

  • We will need to wait for API to be approved.
  • Lets drop Riscv32 and Riscv128, since those targets are not supported (yet 🙂)

@t-mustafin
Copy link
Contributor Author

  • Lets drop Riscv32 and Riscv128, since those targets are not supported (yet 🙂)

@am11 Should I drop Riscv{32,128} all over the patch or except Machine.cs and System.Reflection.Metadata.cs? This two files also have some targets are not supported yet, e.g. LoongArch32.

@am11
Copy link
Member

am11 commented Aug 15, 2023

I think the metadata change is fine if it makes things consistent, but we can ask this question during (or after) the API review whenever it will take place.

@gbalykov
Copy link
Member

@am11 do we need to open separate PR to approve new API?

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Aug 22, 2023 •
edited
Loading

@am11 do we need to open separate PR to approve new API?

I believe #73437 is tracking the API. If it is ready, I can set it ready for approval.

EDIT Based on this work and the question, I marked #73437 "ready for review".

jkotas
jkotas reviewed Aug 30, 2023
View reviewed changes
...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs Outdated
@@ -90,7 +90,7 @@ internal sealed class ManagedTextSection
/// If set, the module contains instructions that assume a 64 bit instruction set. For example it may depend on an address being 64 bits.
/// This may be true even if the module contains only IL instructions because of PlatformInvoke and COM interop.
/// </summary>
internal bool Requires64bits => Machine == Machine.Amd64 || Machine == Machine.IA64 || Machine == Machine.Arm64;
internal bool Requires64bits => Machine == Machine.Amd64 || Machine == Machine.IA64 || Machine == Machine.Arm64 || Machine == Machine.Riscv64;
Copy link
Member

Choose a reason for hiding this comment

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

This is only needed if Windows got ported to Riscv64.

There are many 64-bit architectures missing in this check and it is not causing any issues.

I would keep this simple and omit the Riscv check from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@t-mustafin t-mustafin force-pushed the riscv_libraries branch 2 times, most recently from 4b9e266 to ad6e24a Compare August 30, 2023 21:41
@jkotas jkotas added area-System.Runtime.InteropServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

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

Issue Details

RISC-V API proposed in #73437.
Part of #84834.
cc @alpencolt @gbalykov @ashaurtaev @clamp03.

Author: t-mustafin
Assignees: t-mustafin
Labels:

area-System.Runtime.InteropServices, new-api-needs-documentation, community-contribution, arch-riscv, needs-area-label
Milestone: 9.0.0

@jkotas jkotas added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 31, 2023
@jkotas
Copy link
Member

jkotas commented Aug 31, 2023

#73437 needs to go through API review before this can be merged.

@AaronRobinsonMSFT
Copy link
Member

These new APIs have been approved. The only update was to change the casing of the v to be uppercase so it properly refers to "5".

@AaronRobinsonMSFT AaronRobinsonMSFT removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 12, 2023
@t-mustafin @jkotas
Update src/libraries/System.Reflection.Metadata/tests/PortableExecuta…
8e835e6 
…ble/PEHeadersTests.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
jkotas
jkotas approved these changes Sep 13, 2023
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.

LGTM. Thank you!

@jkotas jkotas mentioned this pull request Sep 13, 2023
Closed
@jkotas jkotas merged commit 1c93ad7 into dotnet:main Sep 13, 2023
170 checks passed
@am11 am11 mentioned this pull request Sep 13, 2023
Closed
@t-mustafin t-mustafin deleted the riscv_libraries branch September 19, 2023 18:58
@ghost ghost locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@am11 am11 am11 left review comments

@ashaurtaev ashaurtaev ashaurtaev approved these changes

@jkotas jkotas jkotas approved these changes

@clamp03 clamp03 clamp03 approved these changes

Assignees

@t-mustafin t-mustafin

Labels
arch-riscv Related to the RISC-V architecture area-System.Runtime.InteropServices community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@t-mustafin @am11 @gbalykov @AaronRobinsonMSFT @jkotas @clamp03 @ashaurtaev