| Message ID | 20240705145910.32736-3-andreistan2003@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/libs/store: add missing USE_PTHREAD to target .o | expand |
On 05/07/2024 3:59 pm, Andrei Stan wrote: > Currently only shared libraries build with USE_PTHREAD enabled. > > This patch adds the macro also to xs.o. > > Backport: 4.18+ # maybe older > Signed-off-by: Andrei Stan <andreistan2003@gmail.com> > --- > tools/libs/store/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile > index 0649cf8307..c6f147c72f 100644 > --- a/tools/libs/store/Makefile > +++ b/tools/libs/store/Makefile > @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h > CFLAGS += $(CFLAGS_libxentoolcore) > > xs.opic: CFLAGS += -DUSE_PTHREAD > +xs.o: CFLAGS += -DUSE_PTHREAD > ifeq ($(CONFIG_Linux),y) > xs.opic: CFLAGS += -DUSE_DLSYM > endif Funnily enough, I did wonder whether that mattered recently. I guess it does. CC Oleksii for a view to 4.19. Also, we should transform the Backport: tag into a Fixes: tag if there's a suitable one to pick. ~Andrew
On 05.07.24 16:59, Andrei Stan wrote: > Currently only shared libraries build with USE_PTHREAD enabled. > > This patch adds the macro also to xs.o. > > Backport: 4.18+ # maybe older > Signed-off-by: Andrei Stan <andreistan2003@gmail.com> Commit dde3e02b7068 did that explicitly, stating that phtreads are not compatible with static linking. Was that reasoning wrong? Juergen > --- > tools/libs/store/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile > index 0649cf8307..c6f147c72f 100644 > --- a/tools/libs/store/Makefile > +++ b/tools/libs/store/Makefile > @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h > CFLAGS += $(CFLAGS_libxentoolcore) > > xs.opic: CFLAGS += -DUSE_PTHREAD > +xs.o: CFLAGS += -DUSE_PTHREAD > ifeq ($(CONFIG_Linux),y) > xs.opic: CFLAGS += -DUSE_DLSYM > endif
On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote: > > On 05.07.24 16:59, Andrei Stan wrote: > > Currently only shared libraries build with USE_PTHREAD enabled. > > > > This patch adds the macro also to xs.o. > > > > Backport: 4.18+ # maybe older > > Signed-off-by: Andrei Stan <andreistan2003@gmail.com> > > Commit dde3e02b7068 did that explicitly, stating that phtreads are > not compatible with static linking. > > Was that reasoning wrong? > > Juergen > --- > > --- > > tools/libs/store/Makefile | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile > > index 0649cf8307..c6f147c72f 100644 > > --- a/tools/libs/store/Makefile > > +++ b/tools/libs/store/Makefile > > @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h > > CFLAGS += $(CFLAGS_libxentoolcore) > > > > xs.opic: CFLAGS += -DUSE_PTHREAD > > +xs.o: CFLAGS += -DUSE_PTHREAD > > ifeq ($(CONFIG_Linux),y) > > xs.opic: CFLAGS += -DUSE_DLSYM > > endif It was a pretty nasty situation, giving some context, this is for a Go based CLI tool and some things in there are multithreaded, but i don't think calling in the bindings happens anywhere in parallel. Without this patch there would be some sort of race conditions (or so I assume from the debugging i've done) happening withing the xen/tools code, making it impossible to start a domain. In this case there were no issues in linking pthreads statically. I was not even aware of that being a possible issue. Is it really? Also I am not too sure how much that code path is actually tested, probably the majority of the testing is done with dynamic builds. Andrei
Ping On Fri, Jul 5, 2024 at 7:05 PM Andrei Stan <andreistan2003@gmail.com> wrote: > > On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote: > > > > On 05.07.24 16:59, Andrei Stan wrote: > > > Currently only shared libraries build with USE_PTHREAD enabled. > > > > > > This patch adds the macro also to xs.o. > > > > > > Backport: 4.18+ # maybe older > > > Signed-off-by: Andrei Stan <andreistan2003@gmail.com> > > > > Commit dde3e02b7068 did that explicitly, stating that phtreads are > > not compatible with static linking. > > > > Was that reasoning wrong? > > > > Juergen > > --- > > > --- > > > tools/libs/store/Makefile | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile > > > index 0649cf8307..c6f147c72f 100644 > > > --- a/tools/libs/store/Makefile > > > +++ b/tools/libs/store/Makefile > > > @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h > > > CFLAGS += $(CFLAGS_libxentoolcore) > > > > > > xs.opic: CFLAGS += -DUSE_PTHREAD > > > +xs.o: CFLAGS += -DUSE_PTHREAD > > > ifeq ($(CONFIG_Linux),y) > > > xs.opic: CFLAGS += -DUSE_DLSYM > > > endif > > It was a pretty nasty situation, giving some context, this is for a Go based CLI > tool and some things in there are multithreaded, but i don't think > calling in the > bindings happens anywhere in parallel. Without this patch there would be > some sort of race conditions (or so I assume from the debugging i've done) > happening withing the xen/tools code, making it impossible to start a domain. > > In this case there were no issues in linking pthreads statically. I was not even > aware of that being a possible issue. Is it really? > > Also I am not too sure how much that code path is actually tested, probably the > majority of the testing is done with dynamic builds. > > Andrei
On 31.08.2024 12:34, Andrei Stan wrote: > Ping This is pinging what exactly? From ... > On Fri, Jul 5, 2024 at 7:05 PM Andrei Stan <andreistan2003@gmail.com> wrote: >> >> On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote: >>> >>> On 05.07.24 16:59, Andrei Stan wrote: >>>> Currently only shared libraries build with USE_PTHREAD enabled. >>>> >>>> This patch adds the macro also to xs.o. >>>> >>>> Backport: 4.18+ # maybe older >>>> Signed-off-by: Andrei Stan <andreistan2003@gmail.com> >>> >>> Commit dde3e02b7068 did that explicitly, stating that phtreads are >>> not compatible with static linking. >>> >>> Was that reasoning wrong? ... Jürgen's question it imo follows that ... >>>> --- >>>> tools/libs/store/Makefile | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile >>>> index 0649cf8307..c6f147c72f 100644 >>>> --- a/tools/libs/store/Makefile >>>> +++ b/tools/libs/store/Makefile >>>> @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h >>>> CFLAGS += $(CFLAGS_libxentoolcore) >>>> >>>> xs.opic: CFLAGS += -DUSE_PTHREAD >>>> +xs.o: CFLAGS += -DUSE_PTHREAD >>>> ifeq ($(CONFIG_Linux),y) >>>> xs.opic: CFLAGS += -DUSE_DLSYM >>>> endif >> >> It was a pretty nasty situation, giving some context, this is for a Go based CLI >> tool and some things in there are multithreaded, but i don't think >> calling in the >> bindings happens anywhere in parallel. Without this patch there would be >> some sort of race conditions (or so I assume from the debugging i've done) >> happening withing the xen/tools code, making it impossible to start a domain. >> >> In this case there were no issues in linking pthreads statically. I was not even >> aware of that being a possible issue. Is it really? >> >> Also I am not too sure how much that code path is actually tested, probably the >> majority of the testing is done with dynamic builds. ... this answer is insufficient, from two angles: - You do in no way answer the question itself, as such an answer would clearly need to say something about the commit Jürgen pointed you at. - It needs to be in the patch description, not just in a reply on list. IOW a re-submission is needed anyway. Jan
On 05.07.24 18:05, Andrei Stan wrote: > On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote: >> >> On 05.07.24 16:59, Andrei Stan wrote: >>> Currently only shared libraries build with USE_PTHREAD enabled. >>> >>> This patch adds the macro also to xs.o. >>> >>> Backport: 4.18+ # maybe older >>> Signed-off-by: Andrei Stan <andreistan2003@gmail.com> >> >> Commit dde3e02b7068 did that explicitly, stating that phtreads are >> not compatible with static linking. >> >> Was that reasoning wrong? >> >> Juergen >> --- >>> --- >>> tools/libs/store/Makefile | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile >>> index 0649cf8307..c6f147c72f 100644 >>> --- a/tools/libs/store/Makefile >>> +++ b/tools/libs/store/Makefile >>> @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h >>> CFLAGS += $(CFLAGS_libxentoolcore) >>> >>> xs.opic: CFLAGS += -DUSE_PTHREAD >>> +xs.o: CFLAGS += -DUSE_PTHREAD >>> ifeq ($(CONFIG_Linux),y) >>> xs.opic: CFLAGS += -DUSE_DLSYM >>> endif > > It was a pretty nasty situation, giving some context, this is for a Go based CLI > tool and some things in there are multithreaded, but i don't think > calling in the > bindings happens anywhere in parallel. Without this patch there would be > some sort of race conditions (or so I assume from the debugging i've done) > happening withing the xen/tools code, making it impossible to start a domain. > > In this case there were no issues in linking pthreads statically. I was not even > aware of that being a possible issue. Is it really? I don't know. Said commit suggests it at least was an issue in the past. No idea what was wrong with it and whether the situation has changed since then. Another question: why are you linking statically? Dynamic linking seems to be the standard for Go. > Also I am not too sure how much that code path is actually tested, probably the > majority of the testing is done with dynamic builds. I think so, yes. I'm not aware of any tests using static libraries. Juergen
diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile index 0649cf8307..c6f147c72f 100644 --- a/tools/libs/store/Makefile +++ b/tools/libs/store/Makefile @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h CFLAGS += $(CFLAGS_libxentoolcore) xs.opic: CFLAGS += -DUSE_PTHREAD +xs.o: CFLAGS += -DUSE_PTHREAD ifeq ($(CONFIG_Linux),y) xs.opic: CFLAGS += -DUSE_DLSYM endif
Currently only shared libraries build with USE_PTHREAD enabled. This patch adds the macro also to xs.o. Backport: 4.18+ # maybe older Signed-off-by: Andrei Stan <andreistan2003@gmail.com> --- tools/libs/store/Makefile | 1 + 1 file changed, 1 insertion(+)