| Message ID | 20220802071240.84626-1-cloudliang@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests: kvm: Fix a compile error in selftests/kvm/rseq_test.c | expand |
On Tue, Aug 02, 2022 at 03:12:40PM +0800, Jinrong Liang wrote: > From: Jinrong Liang <cloudliang@tencent.com> > > The following warning appears when executing: > make -C tools/testing/selftests/kvm > > rseq_test.c: In function ‘main’: > rseq_test.c:237:33: warning: implicit declaration of function ‘gettid’; did you mean ‘getgid’? [-Wimplicit-function-declaration] > (void *)(unsigned long)gettid()); > ^~~~~~ > getgid > /usr/bin/ld: /tmp/ccr5mMko.o: in function `main': > ../kvm/tools/testing/selftests/kvm/rseq_test.c:237: undefined reference to `gettid' > collect2: error: ld returned 1 exit status > make: *** [../lib.mk:173: ../kvm/tools/testing/selftests/kvm/rseq_test] Error 1 The man page says we need #define _GNU_SOURCE #include <unistd.h> which rseq_test.c doesn't have. We have _GNU_SOURCE, but not unistd.h. IOW, I think this patch can be diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index a54d4d05a058..8d3d5eab5e19 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -9,6 +9,7 @@ #include <string.h> #include <signal.h> #include <syscall.h> +#include <unistd.h> #include <sys/ioctl.h> #include <sys/sysinfo.h> #include <asm/barrier.h> Thanks, drew > > Use the more compatible syscall(SYS_gettid) instead of gettid() to fix it. > More subsequent reuse may cause it to be wrapped in a lib file. > > Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > --- > tools/testing/selftests/kvm/rseq_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > index a54d4d05a058..299d316cc759 100644 > --- a/tools/testing/selftests/kvm/rseq_test.c > +++ b/tools/testing/selftests/kvm/rseq_test.c > @@ -229,7 +229,7 @@ int main(int argc, char *argv[]) > ucall_init(vm, NULL); > > pthread_create(&migration_thread, NULL, migration_worker, > - (void *)(unsigned long)gettid()); > + (void *)(unsigned long)syscall(SYS_gettid)); > > for (i = 0; !done; i++) { > vcpu_run(vcpu); > -- > 2.37.1 >
My ldd version is (GNU libc) 2.28, and I get a compilation error in this case. But I use another ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31 is compiling fine. This shows that compilation errors may occur in different GNU libc environments. Would it be more appropriate to use syscall for better compatibility? Thanks, Jinrong Liang Andrew Jones <andrew.jones@linux.dev> 于2022年8月2日周二 23:08写道： > > On Tue, Aug 02, 2022 at 03:12:40PM +0800, Jinrong Liang wrote: > > From: Jinrong Liang <cloudliang@tencent.com> > > > > The following warning appears when executing: > > make -C tools/testing/selftests/kvm > > > > rseq_test.c: In function ‘main’: > > rseq_test.c:237:33: warning: implicit declaration of function ‘gettid’; did you mean ‘getgid’? [-Wimplicit-function-declaration] > > (void *)(unsigned long)gettid()); > > ^~~~~~ > > getgid > > /usr/bin/ld: /tmp/ccr5mMko.o: in function `main': > > ../kvm/tools/testing/selftests/kvm/rseq_test.c:237: undefined reference to `gettid' > > collect2: error: ld returned 1 exit status > > make: *** [../lib.mk:173: ../kvm/tools/testing/selftests/kvm/rseq_test] Error 1 > > The man page says we need > > #define _GNU_SOURCE > #include <unistd.h> > > which rseq_test.c doesn't have. We have _GNU_SOURCE, but not unistd.h. > IOW, I think this patch can be > > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > index a54d4d05a058..8d3d5eab5e19 100644 > --- a/tools/testing/selftests/kvm/rseq_test.c > +++ b/tools/testing/selftests/kvm/rseq_test.c > @@ -9,6 +9,7 @@ > #include <string.h> > #include <signal.h> > #include <syscall.h> > +#include <unistd.h> > #include <sys/ioctl.h> > #include <sys/sysinfo.h> > #include <asm/barrier.h> > > Thanks, > drew > > > > > Use the more compatible syscall(SYS_gettid) instead of gettid() to fix it. > > More subsequent reuse may cause it to be wrapped in a lib file. > > > > Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > > --- > > tools/testing/selftests/kvm/rseq_test.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > > index a54d4d05a058..299d316cc759 100644 > > --- a/tools/testing/selftests/kvm/rseq_test.c > > +++ b/tools/testing/selftests/kvm/rseq_test.c > > @@ -229,7 +229,7 @@ int main(int argc, char *argv[]) > > ucall_init(vm, NULL); > > > > pthread_create(&migration_thread, NULL, migration_worker, > > - (void *)(unsigned long)gettid()); > > + (void *)(unsigned long)syscall(SYS_gettid)); > > > > for (i = 0; !done; i++) { > > vcpu_run(vcpu); > > -- > > 2.37.1 > >
On Wed, Aug 03, 2022 at 09:58:51PM +0800, Jinrong Liang wrote: > My ldd version is (GNU libc) 2.28, and I get a compilation error in this case. > But I use another ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31 is compiling fine. > This shows that compilation errors may occur in different GNU libc environments. > Would it be more appropriate to use syscall for better compatibility? OK, it's a pity, but no big deal to use syscall(). Reviewed-by: Andrew Jones <andrew.jones@linux.dev> Thanks, drew
On Wed, Aug 03, 2022, Andrew Jones wrote: > On Wed, Aug 03, 2022 at 09:58:51PM +0800, Jinrong Liang wrote: > > My ldd version is (GNU libc) 2.28, and I get a compilation error in this case. > > But I use another ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31 is compiling fine. > > This shows that compilation errors may occur in different GNU libc environments. > > Would it be more appropriate to use syscall for better compatibility? > > OK, it's a pity, but no big deal to use syscall(). Ya, https://man7.org/linux/man-pages/man2/gettid.2.html says: The gettid() system call first appeared on Linux in kernel 2.4.11. Library support was added in glibc 2.30. But there are already two other instances of syscall(SYS_gettid) in KVM selftests, tools/testing/selftests/kvm/lib/assert.c even adds a _gettid() wrapper. So rather than having to remember (or discover) to use syscall(SYS_gettid), I wonder if it's possible to conditionally define gettid()? E.g. check for GLIBC version? Or do #define gettid() syscall(SYS_gettid) so that it's always available and simply overrides the library's gettid() if it's provided?
On Wed, Aug 03, 2022 at 04:10:53PM +0000, Sean Christopherson wrote: > On Wed, Aug 03, 2022, Andrew Jones wrote: > > On Wed, Aug 03, 2022 at 09:58:51PM +0800, Jinrong Liang wrote: > > > My ldd version is (GNU libc) 2.28, and I get a compilation error in this case. > > > But I use another ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31 is compiling fine. > > > This shows that compilation errors may occur in different GNU libc environments. > > > Would it be more appropriate to use syscall for better compatibility? > > > > OK, it's a pity, but no big deal to use syscall(). > > Ya, https://man7.org/linux/man-pages/man2/gettid.2.html says: > > The gettid() system call first appeared on Linux in kernel 2.4.11. Library > support was added in glibc 2.30. > > But there are already two other instances of syscall(SYS_gettid) in KVM selftests, > tools/testing/selftests/kvm/lib/assert.c even adds a _gettid() wrapper. Ha! And I found four more in selftests... testing/selftests/powerpc/include/utils.h testing/selftests/proc/proc.h testing/selftests/rseq/param_test.c testing/selftests/sched/cs_prctl_test.c and even more in tools... > > So rather than having to remember (or discover) to use syscall(SYS_gettid), I wonder > if it's possible to conditionally define gettid()? E.g. check for GLIBC version? > Or do > > #define gettid() syscall(SYS_gettid) > > so that it's always available and simply overrides the library's gettid() if it's > provided? Sounds good to me. Now the question is where to put it? kvm_util.h, test_util.h, or maybe we should create a new header just for stuff like this? It doesn't really "fit" in kvm_util.h, but if we put it there, then we greatly reduce the chance that we'll have to revisit this issue again. We could also create a new header just for stuff like this and then include that from kvm_util.h... Thanks, drew
On Wed, Aug 03, 2022, Andrew Jones wrote: > On Wed, Aug 03, 2022 at 04:10:53PM +0000, Sean Christopherson wrote: > > On Wed, Aug 03, 2022, Andrew Jones wrote: > > > On Wed, Aug 03, 2022 at 09:58:51PM +0800, Jinrong Liang wrote: > > > > My ldd version is (GNU libc) 2.28, and I get a compilation error in this case. > > > > But I use another ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31 is compiling fine. > > > > This shows that compilation errors may occur in different GNU libc environments. > > > > Would it be more appropriate to use syscall for better compatibility? > > > > > > OK, it's a pity, but no big deal to use syscall(). > > > > Ya, https://man7.org/linux/man-pages/man2/gettid.2.html says: > > > > The gettid() system call first appeared on Linux in kernel 2.4.11. Library > > support was added in glibc 2.30. > > > > But there are already two other instances of syscall(SYS_gettid) in KVM selftests, > > tools/testing/selftests/kvm/lib/assert.c even adds a _gettid() wrapper. > > Ha! And I found four more in selftests... > > testing/selftests/powerpc/include/utils.h > testing/selftests/proc/proc.h > testing/selftests/rseq/param_test.c > testing/selftests/sched/cs_prctl_test.c > > and even more in tools... Ha, and tools/testing/selftests/sched/cs_prctl_test.c even has the GLIBC crud. #if __GLIBC_PREREQ(2, 30) == 0 #include <sys/syscall.h> static pid_t gettid(void) { return syscall(SYS_gettid); } #endif > > So rather than having to remember (or discover) to use syscall(SYS_gettid), I wonder > > if it's possible to conditionally define gettid()? E.g. check for GLIBC version? > > Or do > > > > #define gettid() syscall(SYS_gettid) > > > > so that it's always available and simply overrides the library's gettid() if it's > > provided? > > Sounds good to me. Now the question is where to put it? kvm_util.h, > test_util.h, or maybe we should create a new header just for stuff > like this? tools/include/uapi/linux/syscall.h? Kind of dirty, but not thaaaat dirty.
On Tue, Aug 02, 2022, Jinrong Liang wrote: > From: Jinrong Liang <cloudliang@tencent.com> > > The following warning appears when executing: > make -C tools/testing/selftests/kvm > > rseq_test.c: In function ‘main’: > rseq_test.c:237:33: warning: implicit declaration of function ‘gettid’; did you mean ‘getgid’? [-Wimplicit-function-declaration] > (void *)(unsigned long)gettid()); > ^~~~~~ > getgid > /usr/bin/ld: /tmp/ccr5mMko.o: in function `main': > ../kvm/tools/testing/selftests/kvm/rseq_test.c:237: undefined reference to `gettid' > collect2: error: ld returned 1 exit status > make: *** [../lib.mk:173: ../kvm/tools/testing/selftests/kvm/rseq_test] Error 1 > > Use the more compatible syscall(SYS_gettid) instead of gettid() to fix it. > More subsequent reuse may cause it to be wrapped in a lib file. > > Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > --- Reviewed-by: Sean Christopherson <seanjc@google.com> Paolo, do you want to grab this for 6.0? It doesn't look like we're going to have a more elegant solution anytime soon...
On 07/09/2022 17:01, Sean Christopherson wrote: > On Tue, Aug 02, 2022, Jinrong Liang wrote: >> From: Jinrong Liang <cloudliang@tencent.com> >> >> The following warning appears when executing: >> make -C tools/testing/selftests/kvm >> >> rseq_test.c: In function ‘main’: >> rseq_test.c:237:33: warning: implicit declaration of function ‘gettid’; did you mean ‘getgid’? [-Wimplicit-function-declaration] >> (void *)(unsigned long)gettid()); >> ^~~~~~ >> getgid >> /usr/bin/ld: /tmp/ccr5mMko.o: in function `main': >> ../kvm/tools/testing/selftests/kvm/rseq_test.c:237: undefined reference to `gettid' >> collect2: error: ld returned 1 exit status >> make: *** [../lib.mk:173: ../kvm/tools/testing/selftests/kvm/rseq_test] Error 1 >> >> Use the more compatible syscall(SYS_gettid) instead of gettid() to fix it. >> More subsequent reuse may cause it to be wrapped in a lib file. >> >> Signed-off-by: Jinrong Liang <cloudliang@tencent.com> >> --- > > Reviewed-by: Sean Christopherson <seanjc@google.com> > Can a 'Cc: stable@vger.kernel.org' be added also as e923b0537d28 got backported to v5.15.58 Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > > Paolo, do you want to grab this for 6.0? It doesn't look like we're going to have > a more elegant solution anytime soon...
On 2022/9/8 00:54, Liam Merwick wrote: > On 07/09/2022 17:01, Sean Christopherson wrote: >> On Tue, Aug 02, 2022, Jinrong Liang wrote: >>> From: Jinrong Liang <cloudliang@tencent.com> >>> >>> The following warning appears when executing: >>> make -C tools/testing/selftests/kvm >>> >>> rseq_test.c: In function ‘main’: >>> rseq_test.c:237:33: warning: implicit declaration of function >>> ‘gettid’; did you mean ‘getgid’? [-Wimplicit-function-declaration] >>> (void *)(unsigned long)gettid()); >>> ^~~~~~ >>> getgid >>> /usr/bin/ld: /tmp/ccr5mMko.o: in function `main': >>> ../kvm/tools/testing/selftests/kvm/rseq_test.c:237: undefined >>> reference to `gettid' >>> collect2: error: ld returned 1 exit status >>> make: *** [../lib.mk:173: >>> ../kvm/tools/testing/selftests/kvm/rseq_test] Error 1 >>> >>> Use the more compatible syscall(SYS_gettid) instead of gettid() to >>> fix it. >>> More subsequent reuse may cause it to be wrapped in a lib file. >>> >>> Signed-off-by: Jinrong Liang <cloudliang@tencent.com> >>> --- >> >> Reviewed-by: Sean Christopherson <seanjc@google.com> >> > > Can a 'Cc: stable@vger.kernel.org' be added also as e923b0537d28 got > backported to v5.15.58 > > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > > >> >> Paolo, do you want to grab this for 6.0? It doesn't look like we're >> going to have >> a more elegant solution anytime soon... > Ping?
On Fri, Sep 16, 2022 at 3:03 AM JinrongLiang <ljr.kernel@gmail.com> wrote: > > > > On 2022/9/8 00:54, Liam Merwick wrote: > > On 07/09/2022 17:01, Sean Christopherson wrote: > >> On Tue, Aug 02, 2022, Jinrong Liang wrote: > >>> From: Jinrong Liang <cloudliang@tencent.com> > >>> > >>> The following warning appears when executing: > >>> make -C tools/testing/selftests/kvm > >>> > >>> rseq_test.c: In function ‘main’: > >>> rseq_test.c:237:33: warning: implicit declaration of function > >>> ‘gettid’; did you mean ‘getgid’? [-Wimplicit-function-declaration] > >>> (void *)(unsigned long)gettid()); > >>> ^~~~~~ > >>> getgid > >>> /usr/bin/ld: /tmp/ccr5mMko.o: in function `main': > >>> ../kvm/tools/testing/selftests/kvm/rseq_test.c:237: undefined > >>> reference to `gettid' > >>> collect2: error: ld returned 1 exit status > >>> make: *** [../lib.mk:173: > >>> ../kvm/tools/testing/selftests/kvm/rseq_test] Error 1 > >>> > >>> Use the more compatible syscall(SYS_gettid) instead of gettid() to > >>> fix it. > >>> More subsequent reuse may cause it to be wrapped in a lib file. > >>> > >>> Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > >>> --- > >> > >> Reviewed-by: Sean Christopherson <seanjc@google.com> > >> > > > > Can a 'Cc: stable@vger.kernel.org' be added also as e923b0537d28 got > > backported to v5.15.58 > > > > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > > > > > >> > >> Paolo, do you want to grab this for 6.0? It doesn't look like we're > >> going to have > >> a more elegant solution anytime soon... > > > Ping? +1, please grab this. Every new branch I create (for new development and to review upstream code) I have to remember to cherry-pick this patch in because I use Clang.
Queued, thanks. Paolo
diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index a54d4d05a058..299d316cc759 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -229,7 +229,7 @@ int main(int argc, char *argv[]) ucall_init(vm, NULL); pthread_create(&migration_thread, NULL, migration_worker, - (void *)(unsigned long)gettid()); + (void *)(unsigned long)syscall(SYS_gettid)); for (i = 0; !done; i++) { vcpu_run(vcpu);