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

Prepare for upstream? #3

Open
yuzibo opened this issue Jun 29, 2022 · 38 comments
Open

Prepare for upstream? #3

yuzibo opened this issue Jun 29, 2022 · 38 comments

Comments

@yuzibo
Copy link

yuzibo commented Jun 29, 2022

Hi,
Amazing work!
Do we have a plan to let Valgrind upstream merge this feature?
If there are need help with real riscv64 hardware resources, please let me know.
Thanks again for your great work~

@petrpavlu
Copy link
Owner

Thanks, my aim is to have this port upstreamable some time in the second half of this year. Goal is that it should initially support RV64GC, pass the whole Valgrind test suite and be able to run some larger applications. Remaining main work for that is on completing support for RV64F, enabling gdbserver, adding coredump support and generally stabilizing the port.

@Xeonacid
Copy link
Contributor

Hi Petr,
I'd like to pick up the enabling gdbserver task if possible.

@petrpavlu
Copy link
Owner

Hi Petr, I'd like to pick up the enabling gdbserver task if possible.

Go for it! Let me know if you require any hints.

@gevorgyana
Copy link

@Xeonacid do you have progress on that? let me know if you need help

@Xeonacid
Copy link
Contributor

@Xeonacid do you have progress on that? let me know if you need help

It's done by #8 already. Thanks to laokz.

@gevorgyana
Copy link

@Xeonacid Is there any work left?

@Xeonacid
Copy link
Contributor

@Xeonacid Is there any work left?

You might try fixing the tests and adding some unit tests for riscv. :)

@gevorgyana
Copy link

Good, that's something to do. Thanks

@rjiejie
Copy link
Contributor

rjiejie commented Dec 2, 2022

We (Alibaba T-Head) can help to rebase this and upstream also, any issues or work we can help ?
We wan to support ISA as RV64gcv_zfh :)

@rjiejie
Copy link
Contributor

rjiejie commented Dec 5, 2022

@petrpavlu Hello ?

@petrpavlu
Copy link
Owner

My roadmap at the moment looks as follows. The items are not in any specific order:

  • Enable more syscall wrappers + write scalar tests if needed.
  • Reshuffle the codegen: merge instruction definitions and simplify selection. A good template should be the AArch64 backend.
  • Analyze and fix remaining failing tests in the Valgrind test suite.
  • Test that the port is able to run some real-world application.
  • Address various smaller TODOs, most of them are recorded as comments directly in the code (git diff master..riscv64-linux, search for "TODO").

I'm afraid a lot of this work is basically cleaning up and finishing things in many different areas, which might be hard for people to pick up if they are not already familiar with the relevant code.

If someone is looking for smaller tasks to tackle then perhaps analyzing and fixing remaining test failures might be suitable.

Scope of the initial port is to support only RV64GC. I don't have any immediate plan to work on the mentioned V (vector) or Zfh (half-precision floating-point) extensions. I realize they will need to be supported at some point but my plan is to rather get a good working port for RV64GC and try to upstream it first.

Adding support for these and other extensions might be then a good opportunity for someone other, as it should be fairly separate and well defined work. It should avoid risk of a duplicated effort.

@rjiejie
Copy link
Contributor

rjiejie commented Dec 7, 2022

My roadmap at the moment looks as follows. The items are not in any specific order:

  • Enable more syscall wrappers + write scalar tests if needed.
  • Reshuffle the codegen: merge instruction definitions and simplify selection. A good template should be the AArch64 backend.
  • Analyze and fix remaining failing tests in the Valgrind test suite.
  • Test that the port is able to run some real-world application.
  • Address various smaller TODOs, most of them are recorded as comments directly in the code (git diff master..riscv64-linux, search for "TODO").

I'm afraid a lot of this work is basically cleaning up and finishing things in many different areas, which might be hard for people to pick up if they are not already familiar with the relevant code.

I agree with you to clean up codes after I looked details of your code (front-end) in translation module, there are some questions for me:

  • RISCV have some coding rules for instructions, e.g. R-type/I-type/S-type and so on, we could add some more friendly interfaces to disassemble instruction :)
  • Is it possible to split huge file like guest_riscv64_toIR.c to guest_riscv64I_toIR.c / guest_riscv64M_toIR.c and so on ? that looks better or maintain easily ? you need to consider Vendor's extension :)
  • Is it possible to reuse some LLVM's riscv backend like MC layer to integrate with Valgrind ?

If someone is looking for smaller tasks to tackle then perhaps analyzing and fixing remaining test failures might be suitable.

Scope of the initial port is to support only RV64GC. I don't have any immediate plan to work on the mentioned V (vector) or Zfh (half-precision floating-point) extensions. I realize they will need to be supported at some point but my plan is to rather get a good working port for RV64GC and try to upstream it first.

