[Bug backends/27925] New: riscv backend only provides return value locations for code compiled for LP64D ABI

[Bug backends/27925] New: riscv backend only provides return value locations for code compiled for LP64D ABI

[wcohen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

            Bug ID: 27925
           Summary: riscv backend only provides return value locations for
                    code compiled for LP64D ABI
           Product: elfutils
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: backends
          Assignee: unassigned at sourceware dot org
          Reporter: wcohen at redhat dot com
                CC: elfutils-devel at sourceware dot org
  Target Milestone: ---

When diagnosing why a port of systemtap to riscv wasn't able to get $return
values from functions I found that the elfutils riscv backend initialization
code only sets up the return value location handler for the riscv for code that
is compiled for hardware floating point (LP64D):

https://sourceware.org/git/?p=elfutils.git;a=blob;f=backends/riscv_init.c;h=551e7bb6d34942222a1627e98de91208ef8261af;hb=HEAD#l67

elfutils dwfl_module_return_value_location() should still be able to provide
the location of the return value for riscv code for the other ABIs besides
LP64D.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wcohen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #1 from William Cohen <wcohen at redhat dot com> ---
Created attachment 13669
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13669&action=edit
Possibe patch to provide return value information for lp64 abi

This patch applies to elfutils 0.183 in fedora 33 and builds successfully. 
When running the systemtap examples testsuite there were a number of probe
handlers that needed $return information what no longer complained about being
unable to find the $return like the following with the patched elfutils for
pfault.stp example:

$ sudo ../install/bin/stap -v  testsuite/systemtap.examples/memory/pfaults.stp 
Pass 1: parsed user script and 462 library scripts using
307316virt/76224res/10492shr/65720data kb, in 3020usr/560sys/3602real ms.
Pass 2: analyzed script: 3 probes, 7 functions, 2 embeds, 7 globals using
422972virt/182640res/11412shr/181376data kb, in 10500usr/1900sys/12425real ms.
Pass 3: translated to C into
"/tmp/stapFHW9Zg/stap_aeb36ff31c1e99bccaad8645b97de810_4982_src.c" using
422972virt/182768res/11540shr/181376data kb, in 300usr/940sys/1247real ms.
Pass 4: compiled C into "stap_aeb36ff31c1e99bccaad8645b97de810_4982.ko" in
51270usr/5930sys/51419real ms.
Pass 5: starting run.
^CPass 5: run completed in 1350usr/2510sys/300065real ms.

Previously with the unpatched elfutils:

$ sudo ../install/bin/stap -v  testsuite/systemtap.examples/memory/pfaults.stp 
Pass 1: parsed user script and 462 library scripts using
307316virt/76260res/10528shr/65720data kb, in 3710usr/640sys/4447real ms.
semantic error: failed to retrieve return value location for handle_mm_fault
[man error::dwarf] (mm/memory.c): identifier '$return' at
/home/riscv/systemtap_write/install/share/systemtap/tapset/linux/memory.stp:91:15
        source:         fault_type = $return
                                     ^

Pass 2: analyzed script: 3 probes, 6 functions, 2 embeds, 7 globals using
422972virt/182872res/11644shr/181376data kb, in 12180usr/2310sys/14710real ms.
Pass 2: analysis failed.  [man error::pass2]

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at klomp dot org,
                   |                            |wilson at gcc dot gnu.org

--- Comment #2 from Mark Wielaard <mark at klomp dot org> ---
Hi Will,

https://sourceware.org/bugzilla/attachment.cgi?id=13669
"Possibe patch to provide return value information for lp64 abi"
Look plausible correct to me.

I added Jim Wilson to the CC to see if he has any opinion on the new code
(since he wrote the existing support).

Would it be possible to create a testcase based on tests/funcretval_test.c and
tests/run-funcretval.sh for this riscv variant?

Thanks,

Mark

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wilson at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #3 from Jim Wilson <wilson at gcc dot gnu.org> ---
I'm not aware of any supported lp64 linux systems, so I don't know why the
patch is needed, and I am unable to test it.

It does look mostly reasonable.  The riscv_init.c change isn't quite correct as
we have 3 64-bit ABIs.  lp64d is 64-bit FP registers, lp64f is 32-bit FP
registers, and lp64 is no FP registers.  So we actually need a check for the
lp64 ABI instead of assuming that !lp64d is lp64.  Also the "lp64i" should be
"lp64" to be consistent with the ABI naming scheme.

I would suspect other changes are required for full lp64 support. 
riscv_abi_cfi references FP registers unconditionally.  riscv_register_info
references FP registers unconditionally.

The lp64 and lp64f support is incomplete because I've never had access to a
system where I can do the necessary work.  Similarly, the ilp32, ilp32f, and
ilp64d support is also incomplete as I don't have access to any system I can
use to develop them.  All RISC-V linux desktop distros are lp64d.  I think the
linux kernel lacks support for ilp32f and lp64f so those may be impossible to
build at present.  There are some embedded linux distros that use the 32-bit
ABIs, and/or that use soft-float ABIs, but SiFive doesn't support those.  So
I'd have to build an entire linux distro from scratch and boot on qemu to set
up an environment just to do an elfutils port, and I haven't been willing to do
that.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wcohen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #4 from William Cohen <wcohen at redhat dot com> ---
The patch was a first attempt to get something that addresses the issue. 
Thanks for pointing out where it needs improvements.

Don't have a good way to test the issue in user-space so far as the 64-bit
risc-v gcc environment seems the expect to have have floating point support. 
One way to test might to use perf to set up a probe point
(https://www.brendangregg.com/perf.html), but I don't know if perf is using
elfutils to get the return value location. Based on the pfault.stp example
would have something like:

sudo perf probe handle_mm_fault%return $retval
sudo perf record -e probe:handle_mm_fault__return -aR sleep 1
sudo perf report
sudo perf probe --del="handle_mm_fault__return"


This issue occurs when using systemtap on the RISC-V linux kernel which doesn't
link against the normal glibc runtime and takes steps to ensure that the
floating point hardware is not used.  Below is a example gcc commandline from a
native build (note the -mabi=lp64 -march=rv64imac options):

  gcc -Wp,-MMD,arch/riscv/kernel/probes/.kprobes.o.d -nostdinc -isystem
/usr/lib/gcc/riscv64-redhat-linux/10/include -I./arch/riscv/include
-I./arch/riscv/include/generated  -I./include -I./arch/riscv/include/uapi
-I./arch/riscv/include/generated/uapi -I./include/uapi
-I./include/generated/uapi -include ./include/linux/compiler-version.h -include
./include/linux/kconfig.h -include ./include/linux/compiler_types.h
-D__KERNEL__ -DCC_USING_PATCHABLE_FUNCTION_ENTRY -fmacro-prefix-map=./= -Wall
-Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing
-fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration
-Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu89
-mabi=lp64 -march=rv64imac -mno-save-restore
-DCONFIG_PAGE_OFFSET=0xffffffe000000000 -mcmodel=medany -fno-omit-frame-pointer
-mstrict-align -fno-delete-null-pointer-checks -Wno-frame-address
-Wno-format-truncation -Wno-format-overflow -Wno-address-of-packed-member -O2
-fno-allow-store-data-races -Wframe-larger-than=2048 -fno-stack-protector
-Wimplicit-fallthrough=5 -Wno-unused-but-set-variable
-Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls
-fno-stack-clash-protection -g -fpatchable-function-entry=8
-Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-stringop-truncation
-Wno-zero-length-bounds -Wno-array-bounds -Wno-stringop-overflow -Wno-restrict
-Wno-maybe-uninitialized -fno-strict-overflow -fno-stack-check -fconserve-stack
-Werror=date-time -Werror=incompatible-pointer-types -Werror=designated-init
-Wno-packed-not-aligned   
-DKBUILD_MODFILE='"arch/riscv/kernel/probes/kprobes"'
-DKBUILD_BASENAME='"kprobes"' -DKBUILD_MODNAME='"kprobes"'
-D__KBUILD_MODNAME=kmod_kprobes -c -o arch/riscv/kernel/probes/kprobes.o
arch/riscv/kernel/probes/kprobes.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wilson at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #5 from Jim Wilson <wilson at gcc dot gnu.org> ---
I hadn't thought about the kernel.  It is compiled LP64 and with the FP
extensions disabled, to avoid accidentally using FP registers.  It does
save/restore FP registers in the context switching code, but otherwise doesn't
use FP registers, so that we don't have to save/restore FP registers in system
calls.  If there are no FP types in the kernel then LP64D will behave
identically to LP64.  But if you have a testsuite that checks all features,
then we would have to implement LP64 correctly, and your patch is a step in the
right direction.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #6 from Mark Wielaard <mark at klomp dot org> ---
There is tests/funcretval_test.c if you compile that (or a subset that doesn't
return floating point values) as described in tests/run-funcretval.sh then we
can construct a user space testcase.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wcohen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #7 from William Cohen <wcohen at redhat dot com> ---
Created attachment 13676
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13676&action=edit
Reworked risv retval patch

I reworked that patch to add lp64f support and added lp64f support.  I assumed
that the lp64f falls back to passing doubles in integer register, but I didn't
see that spelled out in the calling conventions
(https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf).  Is there
some document that describes the lp64f calling conventions?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wilson at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #8 from Jim Wilson <wilson at gcc dot gnu.org> ---
Calling convention docs:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc
Recently converted from markdown to asciidoc.  The releases link on the right
contains pdf files which are likely the easiest way to read this.

Yes, any float larger than the FP regs is passed in the integer regs.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wilson at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #9 from Jim Wilson <wilson at gcc dot gnu.org> ---
The patch looks OK to me.

In the old code, for float and double (not complex) we call pass_in_fpr_lp64d
because it returns the same result as pass_in_fpr_lp64f and not worth an extra
check for that.  In the new code, float and double are now handled separately,
and the call to pass_in_fpr_lp64d for the float case looks a little funny but
it will give the right result.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #10 from Mark Wielaard <mark at klomp dot org> ---
(In reply to Jim Wilson from comment #9)
> The patch looks OK to me.

So I like to include it. Will, could you sent the patch as a git commit (see
CONTRIBUTING) to the elfutils-devel list?

And if at all possible could someone generate some testcases? See Comment #6:

  There is tests/funcretval_test.c if you compile that
  (or a subset that doesn't return floating point values)
  as described in tests/run-funcretval.sh then we can
  construct a user space testcase.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wilson at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #11 from Jim Wilson <wilson at gcc dot gnu.org> ---
Userspace is lp64d.  The kernel is lp64.  You can't create an lp64 binary
because there are no lp64 start files or libraries.  The kernel can be linked
because it doesn't require any start files or libraries.  The riscv backend is
using the ELF header flags to determine if a binary is lp64 or lp64d, so if you
can't create an lp64 binary you can't run an lp64 test.  Similarly, you also
can't construct and run lp64f, ilp32, ilp32f, or ilp32d ABI tests for the same
reason.

The difference between lp64 and lp64d is that FP values are passed in integer
regs in the first and FP regs in the second.  If you aren't testing FP values,
then you aren't properly testing the lp64 ABI.  Though given that the kernel is
compiled soft-float, such testing would be sufficient for using elfutils with
an lp64 kernel, but not for a lp64 user space.

The best way to test the lp64 ABI support is to build an entire linux system
from scratch using an lp64 userspace.  You can then natively test the lp64
support. 
 Likewise for lp64f, ilp32, ilp32f, and ilp32d.  That is outside the scope of
this bug report.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[wcohen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

--- Comment #12 from William Cohen <wcohen at redhat dot com> ---
The patch for the retval handling has been submitted.  As mentioned in comment
#9 testing is not trivial.  Normal user-space executables are compiled with
floating-point enabled and those executables work fine without the fixes.  The
LP64 and LP64F ABIs don't have the need start or libraries to compile
user-space tests.  Leaving the testsuite for later consideration.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug backends/27925] riscv backend only provides return value locations for code compiled for LP64D ABI

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27925

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #13 from Mark Wielaard <mark at klomp dot org> ---
(In reply to William Cohen from comment #12)
> The patch for the retval handling has been submitted.

Thanks. I added a ChangeLog entry, made some (GNU) code style changes and
committed as:

commit 2e57301be1bbb9c34f8a59122ab500de46eb7acb (HEAD -> master)
Author: William Cohen <wcohen@redhat.com>
Date:   Wed Sep 29 14:56:15 2021 -0400

    RISC-V: PR27925 Add support for LP64 and LP64F ABIs return values

    The RISC-V Linux kernel is compiled without floating point (the LP64
    ABI) and elfutils could not obtain return value locations for
    functions in the kernel.  This issue was noticed when Systemtap
    generated RISC-V kernel modules for scripts that used $return target
    variables in function return probes. This patch adds the needed
    support to provide return value information for the LP64 and LP64F
    ABIs.

    Signed-off-by: William Cohen <wcohen@redhat.com>

>  As mentioned in
> comment #9 testing is not trivial.  Normal user-space executables are
> compiled with floating-point enabled and those executables work fine without
> the fixes.  The LP64 and LP64F ABIs don't have the need start or libraries
> to compile user-space tests.  Leaving the testsuite for later consideration.

OK. But even a testcase for the already working "normal" user-space would be
appreciated. And we might be able to create a kernel module as testcase.

-- 
You are receiving this mail because:
You are on the CC list for the bug.