[PATCH iproute2] build: Fix link errors on some systems

[PATCH iproute2] build: Fix link errors on some systems

[Roi Dayan]

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(-)

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
-- 
1.8.3.1

Re: [PATCH iproute2] build: Fix link errors on some systems

[Petr Machata]

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?

Re: [PATCH iproute2] build: Fix link errors on some systems

[Roi Dayan]

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.

Re: [PATCH iproute2] build: Fix link errors on some systems

[Roi Dayan]

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?

Re: [PATCH iproute2] build: Fix link errors on some systems

[Petr Machata]

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?

Re: [PATCH iproute2] build: Fix link errors on some systems

[Roi Dayan]

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.

Re: [PATCH iproute2] build: Fix link errors on some systems

[Petr Machata]

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.

Re: [PATCH iproute2] build: Fix link errors on some systems

[Roi Dayan]

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.

Re: [PATCH iproute2] build: Fix link errors on some systems

[Stephen Hemminger]

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.

Re: [PATCH iproute2] build: Fix link errors on some systems

[Petr Machata]

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...