RISC-V: Support PLT hooking to trace library functions #1853
Labels
Milestone
Comments
|
Please check review/riscv-libcall-v1. I only tested it on qemu but it works well for me. |
|
My test result is like below: |
|
The following results were obtained from |
|
Thanks for working on this! But I see many failures in a RISC-V board and one of the failure is as follows. |
|
Here are the full failures except for argument tests. |
|
Can you share your binary? It works fine on my system. |
Sure. I've attached the binary at t-thread.txt. The |
|
Oh, right. I missed a place to update the PLTGOT offset. My system enabled BIND_NOW by default so I couldn't check. Can you please test this? diff --git a/libmcount/plthook.c b/libmcount/plthook.c
index 27537baf..42a638d8 100644
--- a/libmcount/plthook.c
+++ b/libmcount/plthook.c
@@ -762,7 +762,7 @@ static void update_pltgot(struct mcount_thread_data *mtdp, struct plthook_data *
pthread_mutex_lock(&resolver_mutex);
#endif
if (!pd->resolved_addr[dyn_idx]) {
- int got_idx = 3 + dyn_idx;
+ int got_idx = ARCH_PLTGOT_OFFSET + dyn_idx;
plthook_addr = mcount_arch_plthook_addr(pd, dyn_idx);
setup_pltgot(pd, got_idx, dyn_idx, (void *)plthook_addr);
} |
|
Thanks for the update. I see that it now works fine with your fix! I just ran the Here are the full list of |
|
I see one of weird failure in 119, 120 and 121 have the same failures as 048 in clang compiled binaries. |
|
I think we better focus on these failures. |
|
Can you please attach the |
|
Sure. Here it is. t-malloc.txt |
|
It looks exception handling is broken as well. It still fails even without libcall and with the following changes. diff --git a/arch/riscv64/mcount.S b/arch/riscv64/mcount.S
index e5fc3047..9a7bf9ea 100644
--- a/arch/riscv64/mcount.S
+++ b/arch/riscv64/mcount.S
@@ -3,6 +3,7 @@
.text
GLOBAL(_mcount)
+ ret
/* setup frame pointer & return address */
addi sp, sp, -80
sd ra, 72(sp)
diff --git a/libmcount/wrap.c b/libmcount/wrap.c
index 30adcf09..ed94bad2 100644
--- a/libmcount/wrap.c
+++ b/libmcount/wrap.c
@@ -315,49 +315,11 @@ __visible_default int backtrace(void **buffer, int sz)
__visible_default void __cxa_throw(void *exception, void *type, void *dest)
{
- struct mcount_thread_data *mtdp;
-
- if (unlikely(real_cxa_throw == NULL))
- mcount_hook_functions();
-
- mtdp = get_thread_data();
- if (!check_thread_data(mtdp)) {
- pr_dbg2("%s: exception thrown from [%d]\n", __func__, mtdp->idx);
-
- mtdp->in_exception = true;
-
- /*
- * restore return addresses so that it can unwind stack
- * frames safely during the exception handling.
- * It pairs to mcount_rstack_reset_exception().
- */
- mcount_rstack_restore(mtdp);
- }
-
real_cxa_throw(exception, type, dest);
}
__visible_default void __cxa_rethrow(void)
{
- struct mcount_thread_data *mtdp;
-
- if (unlikely(real_cxa_rethrow == NULL))
- mcount_hook_functions();
-
- mtdp = get_thread_data();
- if (!check_thread_data(mtdp)) {
- pr_dbg2("%s: exception rethrown from [%d]\n", __func__, mtdp->idx);
-
- mtdp->in_exception = true;
-
- /*
- * restore return addresses so that it can unwind stack
- * frames safely during the exception handling.
- * It pairs to mcount_rstack_reset_exception()
- */
- mcount_rstack_restore(mtdp);
- }
-
real_cxa_rethrow();
}
|
|
The binaries are attached here. |
|
Is this built by clang? I cannot reproduce. And the exception handling works. |
Yes, it's built by clang.
|
|
I think fixing all those failures might be difficult right now so it might be better to merge the current implementation with #1853 (comment), then create separate issues for these problems. |
|
I can reproduce the malloc failure with clang. |
|
Looks like the malloc issue is due to alignment. The following change would make it work. diff --git a/tests/s-malloc.c b/tests/s-malloc.c
index 9afb0e42..f94f67e3 100644
--- a/tests/s-malloc.c
+++ b/tests/s-malloc.c
@@ -10,7 +10,7 @@ int free_count;
void *malloc(size_t size)
{
- static char buf[MALLOC_BUFSIZE];
+ static char buf[MALLOC_BUFSIZE] __attribute__((aligned(16)));
static unsigned alloc_size;
void *ptr; |
|
Thanks. I can also see that the above change makes it work, but do we always have to fix the target program? |
|
And I cannot reproduce the exception issue. |
No, but this code is tricky. Normally malloc() should return an aligned memory region. |
|
AFAIK, static char buf[MALLOC_BUFSIZE] __attribute__((aligned(8))); |
|
Oh... wait.. I think I tested on a wrong branch. Let me take a look more. |
|
I can add a debugging log as follow. diff --git a/tests/s-malloc.c b/tests/s-malloc.c
index 9afb0e42..89701ebe 100644
--- a/tests/s-malloc.c
+++ b/tests/s-malloc.c
@@ -14,6 +14,7 @@ void *malloc(size_t size)
static unsigned alloc_size;
void *ptr;
+ fprintf(stderr, "buf = %p (%lu)\n", buf, (unsigned long)buf);
size = ALIGN(size, 16);
if (alloc_size + size > sizeof(buf))
return NULL;Then it shows the clang compiled binary shows the alignment is broken in The address of |
|
Another issue here is that attaching uftrace makes the t-malloc (gcc compiled) t-malloc.clang (clang compiled) |
It depends on the implementation. And this code uses 16 byte alignment. |
|
The issue here is that it's not related to libcall. I see that renaming |
|
Maybe uftrace got confused by those |
|
The point of the test is to check if the target process implements its own malloc() and free(). And uftrace (libmcount) uses malloc and free internally that means it calls back to the target code inside libmcount. The purpose of the test is to verify it works. |
|
I think I can close this issue with the change. Please file a separate issue for remaining bugs. |
|
Sure. I think the current change is enough for the basic PLT hooking implementation. Let's discuss in a separate issues. Thanks for the work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Now we have the basic RISC-V (64-bit) support but it still lacks the library call tracing. I'd like to have it before the new release.
This work is a part of RISC-V support (#1503).
The text was updated successfully, but these errors were encountered: