Unused macro

Unused macro

[Alejandro Colomar (man-pages)]

Hi,

I was updating the includes on some manual pages, when I found that a
macro used ARRAY_SIZE() without including a header that defines it.
That surprised me, because it would more than likely result in a compile
error, but of course, the macro wasn't being used:

.../linux$ grep -rn SCTP_CHUNKMAP_IS_ALL_SET
include/uapi/linux/netfilter/xt_sctp.h:80:#define
SCTP_CHUNKMAP_IS_ALL_SET(chunkmap) \
.../linux$

I'd remove it myself and send a patch, but there are probably more
unused macros/functions in the same or related headers, and I see that
many years ago you did some clean up, so maybe you want to clean up even
more, and maybe remove an entire set of functions/macros.

Cheers,

Alex

-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: Unused macro

[Florian Westphal]

Alejandro Colomar (man-pages) <alx.manpages@gmail.com> wrote:
> I was updating the includes on some manual pages, when I found that a
> macro used ARRAY_SIZE() without including a header that defines it.
> That surprised me, because it would more than likely result in a compile
> error, but of course, the macro wasn't being used:
> 
> .../linux$ grep -rn SCTP_CHUNKMAP_IS_ALL_SET
> include/uapi/linux/netfilter/xt_sctp.h:80:#define
> SCTP_CHUNKMAP_IS_ALL_SET(chunkmap) \
> .../linux$

This is an UAPI header, this macro is used by userspace software, e.g.
iptables.

Re: Unused macro

[Alejandro Colomar (man-pages)]

Hello Florian,

On 4/4/21 10:05 PM, Florian Westphal wrote:
> Alejandro Colomar (man-pages) <alx.manpages@gmail.com> wrote:
>> I was updating the includes on some manual pages, when I found that a
>> macro used ARRAY_SIZE() without including a header that defines it.
>> That surprised me, because it would more than likely result in a compile
>> error, but of course, the macro wasn't being used:
>>
>> .../linux$ grep -rn SCTP_CHUNKMAP_IS_ALL_SET
>> include/uapi/linux/netfilter/xt_sctp.h:80:#define
>> SCTP_CHUNKMAP_IS_ALL_SET(chunkmap) \
>> .../linux$
> 
> This is an UAPI header, this macro is used by userspace software, e.g.
> iptables.
> 

Ahh, I see.  Thanks.

Then we still have the issue that ARRAY_SIZE is not defined in that
header (see a simple test below).  You should probably include some
header that provides it.

But again, if no one noticed this in more than a decade, either no one
used this macro, or they included other headers in the same file where
they used the macro.  So I'd still rethink if maybe that macro (and
possibly others) is really needed.

Test 1:

[[
$ cat test.c
#include <linux/netfilter/xt_sctp.h>

int foo(int x)
{
	int a[x];

	return ARRAY_SIZE(a);
}
$ cc -Wall -Wextra -Werror test.c -S -o test.s
test.c: In function ‘foo’:
test.c:7:9: error: implicit declaration of function ‘ARRAY_SIZE’
[-Werror=implicit-function-declaration]
    7 |  return ARRAY_SIZE(a);
      |         ^~~~~~~~~~
cc1: all warnings being treated as errors
$
]]

As you can see, no ARRAY_SIZE().

Test2:

[[
$ cat test.c
#include <linux/netfilter/xt_sctp.h>

int foo(int x)
{
	unsigned a[x];

	return SCTP_CHUNKMAP_IS_ALL_SET(a);
}
$ cc -Wall -Wextra -Werror test.c -S -o test.s
$ <test.s grep ARRAY_SIZE
	call	ARRAY_SIZE@PLT
$
]]

In this second test, tere's no error, because it's a system header, but
you can see that ARRAY_SIZE is treated as an implicitly declared
function, and it appears in the assembly file.

Cheers,

Alex



-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: Unused macro

[Pablo Neira Ayuso]

Hi,

On Sun, Apr 04, 2021 at 10:44:12PM +0200, Alejandro Colomar (man-pages) wrote:
> Hello Florian,
> 
> On 4/4/21 10:05 PM, Florian Westphal wrote:
> > Alejandro Colomar (man-pages) <alx.manpages@gmail.com> wrote:
> >> I was updating the includes on some manual pages, when I found that a
> >> macro used ARRAY_SIZE() without including a header that defines it.
> >> That surprised me, because it would more than likely result in a compile
> >> error, but of course, the macro wasn't being used:
> >>
> >> .../linux$ grep -rn SCTP_CHUNKMAP_IS_ALL_SET
> >> include/uapi/linux/netfilter/xt_sctp.h:80:#define
> >> SCTP_CHUNKMAP_IS_ALL_SET(chunkmap) \
> >> .../linux$
> > 
> > This is an UAPI header, this macro is used by userspace software, e.g.
> > iptables.
> > 
> 
> Ahh, I see.  Thanks.
> 
> Then we still have the issue that ARRAY_SIZE is not defined in that
> header (see a simple test below).  You should probably include some
> header that provides it.

