| Message ID | 1609355503-7981-1-git-send-email-roid@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [iproute2] build: Fix link errors on some systems | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Roi Dayan <roid@nvidia.com> writes: > Since moving get_rate() and get_size() from tc to lib, on some > systems we fail to link because of missing the math lib. > Move the link flag from tc makefile to the main makefile. Hmm, yeah, it gets optimized out on x86-64. The issue is reproducible on any platform with -O0. > ../lib/libutil.a(utils.o): In function `get_rate': > utils.c:(.text+0x10dc): undefined reference to `floor' > ../lib/libutil.a(utils.o): In function `get_size': > utils.c:(.text+0x1394): undefined reference to `floor' > ../lib/libutil.a(json_print.o): In function `sprint_size': > json_print.c:(.text+0x14c0): undefined reference to `rint' > json_print.c:(.text+0x14f4): undefined reference to `rint' > json_print.c:(.text+0x157c): undefined reference to `rint' > > Fixes: f3be0e6366ac ("lib: Move get_rate(), get_rate64() from tc here") > Fixes: 44396bdfcc0a ("lib: Move get_size() from tc here") > Fixes: adbe5de96662 ("lib: Move sprint_size() from tc here, add print_size()") > Signed-off-by: Roi Dayan <roid@nvidia.com> > --- > Makefile | 1 + > tc/Makefile | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index e64c65992585..2a604ec58905 100644 > --- a/Makefile > +++ b/Makefile > @@ -59,6 +59,7 @@ SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man > > LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a > LDLIBS += $(LIBNETLINK) > +LDFLAGS += -lm > > all: config.mk > @set -e; \ > diff --git a/tc/Makefile b/tc/Makefile > index 5a517af20b7c..8d91900716c1 100644 > --- a/tc/Makefile > +++ b/tc/Makefile > @@ -110,7 +110,7 @@ ifneq ($(TC_CONFIG_NO_XT),y) > endif > > TCOBJ += $(TCMODULES) > -LDLIBS += -L. -lm > +LDLIBS += -L. > > ifeq ($(SHARED_LIBS),y) > LDLIBS += -ldl This will work, but it will give a libm dependency to all the tools. util.c currently tries not to do that: /* emulate ceil() without having to bring-in -lm and always be >= 1 */ *val = t; if (*val < t) *val += 1; I think that just adding an unnecessary -lm is more of a tidiness issue than anything else. One way to avoid it is to split the -lm deps out from util.c / json_print.c to like util_math.c / json_print_math.c. That way they will be in an .o of their own, and won't be linked in unless the binary in question needs the code. Then the binaries that do call it can keep on linking in -lm like they did so far. Thoughts?
On 2021-01-04 6:07 PM, Petr Machata wrote: > > Roi Dayan <roid@nvidia.com> writes: > >> Since moving get_rate() and get_size() from tc to lib, on some >> systems we fail to link because of missing the math lib. >> Move the link flag from tc makefile to the main makefile. > > Hmm, yeah, it gets optimized out on x86-64. The issue is reproducible > on any platform with -O0. > >> ../lib/libutil.a(utils.o): In function `get_rate': >> utils.c:(.text+0x10dc): undefined reference to `floor' >> ../lib/libutil.a(utils.o): In function `get_size': >> utils.c:(.text+0x1394): undefined reference to `floor' >> ../lib/libutil.a(json_print.o): In function `sprint_size': >> json_print.c:(.text+0x14c0): undefined reference to `rint' >> json_print.c:(.text+0x14f4): undefined reference to `rint' >> json_print.c:(.text+0x157c): undefined reference to `rint' >> >> Fixes: f3be0e6366ac ("lib: Move get_rate(), get_rate64() from tc here") >> Fixes: 44396bdfcc0a ("lib: Move get_size() from tc here") >> Fixes: adbe5de96662 ("lib: Move sprint_size() from tc here, add print_size()") >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> --- >> Makefile | 1 + >> tc/Makefile | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/Makefile b/Makefile >> index e64c65992585..2a604ec58905 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -59,6 +59,7 @@ SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man >> >> LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a >> LDLIBS += $(LIBNETLINK) >> +LDFLAGS += -lm >> >> all: config.mk >> @set -e; \ >> diff --git a/tc/Makefile b/tc/Makefile >> index 5a517af20b7c..8d91900716c1 100644 >> --- a/tc/Makefile >> +++ b/tc/Makefile >> @@ -110,7 +110,7 @@ ifneq ($(TC_CONFIG_NO_XT),y) >> endif >> >> TCOBJ += $(TCMODULES) >> -LDLIBS += -L. -lm >> +LDLIBS += -L. >> >> ifeq ($(SHARED_LIBS),y) >> LDLIBS += -ldl > > This will work, but it will give a libm dependency to all the tools. > util.c currently tries not to do that: > > /* emulate ceil() without having to bring-in -lm and always be >= 1 */ > *val = t; > if (*val < t) > *val += 1; > > I think that just adding an unnecessary -lm is more of a tidiness issue > than anything else. One way to avoid it is to split the -lm deps out > from util.c / json_print.c to like util_math.c / json_print_math.c. That > way they will be in an .o of their own, and won't be linked in unless > the binary in question needs the code. Then the binaries that do call it > can keep on linking in -lm like they did so far. > > Thoughts? > ok fine by me.
On 2021-01-06 10:42 AM, Roi Dayan wrote: > > > On 2021-01-04 6:07 PM, Petr Machata wrote: >> >> Roi Dayan <roid@nvidia.com> writes: >> >>> Since moving get_rate() and get_size() from tc to lib, on some >>> systems we fail to link because of missing the math lib. >>> Move the link flag from tc makefile to the main makefile. >> >> Hmm, yeah, it gets optimized out on x86-64. The issue is reproducible >> on any platform with -O0. >> >>> ../lib/libutil.a(utils.o): In function `get_rate': >>> utils.c:(.text+0x10dc): undefined reference to `floor' >>> ../lib/libutil.a(utils.o): In function `get_size': >>> utils.c:(.text+0x1394): undefined reference to `floor' >>> ../lib/libutil.a(json_print.o): In function `sprint_size': >>> json_print.c:(.text+0x14c0): undefined reference to `rint' >>> json_print.c:(.text+0x14f4): undefined reference to `rint' >>> json_print.c:(.text+0x157c): undefined reference to `rint' >>> >>> Fixes: f3be0e6366ac ("lib: Move get_rate(), get_rate64() from tc here") >>> Fixes: 44396bdfcc0a ("lib: Move get_size() from tc here") >>> Fixes: adbe5de96662 ("lib: Move sprint_size() from tc here, add >>> print_size()") >>> Signed-off-by: Roi Dayan <roid@nvidia.com> >>> --- >>> Makefile | 1 + >>> tc/Makefile | 2 +- >>> 2 files changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/Makefile b/Makefile >>> index e64c65992585..2a604ec58905 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -59,6 +59,7 @@ SUBDIRS=lib ip tc bridge misc netem genl tipc >>> devlink rdma dcb man >>> LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a >>> LDLIBS += $(LIBNETLINK) >>> +LDFLAGS += -lm >>> all: config.mk >>> @set -e; \ >>> diff --git a/tc/Makefile b/tc/Makefile >>> index 5a517af20b7c..8d91900716c1 100644 >>> --- a/tc/Makefile >>> +++ b/tc/Makefile >>> @@ -110,7 +110,7 @@ ifneq ($(TC_CONFIG_NO_XT),y) >>> endif >>> TCOBJ += $(TCMODULES) >>> -LDLIBS += -L. -lm >>> +LDLIBS += -L. >>> ifeq ($(SHARED_LIBS),y) >>> LDLIBS += -ldl >> >> This will work, but it will give a libm dependency to all the tools. >> util.c currently tries not to do that: >> >> /* emulate ceil() without having to bring-in -lm and always be >= >> 1 */ >> *val = t; >> if (*val < t) >> *val += 1; >> >> I think that just adding an unnecessary -lm is more of a tidiness issue >> than anything else. One way to avoid it is to split the -lm deps out >> from util.c / json_print.c to like util_math.c / json_print_math.c. That >> way they will be in an .o of their own, and won't be linked in unless >> the binary in question needs the code. Then the binaries that do call it >> can keep on linking in -lm like they did so far. >> >> Thoughts? >> > > ok fine by me. I looked at this and for get_size()/rate/.. it went smooth. but for print_color_size() there is an issue that it uses _IS_JSON_CONTEXT and statuic *_jw which are defined in json_print.c Is it ok to expose those in json_print.h now so json_print_math.c could use?
Roi Dayan <roid@nvidia.com> writes: > On 2021-01-06 10:42 AM, Roi Dayan wrote: >> >> On 2021-01-04 6:07 PM, Petr Machata wrote: >>> >>> I think that just adding an unnecessary -lm is more of a tidiness issue >>> than anything else. One way to avoid it is to split the -lm deps out >>> from util.c / json_print.c to like util_math.c / json_print_math.c. That >>> way they will be in an .o of their own, and won't be linked in unless >>> the binary in question needs the code. Then the binaries that do call it >>> can keep on linking in -lm like they did so far. >>> >>> Thoughts? >>> >> ok fine by me. > > I looked at this and for get_size()/rate/.. it went smooth. > but for print_color_size() there is an issue that it uses > _IS_JSON_CONTEXT and statuic *_jw which are defined in json_print.c > Is it ok to expose those in json_print.h now so json_print_math.c > could use? You don't need json_print_math.h IMHO, it can all be backed by the same header, just different implementation modules. From the API point of view, I don't think the user should really care which of the symbols use math (though of course they will have to know whether to link in -lm). Regarding the publishing, the _jw reference can be changed to a call to is_json_context(), which does the same thing. Then _jw can stay private in json_print.c. Exposing an _IS_JSON_CONTEXT / _IS_FP_CONTEXT might be odd on account of the initial underscore, but since it's only used in implementations, maybe it's OK?
On 2021-01-06 3:16 PM, Petr Machata wrote: > > Roi Dayan <roid@nvidia.com> writes: > >> On 2021-01-06 10:42 AM, Roi Dayan wrote: >>> >>> On 2021-01-04 6:07 PM, Petr Machata wrote: >>>> >>>> I think that just adding an unnecessary -lm is more of a tidiness issue >>>> than anything else. One way to avoid it is to split the -lm deps out >>>> from util.c / json_print.c to like util_math.c / json_print_math.c. That >>>> way they will be in an .o of their own, and won't be linked in unless >>>> the binary in question needs the code. Then the binaries that do call it >>>> can keep on linking in -lm like they did so far. >>>> >>>> Thoughts? >>>> >>> ok fine by me. >> >> I looked at this and for get_size()/rate/.. it went smooth. >> but for print_color_size() there is an issue that it uses >> _IS_JSON_CONTEXT and statuic *_jw which are defined in json_print.c >> Is it ok to expose those in json_print.h now so json_print_math.c >> could use? > > You don't need json_print_math.h IMHO, it can all be backed by the same > header, just different implementation modules. From the API point of > view, I don't think the user should really care which of the symbols use > math (though of course they will have to know whether to link in -lm). right ok. > > Regarding the publishing, the _jw reference can be changed to a call to > is_json_context(), which does the same thing. Then _jw can stay private > in json_print.c. > > Exposing an _IS_JSON_CONTEXT / _IS_FP_CONTEXT might be odd on account of > the initial underscore, but since it's only used in implementations, > maybe it's OK? > With is_json_context() I cannot check the type passed by the caller. i.e. PRINT_JSON, PRINT_FP, PRINT_ANY. From what I see now callers use is_json_context() to decide which print to use. but in print_color_size() I should check the type to decide which print.
Roi Dayan <roid@nvidia.com> writes: > On 2021-01-06 3:16 PM, Petr Machata wrote: >> Regarding the publishing, the _jw reference can be changed to a call to >> is_json_context(), which does the same thing. Then _jw can stay private >> in json_print.c. >> Exposing an _IS_JSON_CONTEXT / _IS_FP_CONTEXT might be odd on account of >> the initial underscore, but since it's only used in implementations, >> maybe it's OK? >> > > With is_json_context() I cannot check the type passed by the caller. > i.e. PRINT_JSON, PRINT_FP, PRINT_ANY. I meant s/_jw/is_json_context()/. Like this: #define _IS_JSON_CONTEXT(type) \ ((type & PRINT_JSON || type & PRINT_ANY) && is_json_context()) Then _jw can stay private.
On Wed, 6 Jan 2021 10:42:35 +0200 Roi Dayan <roid@nvidia.com> wrote: > > > > I think that just adding an unnecessary -lm is more of a tidiness issue > > than anything else. One way to avoid it is to split the -lm deps out > > from util.c / json_print.c to like util_math.c / json_print_math.c. That > > way they will be in an .o of their own, and won't be linked in unless > > the binary in question needs the code. Then the binaries that do call it > > can keep on linking in -lm like they did so far. > > > > Thoughts? > > Adding -lm to just some tools is not really required. The linker will ignore the shared library if not used.
Stephen Hemminger <stephen@networkplumber.org> writes: > On Wed, 6 Jan 2021 10:42:35 +0200 > Roi Dayan <roid@nvidia.com> wrote: > >> > >> > I think that just adding an unnecessary -lm is more of a tidiness issue >> > than anything else. One way to avoid it is to split the -lm deps out >> > from util.c / json_print.c to like util_math.c / json_print_math.c. That >> > way they will be in an .o of their own, and won't be linked in unless >> > the binary in question needs the code. Then the binaries that do call it >> > can keep on linking in -lm like they did so far. >> > >> > Thoughts? >> > > > Adding -lm to just some tools is not really required. > The linker will ignore the shared library if not used. I don't think that's true. $ echo 'int main() {}' | gcc -x c /dev/stdin -lm $ ldd a.out linux-vdso.so.1 (0x00007fff903e5000) libm.so.6 => /lib64/libm.so.6 (0x00007fa475d75000) libc.so.6 => /lib64/libc.so.6 (0x00007fa475bab000) /lib64/ld-linux-x86-64.so.2 (0x00007fa475ee4000) Anyway, without the split to math / non-math modules, the DSO will actually end up being necessary, because the undefined references to floor() etc. in util.o / json_print.o will bring it in. Except of course not everybody actually uses the code...
On 2021-01-06 4:24 PM, Petr Machata wrote: > > Roi Dayan <roid@nvidia.com> writes: > >> On 2021-01-06 3:16 PM, Petr Machata wrote: >>> Regarding the publishing, the _jw reference can be changed to a call to >>> is_json_context(), which does the same thing. Then _jw can stay private >>> in json_print.c. >>> Exposing an _IS_JSON_CONTEXT / _IS_FP_CONTEXT might be odd on account of >>> the initial underscore, but since it's only used in implementations, >>> maybe it's OK? >>> >> >> With is_json_context() I cannot check the type passed by the caller. >> i.e. PRINT_JSON, PRINT_FP, PRINT_ANY. > > I meant s/_jw/is_json_context()/. Like this: > > #define _IS_JSON_CONTEXT(type) \ > ((type & PRINT_JSON || type & PRINT_ANY) && is_json_context()) > > Then _jw can stay private. > right. thanks. i'll submit v2 for review.
diff --git a/Makefile b/Makefile index e64c65992585..2a604ec58905 100644 --- a/Makefile +++ b/Makefile @@ -59,6 +59,7 @@ SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a LDLIBS += $(LIBNETLINK) +LDFLAGS += -lm all: config.mk @set -e; \ diff --git a/tc/Makefile b/tc/Makefile index 5a517af20b7c..8d91900716c1 100644 --- a/tc/Makefile +++ b/tc/Makefile @@ -110,7 +110,7 @@ ifneq ($(TC_CONFIG_NO_XT),y) endif TCOBJ += $(TCMODULES) -LDLIBS += -L. -lm +LDLIBS += -L. ifeq ($(SHARED_LIBS),y) LDLIBS += -ldl
Since moving get_rate() and get_size() from tc to lib, on some systems we fail to link because of missing the math lib. Move the link flag from tc makefile to the main makefile. ../lib/libutil.a(utils.o): In function `get_rate': utils.c:(.text+0x10dc): undefined reference to `floor' ../lib/libutil.a(utils.o): In function `get_size': utils.c:(.text+0x1394): undefined reference to `floor' ../lib/libutil.a(json_print.o): In function `sprint_size': json_print.c:(.text+0x14c0): undefined reference to `rint' json_print.c:(.text+0x14f4): undefined reference to `rint' json_print.c:(.text+0x157c): undefined reference to `rint' Fixes: f3be0e6366ac ("lib: Move get_rate(), get_rate64() from tc here") Fixes: 44396bdfcc0a ("lib: Move get_size() from tc here") Fixes: adbe5de96662 ("lib: Move sprint_size() from tc here, add print_size()") Signed-off-by: Roi Dayan <roid@nvidia.com> --- Makefile | 1 + tc/Makefile | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)