Is there any time schedule ?
you can try to upstream a subset of ISAs if you have cleaned up that part, and qualify it easily also, alright ?
I mean you can upstream RV64I, and then RV64M, one by one :) do you agree ?
we can help to test or review if you needed :)

Adding support for these and other extensions might be then a good opportunity for someone other, as it should be fairly separate and well defined work. It should avoid risk of a duplicated effort.

we can help to support Zfh extension based on your code if nobody have schedule to implement that :)

@petrpavlu
Copy link
Owner

I agree with you to clean up codes after I looked details of your code (front-end) in translation module, there are some questions for me:

  • RISCV have some coding rules for instructions, e.g. R-type/I-type/S-type and so on, we could add some more friendly interfaces to disassemble instruction :)

The backend (host_riscv64_defs.c) has per-type emitters. I'm not immediately sure doing something similar in the frontend (guest_riscv64_toIR.c) would be beneficial. I'd probably need to see some prototype first. To be honest, I'm reasonably satisfied with the frontend code at the moment.

  • Is it possible to split huge file like guest_riscv64_toIR.c to guest_riscv64I_toIR.c / guest_riscv64M_toIR.c and so on ? that looks better or maintain easily ? you need to consider Vendor's extension :)

The frontend code is very linear, it simply decodes instructions one by one. It doesn't look to me that splitting it into multiple files would make it easier to navigate.

It is also important that the overall Valgrind project has consistent implementation. The riscv64 code in this case follows what is done for other supported architectures upstream.

  • Is it possible to reuse some LLVM's riscv backend like MC layer to integrate with Valgrind ?

I don't think it is easily possible but I'm not sure what particular idea you have in mind.

If someone is looking for smaller tasks to tackle then perhaps analyzing and fixing remaining test failures might be suitable.
Scope of the initial port is to support only RV64GC. I don't have any immediate plan to work on the mentioned V (vector) or Zfh (half-precision floating-point) extensions. I realize they will need to be supported at some point but my plan is to rather get a good working port for RV64GC and try to upstream it first.

Is there any time schedule ?
you can try to upstream a subset of ISAs if you have cleaned up that part, and qualify it easily also, alright ?
I mean you can upstream RV64I, and then RV64M, one by one :) do you agree ?
we can help to test or review if you needed :)

I'm hoping to send initial patches for upstream review soon. Review process and inclusion upstream usually takes some time so that still gives room in the meantime to address various TODOs. Some remaining ones and also other further work would be then done upstream, at least that is the idea.

It looks better to me to upstream support for RV64GC at once. It is the current base target for most Linux distributions and so supporting less than that is generally not very useful. Besides, the code is already there and it would only create more effort to have to split it up for multiple reviews.

Adding support for these and other extensions might be then a good opportunity for someone other, as it should be fairly separate and well defined work. It should avoid risk of a duplicated effort.

we can help to support Zfh extension based on your code if nobody have schedule to implement that :)

Sure, go ahead. I currently don't intend to work on supporting the Zfh extension and I'm not aware of anyone else looking at it. I'd be happy to review its implementation and include it in the port. Let me know if you need any help.

Please be only aware that I plan to do some reshuffling of the backend code (as mentioned earlier) so that might cause then a bit of extra work to rebase your code.

@paulfloyd
Copy link
Contributor

I won't be able to help review the VEX code as it's the bit that I know the least. But I'm ready upstream and already done some testing and added a couple of issues.

@paulfloyd
Copy link
Contributor

We'll be discussing this on Friday (10th Feb 2023). See the valgrind developers mailing list.

@petrpavlu
Copy link
Owner

We'll be discussing this on Friday (10th Feb 2023). See the valgrind developers mailing list.

Thanks for the notice. I should be able to join.

@rjiejie
Copy link
Contributor

rjiejie commented Feb 9, 2023

@petrpavlu We have finished Zfh ISA extension and start full regressing.
We found some errors/failures are depend on host linux environment/toolchain version(e.g. glibc),
could you list more details about for your all wrong cases ? it's helpful for us to check/fix that :)

@petrpavlu
Copy link
Owner

@petrpavlu We have finished Zfh ISA extension and start full regressing.

Nice!

We found some errors/failures are depend on host linux environment/toolchain version(e.g. glibc), could you list more details about for your all wrong cases ? it's helpful for us to check/fix that :)

The following is from openSUSE Tumbleweed 20220926 and with packages gcc-12-2.1.riscv64 + glibc-2.36-5.1.riscv64:

