Fix compatibility issues for both 64-bit and 32-bit systems. #22
Conversation
libminkadaptor/src/syscall.S
Outdated
| // Copyright (c) 2025, Qualcomm Innovation Center, Inc. All rights reserved. | ||
| // SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| #if defined(__aarch64__) |
There was a problem hiding this comment.
Instead of adding these #if defined within the file, use a AARCH64 conditional in the CMake file please.
There was a problem hiding this comment.
Why on the earth do we need these assembly bits? We have syscall() in libc, we have other standard ways to call into the kernel. Why do we need this?
There was a problem hiding this comment.
You can read this comment to get some insight into the behavior of this assembly code @lumag : https://github.com/qualcomm/minkipc/blob/main/libminkadaptor/src/supplicant.c#L57
The reason this is written in assembly, is to take care of a race condition found in most glibc libraries, whereby when a kill signal for the thread arrives after glibc checks for it, the signal is ignored and the thread gets indefinitely blocked in the kernel. In-effect, there is a possibility of a race between the arrival of the signal and the checking of the signal, this old LWN article digs into it more properly if you're interested: https://lwn.net/Articles/683118/
libminkadaptor/src/supplicant.c
Outdated
| uint64_t pc_current = (uint64_t)ucontext->uc_mcontext.pc; | ||
| uint64_t addr_recv_ioctl = (uint64_t)recv_ioctl; | ||
| uint64_t addr_recv = (uint64_t)recv; | ||
| uint64_t pc_current = (uint64_t)get_pc_from_ucontext(ucontext); |
There was a problem hiding this comment.
Why not make pc_current, addr_recv_ioctl and addr_recv uintptr_t?
| #include <stddef.h> | ||
|
|
||
| #ifndef container_of | ||
| #define container_of(ptr, type, member) ({ \ |
There was a problem hiding this comment.
This can be simplified, please see: https://github.com/quic/quic-teec/blob/main/libqcomtee/include/qcomtee_object.h#L242
There was a problem hiding this comment.
Also qlist.h is only for defining the Qlist interfaces and macros, please move it either directly to the .c file using it or find another generic header providing generic helper macros.
There was a problem hiding this comment.
offsetof (<stddef. h>/<cstddef>) is a part of the standard library, supported by mainstream compilers such as GCC, Clang, MSVC, ICC, as well as various operating systems/RTOS.
__Builtin_offsetof is only reliable in the ecosystem of GCC/CLang; When ported to MSVC or other compilers, it may be directly unavailable or require macro replacement.
Whereby if the code needs to be ported between different compilers and systems, it is recommended to prioritize using standard offsetof.
There was a problem hiding this comment.
Alright sure, if you want to keep it. However like I said, please move to either .c or a generic header, don't keep this in qlist.h.
| QLIST_FOR_ALL(list, node) | ||
| { | ||
| l_item = (list_item *)node; | ||
| l_item = container_of(node, list_item, qn); |
There was a problem hiding this comment.
Please fix the typos in the commit message.
| #include <stddef.h> | ||
|
|
||
| #ifndef container_of | ||
| #define container_of(ptr, type, member) ({ \ |
There was a problem hiding this comment.
As mentioned above, better move it directly to the .c file.
There was a problem hiding this comment.
I don't recommend defining container_of in a .c file, as it's a generic macro commonly used for list node processing.
| my_rsp->buf.st_dev = (uint64_t)buf.st_dev; | ||
| #if !defined(__aarch64__) | ||
| memcpy(my_rsp->buf.__pad0, buf.__pad0, 4); | ||
| my_rsp->buf.__st_ino = (uint32_t)buf.__st_ino; |
There was a problem hiding this comment.
On line 305, why delete it? Why not use my_rsp->buf.__st_ino = (uint32_t)buf.st_ino?
There was a problem hiding this comment.
The variable was assigned a value twice.
Please see line 320.
| } else { | ||
| my_rsp->buf.st_dev = (uint64_t)buf.st_dev; | ||
| #if !defined(__aarch64__) | ||
| memcpy(my_rsp->buf.__pad0, buf.__pad0, 4); |
There was a problem hiding this comment.
Instead of deleting memcpy() I think it's better to set 0 in my_rsp0->buf.__pad0?
There was a problem hiding this comment.
The variable was assigned a value twice.
Please see line 319.
| my_rsp->buf.st_ctim = (uint32_t)((struct timespec)buf.st_ctim).tv_sec; | ||
| my_rsp->buf.st_ctim_nsec = (uint32_t)((struct timespec)buf.st_ctim).tv_nsec; | ||
| my_rsp->buf.st_ino = (uint64_t)buf.st_ino; | ||
| #if !defined(__aarch64__) |
There was a problem hiding this comment.
Why delete this #if section? I think it compiles perfectly and is valid.
| my_rsp->ret = -1; | ||
| } else { | ||
| my_rsp->buf.st_dev = (uint64_t)buf.st_dev; | ||
| #if !defined(__aarch64__) |
There was a problem hiding this comment.
Same comments as above.
There was a problem hiding this comment.
The variable was assigned a value twice.
Please see line 375.
| my_rsp->buf.st_ctim = (uint32_t)((struct timespec)buf.st_ctim).tv_sec; | ||
| my_rsp->buf.st_ctim_nsec = (uint32_t)((struct timespec)buf.st_ctim).tv_nsec; | ||
| my_rsp->buf.st_ino = (uint64_t)buf.st_ino; | ||
| #if !defined(__aarch64__) |
There was a problem hiding this comment.
Same comments as above.
jiaxli-QC
force-pushed
the
main
branch
2 times, most recently
from
a947c17 to
512097e
Compare
harshaldev27
left a comment
harshaldev27
left a comment
There was a problem hiding this comment.
Please update the commit messages. They should follow the right commit styles.
There was a problem hiding this comment.
Can you fix the typo in the commit message? syscall.S.
Also, please use the part before colon ':' to refer to the module.
libminkdaptor: Add 32-bit compilation support via syscall32.S
I would also suggest you use co-pilot for fixing grammatical or other minor mistakes.
Also explain what kind of modifications have you made to the assembly.
| extern int recv(void); | ||
| extern int recv_skip(void); | ||
|
|
||
| static inline uintptr_t get_pc_from_ucontext(ucontext_t *ucontext) { |
There was a problem hiding this comment.
Can you move these inline functions to supplicant.h?
There was a problem hiding this comment.
I feel okay about this.
But since this PR addresses a 32-bit system compilation issue, could this modification potentially cause confusion with the content?
Fixed compatible issue for syscal on both arm32 and arm64 system. The assembly instructions have been modified. Signed-off-by: Jiaxing Li <jiaxing.li@oss.qualcomm.com>
mcontext_t has no member named pc on 32-bit system. Unable to access the pc member from mcontext_t on 32-bit system. Signed-off-by: Jiaxing Li <jiaxing.li@oss.qualcomm.com>
Forcefully converting a QNode pointer to list_item *. Unable to meet alignment requirements. Use container_of macro to complete the address translation. Signed-off-by: Jiaxing Li <jiaxing.li@oss.qualcomm.com>
Directly accessed the non-public internal fields of glibc (__pad0, __st_ino). On 32-bit ARM, the layout of struct stat and these field names do not exist. Only access fields that comply with the POSIX standard to modify compatibility issue. Signed-off-by: Jiaxing Li <jiaxing.li@oss.qualcomm.com>
Replace 'uint64_t' type with 'uintptr_t' type. Use '%zu' to print the size_t variable, as the size_t type length is different in 32-bit and 64-bit systems. Signed-off-by: Jiaxing Li <jiaxing.li@oss.qualcomm.com>
jiaxli-QC
changed the title
3 participants
Fix compatibility issues for both 64-bit and 32-bit systems.