Skip to content

Add a few more riscv64 stubs in PAL#74793

Merged
jkotas merged 5 commits into
dotnet:mainfrom
am11:feature/riscv-build
Sep 4, 2022
Merged

Add a few more riscv64 stubs in PAL#74793
jkotas merged 5 commits into
dotnet:mainfrom
am11:feature/riscv-build

Conversation

@am11

@am11 am11 commented Aug 30, 2022

Copy link
Copy Markdown
Member

Also cleaned up some unused code.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-PAL-coreclr only for closed issues labels Aug 30, 2022
Comment thread src/coreclr/pal/src/arch/loongarch64/context.S Outdated
Comment thread src/coreclr/pal/src/thread/context.cpp Outdated
@am11
am11 force-pushed the feature/riscv-build branch 2 times, most recently from 644a306 to 9e84253 Compare August 30, 2022 08:51
@am11
am11 force-pushed the feature/riscv-build branch from 9887ad1 to 42ccb5f Compare August 30, 2022 14:48
@am11
am11 marked this pull request as ready for review August 30, 2022 16:24
@am11
am11 requested review from janvorli and jkotas and removed request for janvorli August 30, 2022 16:25
@am11

am11 commented Sep 2, 2022

Copy link
Copy Markdown
Member Author

@jkotas, please take a look.

Comment thread src/coreclr/pal/inc/pal.h
Comment thread src/coreclr/pal/inc/pal.h Outdated
Comment thread src/coreclr/pal/inc/rt/palrt.h Outdated
Comment thread src/coreclr/pal/src/thread/context.cpp Outdated
Comment thread src/coreclr/pal/src/thread/context.cpp Outdated
Comment thread src/coreclr/pal/src/thread/context.cpp Outdated
Comment thread src/coreclr/pal/src/include/pal/context.h Outdated
Comment thread src/coreclr/pal/src/include/pal/context.h Outdated
Comment thread src/coreclr/pal/src/include/pal/context.h Outdated
Comment thread src/coreclr/pal/inc/rt/palrt.h Outdated
@@ -343,6 +343,7 @@ enum {
EM_NORC = 218, // Nanoradio Optimized RISC

@am11 am11 Sep 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@jkotas, this file was once copied from https://github.com/llvm-mirror/llvm/blob/8b47c17a53d683f313eaaa93c4a53de26d8fcba5/include/llvm/Support/ELF.h in 8f1948e.

In the current llvm tree, it lives at a different location https://github.com/llvm/llvm-project/blob/c444af1c20b35555f2fdb2c1ca38d3f23b2faebd/llvm/include/llvm/BinaryFormat/ELF.h, it is refactored and requires additional dependency StringRef.h. Furthermore, since the code is diverged, new architectures (like loongarch64) were manually added in our copy (which is mostly unused code). It is no longer a drop-in replacement when comparing with upstream.

Except for windows crossdac, which requires struct Elf32_Ehdr definition from this header in elfreader.cpp, we only need the eight machine numbers (EM_xxx) of currently supported architectures in pal.h (ref: https://github.com/torvalds/linux/blob/master/include/uapi/linux/elf-em.h). Should we define them in pal.h and move those struct definitions needed for windows crossdac in elfreader.h/cpp?

struct Elf32_Ehdr is also defined in

(and 64 bit variant in ..64.h). coreclr uses these freebsd-elfXx.h headers for remote unwinding, and mono also uses it in dwarfwriter on windows:
#ifndef HOST_WIN32
#include <external/libunwind/include/remote/freebsd-elf32.h>
#include <external/libunwind/include/remote/freebsd-elf64.h>
but coreclr's elfreader requires a few other definitions on windows which are missing from freebsd-elfXx.h. We can try to make it work if this is a right path forward.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is unfortunate that there does not seem to be a standard header with ELF definitions that everybody can use, and everybody ends up creating their own version instead. I guess that we will end up following the same pattern since it is the only option that works.

I think it still makes sense to snap to LLVM version as much as possible, but without introducing dependencies on the rest of LLVM infrastructure like StringRef.h.

I would change the comment on the top of the file to say that this is a subset of the ELF.h from LLVM tree (ie our copy should just have lines deleted, and nothing significant added), delete parts that we do not need, and sync parts that diverged.

Comment thread src/coreclr/pal/inc/pal.h Outdated
Comment thread src/coreclr/pal/inc/pal.h Outdated

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

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 75e65e9 into dotnet:main Sep 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-PAL-coreclr only for closed issues 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