== 653 tests, 20 stderr failures, 7 stdout failures, 1 stderrB failure, 1 stdoutB failure, 0 post failures ==
gdbserver_tests/hginfo                   (stderrB)
gdbserver_tests/hgtls                    (stdoutB)
memcheck/tests/atomic_incs               (stdout)
memcheck/tests/atomic_incs               (stderr)
memcheck/tests/cdebug_zlib_gnu           (stderr)
memcheck/tests/leak-segv-jmp             (stderr)
memcheck/tests/linux/stack_changes       (stderr)
memcheck/tests/linux/sys-execveat        (stdout)
memcheck/tests/linux/sys-execveat        (stderr)
memcheck/tests/origin2-not-quite         (stderr)
memcheck/tests/origin4-many              (stderr)
memcheck/tests/pointer-trace             (stderr)
memcheck/tests/sh-mem-random             (stdout)
memcheck/tests/sh-mem-random             (stderr)
memcheck/tests/xml1                      (stderr)
helgrind/tests/annotate_hbefore          (stderr)
helgrind/tests/tc07_hbl1                 (stdout)
helgrind/tests/tc07_hbl1                 (stderr)
helgrind/tests/tc08_hbl2                 (stdout)
helgrind/tests/tc08_hbl2                 (stderr)
helgrind/tests/tls_threads               (stderr)
drd/tests/annotate_hbefore               (stderr)
drd/tests/pth_barrier_thr_cr             (stderr)
drd/tests/pth_mutex_signal               (stderr)
drd/tests/tc07_hbl1                      (stdout)
drd/tests/tc07_hbl1                      (stderr)
drd/tests/tc08_hbl2                      (stdout)
drd/tests/tc08_hbl2                      (stderr)
none/tests/libvexmultiarch_test          (stderr)

@rjiejie
Copy link
Contributor

rjiejie commented Mar 2, 2023

@petrpavlu @paulfloyd we finished full regression of Zfh extension ISA,
how do we push this patch ? make a pull request on GitHub or send patch to mail list of valgrind community ?
Any suggestions ?

@paulfloyd
Copy link
Contributor

"Valgrind community" usually just means Mark W, Julian S and myself (with Julian often being busy with other work).

We discussed upstreaming at a recent video conf. Work is planned to start after the Valgrind 3.21 release at the end of April.

@petrpavlu have you considered asking Mark and Julian if you can get a commit bit for sourceware.org? That would make upstreaming and maintenance a lot easier.

@rjiejie
Copy link
Contributor

rjiejie commented Mar 3, 2023

"Valgrind community" usually just means Mark W, Julian S and myself (with Julian often being busy with other work).
We discussed upstreaming at a recent video conf. Work is planned to start after the Valgrind 3.21 release at the end of April.

Sounds good 👍
I will make a pull request to @petrpavlu on GitHub soon.

BTW,
@paulfloyd whether we (Alibaba T-Head) need to sign something like "contributor license" if commit patch to community directly later ?

@paulfloyd
Copy link
Contributor

@rjiejie As far as I'm aware, no need to sign a license. Valgrind is GPLv2 (with the exception of the public headers which are BSD licensed to allow users to include them in order to use client requests).

Each new file must start with the GPLv2 license and also include a copyright notice.

Optionally you may want to create a "README.riscv" file in the top source directory. Here you can put things like where to get more RISC-V info, a list of todos and some history of the port and contributors.

@petrpavlu
Copy link
Owner

"Valgrind community" usually just means Mark W, Julian S and myself (with Julian often being busy with other work).
We discussed upstreaming at a recent video conf. Work is planned to start after the Valgrind 3.21 release at the end of April.

Sounds good +1 I will make a pull request to @petrpavlu on GitHub soon.

Sounds good, I'll review the changes. Note that as previously mentioned, I did some reshuffling of the backend code to avoid a lot of almost duplicate code and to be more consistent with how other backends look like, in particular AArch64 and MIPS ones. Hopefully it won't create much extra work for you to rebase your changes. You can also potentially post your current changes as-is and I can rebase them myself.

@petrpavlu have you considered asking Mark and Julian if you can get a commit bit for sourceware.org? That would make upstreaming and maintenance a lot easier.

I will most likely ask for that some time during upstream review of the port.

Optionally you may want to create a "README.riscv" file in the top source directory. Here you can put things like where to get more RISC-V info, a list of todos and some history of the port and contributors.

Agreed, adding README.riscv and a RISC-V variant of docs/internals/qemu-aarch64-linux-HOWTO.txt is on my TODO list.

@rjiejie
Copy link
Contributor

rjiejie commented Mar 6, 2023

@petrpavlu i have rebased and made a pull request #12 to your branch :)

For Zfh (float16) extension ISA, I add some common IRs, pls help to review also @paulfloyd

@paulfloyd
Copy link
Contributor

I can look, but VEX is the part that I've worked on the least.

