Prepare for upstream? - Githubissues

yuzibo commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

petrpavlu commented 2 years ago

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 commented 1 year ago

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

Xeonacid commented 1 year ago

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

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

gevorgyana commented 1 year ago

@Xeonacid Is there any work left?

Xeonacid commented 1 year ago

@Xeonacid Is there any work left?

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

gevorgyana commented 1 year ago

Good, that's something to do. Thanks

rjiejie commented 1 year ago

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 commented 1 year ago

@petrpavlu Hello ?

petrpavlu commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

petrpavlu commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

"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 commented 1 year ago

"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 commented 1 year ago

@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 commented 1 year ago

"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 commented 1 year ago

@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 commented 1 year ago

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

rjiejie commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

rjiejie commented 1 year ago

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

petrpavlu commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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

paulfloyd commented 4 months ago

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.

petrpavlu / valgrind-riscv64

Prepare for upstream? #3