SCTP_CHUNKMAP_IS_* macros are used from iptables/extensions/xt_sctp.h
(iptables userspace codebase). These macros are also used internally
from net/netfilter/xt_sctp.c. It's using a rather unorthodox trick to
share code between the kernel and userspace, otherwise iptables would
need to keep a copy of this code.

BTW, why do you need xt_sctp.h for the manpages? This header is rather
specific to the match on sctp from the xtables infrastructure, so it's
not so useful from a programmer perspective (manpages) I think.

> But again, if no one noticed this in more than a decade, either no one
> used this macro, or they included other headers in the same file where
> they used the macro.  So I'd still rethink if maybe that macro (and
> possibly others) is really needed.
> 
> Test 1:
> 
> [[
> $ cat test.c
> #include <linux/netfilter/xt_sctp.h>
> 
> int foo(int x)
> {
> 	int a[x];
> 
> 	return ARRAY_SIZE(a);
> }
> $ cc -Wall -Wextra -Werror test.c -S -o test.s
> test.c: In function ‘foo’:
> test.c:7:9: error: implicit declaration of function ‘ARRAY_SIZE’
> [-Werror=implicit-function-declaration]
>     7 |  return ARRAY_SIZE(a);
>       |         ^~~~~~~~~~
> cc1: all warnings being treated as errors
> $
> ]]

I see, this is breaking self-compilation of the headers.

If there a need to remove xt_sctp.h from the ignore-list of the header
self-compilation infrastructure, it should be possible to fix
userspace to keep its own copy and probably add a #warn on the UAPI
header to let other possible consumers of this macro that this macro
will go away at some point.

Re: Unused macro

[Alejandro Colomar (man-pages)]

Hi Pablo,

On 4/5/21 11:44 PM, Pablo Neira Ayuso wrote:
>>
>> Then we still have the issue that ARRAY_SIZE is not defined in that
>> header (see a simple test below).  You should probably include some
>> header that provides it.
> 
> SCTP_CHUNKMAP_IS_* macros are used from iptables/extensions/xt_sctp.h
> (iptables userspace codebase). These macros are also used internally
> from net/netfilter/xt_sctp.c. It's using a rather unorthodox trick to
> share code between the kernel and userspace, otherwise iptables would
> need to keep a copy of this code.
> 
> BTW, why do you need xt_sctp.h for the manpages? This header is rather
> specific to the match on sctp from the xtables infrastructure, so it's
> not so useful from a programmer perspective (manpages) I think.

I don't need it.  Actually, I was fixing the includes in the SYNOPSIS, 
and when I was grepping for some variable (I don't even remember which 
one), the search brought me at some point to this header, where the use 
of ARRAY_SIZE() caught my attention :-)

> 
>> But again, if no one noticed this in more than a decade, either no one
>> used this macro, or they included other headers in the same file where
>> they used the macro.  So I'd still rethink if maybe that macro (and
>> possibly others) is really needed.
>>
>> Test 1:
>>
>> [[
>> $ cat test.c
>> #include <linux/netfilter/xt_sctp.h>
>>
>> int foo(int x)
>> {
>> 	int a[x];
>>
>> 	return ARRAY_SIZE(a);
>> }
>> $ cc -Wall -Wextra -Werror test.c -S -o test.s
>> test.c: In function ‘foo’:
>> test.c:7:9: error: implicit declaration of function ‘ARRAY_SIZE’
>> [-Werror=implicit-function-declaration]
>>      7 |  return ARRAY_SIZE(a);
>>        |         ^~~~~~~~~~
>> cc1: all warnings being treated as errors
>> $
>> ]]
> 
> I see, this is breaking self-compilation of the headers.
> 
> If there a need to remove xt_sctp.h from the ignore-list of the header
> self-compilation infrastructure, it should be possible to fix
> userspace to keep its own copy and probably add a #warn on the UAPI
> header to let other possible consumers of this macro that this macro
> will go away at some point.
> 

Well, I ignore what is this header for; I was just reporting something 
that I casually found and might be a bug.  I'll leave the fix to you ;)

Cheers,

Alex

-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/