@rjiejie
Copy link
Contributor

rjiejie commented Mar 7, 2023

@paulfloyd How do I commit patch to community mail list ?
I can not find any details like mail format from https://valgrind.org/help/contributing.html
it says commit patch to "valgrind-developers" just, I have a misc improved small patch to contribute :)

@paulfloyd
Copy link
Contributor

@rjiejie if your patch is only for riscv, just go though the pull request process here and let @petrpavlu handle it.

if you have something that is not riscv specific, https://bugs.kde.org is the place to submit patches

Valgrind has a very long history, so the mailing lists are on sourceforge, the git repo hosted by sourceware (as is the web site now) and the bugzilla is kde.

@rjiejie
Copy link
Contributor

rjiejie commented Mar 7, 2023

if you have something that is not riscv specific, https://bugs.kde.org is the place to submit patches

Valgrind has a very long history, so the mailing lists are on sourceforge, the git repo hosted by sourceware (as is the web site now) and the bugzilla is kde.

It's not a bug, it's a improved patch, I need to commit into bug system also ?

@paulfloyd
Copy link
Contributor

If the patch is for riscv you need to submit it here.

@rjiejie
Copy link
Contributor

rjiejie commented Mar 13, 2023

@petrpavlu We make other some PRs for riscv also, pls review them #13 #14

@petrpavlu
Copy link
Owner

Opened https://bugs.kde.org/show_bug.cgi?id=468575 with v1 posted so Valgrind maintainers can start looking at this port.

Note: I'm aware of #16. I'll review it and add it in a next version for upstream with any other fixes.

@rjiejie
Copy link
Contributor

rjiejie commented Apr 20, 2023

Opened https://bugs.kde.org/show_bug.cgi?id=468575 with v1 posted so Valgrind maintainers can start looking at this port.

excellent efforts and big news :)

Note: I'm aware of #16. I'll review it and add it in a next version for upstream with any other fixes.

Ok, thanks.

@rjiejie
Copy link
Contributor

rjiejie commented Apr 20, 2023

@paulfloyd @petrpavlu
We consider to add RVV/Vector feature in valgrind and met some issues from investigation.

RVV like ARM's SVE programming model, it's scalable/VLA, that means the vector length is agnostic.
ARM's SVE is not supported in valgrind :(

There is not any VLA vector IR to represent these vector model, it's the big issue.
Also some other common module like "register allocator" in VEX can not represent vector type which use
more than one register.
In another hand, some tool plugin like Memcheck will use data type like "Ity_V128/Ity_V256" to generate IRs, it's a big challenge

Any ideas for supporting of scalable vector model ?

@rjiejie
Copy link
Contributor

rjiejie commented Apr 20, 2023

@paulfloyd @petrpavlu We consider to add RVV/Vector feature in valgrind and met some issues from investigation.

RVV like ARM's SVE programming model, it's scalable/VLA, that means the vector length is agnostic. ARM's SVE is not supported in valgrind :(

There is not any VLA vector IR to represent these vector model, it's the big issue. Also some other common module like "register allocator" in VEX can not represent vector type which use more than one register. In another hand, some tool plugin like Memcheck will use data type like "Ity_V128/Ity_V256" to generate IRs, it's a big challenge

Any ideas for supporting of scalable vector model ?

use another individual issue to follow vector #17

@mingyuan-xia
Copy link

@paulfloyd any news on the merge ticket on KDE bug tracker?
The port contributors have explained that some unit test failures are reasonable w.r.t. RISC-V memory model.
Wondering if we can kick on the RISC-V port in 2024. Could be a big news for the RISC-V international community.

@paulfloyd
Copy link
Contributor

@paulfloyd any news on the merge ticket on KDE bug tracker? The port contributors have explained that some unit test failures are reasonable w.r.t. RISC-V memory model. Wondering if we can kick on the RISC-V port in 2024. Could be a big news for the RISC-V international community.

There was a thread discussing vector extensions. I need to check if that has reached a conclusion.

Other than that, it's just a question of time and having too many other things to do. I do have access to at least one riscv machine

Linux cfarm92 5.19.0-1021-generic #23~22.04.1-Ubuntu SMP Thu Jun 22 12:49:35 UTC 2023 riscv64 riscv64 riscv64 GNU/Linux

@davidlt
Copy link

davidlt commented May 8, 2024

There are another 5 StarFive VF2 boards in Sourceware buildbot farm. Valgrind hasn't been enabled there yet.

@paulfloyd
Copy link
Contributor

There are another 5 StarFive VF2 boards in Sourceware buildbot farm. Valgrind hasn't been enabled there yet.

I'm sure that Mark Wielaard will do the necessary setup once riscv has been merged in git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants