7:56 a.m.
Apparently our usage of __nonnull__ attribute is not completely aligned
with what gcc developers mean with it. Use of the attribute declares
that the parameter can never be NULL which means that the checks in the
IF_NO_PAMH for example can be removed by the compiler during the
optimization. We would need to remove the __nonnull__ attribute or at
least add -fno-delete-null-pointer-checks to the default CFLAGS.
I'd prefer removing the __nonnull__.
What do you think?
Tomas
-------- Forwarded Message --------
From: Jonathan Wakely <jwakely(a)fedoraproject.org>
Reply-to: Development discussions related to Fedora
<devel(a)lists.fedoraproject.org>
To: Development discussions related to Fedora <devel(a)lists.fedoraprojec
t.org>
Subject: Re: GCC 6 -Wnonnull is too aggressive
Date: Fri, 19 Feb 2016 12:30:58 +0000
On 19/02/16 10:49 +0100, Petr Spacek wrote:
The thing is that some developers (e.g. me and ISC :-)) do not think
that
assert() should be used only in debug builds.
E.g. BIND itself is written in "Design by contract" spirit and
asserts are
used all over the place to make sure that code which does not behave
as
intended is killed as soon as possible (and thus prevented from doing
collateral damage). Call it defensive programming if you wish.
That's fine. Don't use attribute((nonnull)) then.
It's illogical to promise the compiler that something will never,
ever, ever be null, but also check whether it's null. If you're not
100% certain it can't be null, then you lied to the compiler. If you
are 100% certain it can't be null then the assert is redundant and can
be removed.
If you're only 99.999% certain, and so the assert serves a useful
purpose, then don't make promises that you can't keep.
If you just want to advise the compiler that something *probably*
won't be null, and tell it to optimize based on that assumption, then
you can use __builtin_expect() to provide that hint (although I
wouldn't actually recommend doing so).
Don't make a promise when you mean to give a hint.
--
devel mailing list
devel(a)lists.fedoraproject.org
http://lists.fedoraproject.org/admin/lists/devel@lists.fedoraproject.or
g
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
(You'll never know whether the road is wrong though.)
8:19 a.m.
Hi,
On Fri, Feb 19, Tomas Mraz wrote:
Apparently our usage of __nonnull__ attribute is not completely
aligned
with what gcc developers mean with it. Use of the attribute declares
that the parameter can never be NULL which means that the checks in the
IF_NO_PAMH for example can be removed by the compiler during the
optimization. We would need to remove the __nonnull__ attribute or at
least add -fno-delete-null-pointer-checks to the default CFLAGS.
I'd prefer removing the __nonnull__.
What do you think?
From Linux-PAM code:
#if PAM_GNUC_PREREQ(3,3) && !defined(LIBPAM_COMPILE)
# define PAM_NONNULL(params) __attribute__((__nonnull__ params))
#else
# define PAM_NONNULL(params)
#endif
We don't use it for libpam itself, I fixed that already 10 years ago.
The problem is well known, from the GCC documentation:
"causes the compiler to check that, in calls to my_memcpy, arguments
dest and src are non-null. If the compiler determines that a null
pointer is passed in an argument slot marked as non-null, and the
-Wnonnull option is enabled, a warning is issued. The compiler may
also choose to make optimizations based on the knowledge that certain
function arguments will not be null. "
So our usage of the nonnull attribute is matching exactly what
was documented for GCC: the calling code should get a warning
from the compiler, if it is using NULL as argument, and the
called functions in libpam are not compiled with this attribute,
so that the checks are not removed.
I think we only need to check, if we haven't make changes to the
code, so that the nonnull attribute is also used for to be called
code.
Thorsten
Tomas
-------- Forwarded Message --------
From: Jonathan Wakely <jwakely(a)fedoraproject.org>
Reply-to: Development discussions related to Fedora
<devel(a)lists.fedoraproject.org>
To: Development discussions related to Fedora <devel(a)lists.fedoraprojec
t.org>
Subject: Re: GCC 6 -Wnonnull is too aggressive
Date: Fri, 19 Feb 2016 12:30:58 +0000
On 19/02/16 10:49 +0100, Petr Spacek wrote:
> The thing is that some developers (e.g. me and ISC :-)) do not think
> that
> assert() should be used only in debug builds.
>
> E.g. BIND itself is written in "Design by contract" spirit and
> asserts are
> used all over the place to make sure that code which does not behave
> as
> intended is killed as soon as possible (and thus prevented from doing
> collateral damage). Call it defensive programming if you wish.
That's fine. Don't use attribute((nonnull)) then.
It's illogical to promise the compiler that something will never,
ever, ever be null, but also check whether it's null. If you're not
100% certain it can't be null, then you lied to the compiler. If you
are 100% certain it can't be null then the assert is redundant and can
be removed.
If you're only 99.999% certain, and so the assert serves a useful
purpose, then don't make promises that you can't keep.
If you just want to advise the compiler that something *probably*
won't be null, and tell it to optimize based on that assumption, then
you can use __builtin_expect() to provide that hint (although I
wouldn't actually recommend doing so).
Don't make a promise when you mean to give a hint.
--
devel mailing list
devel(a)lists.fedoraproject.org
http://lists.fedoraproject.org/admin/lists/devel@lists.fedoraproject.or
g
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
(You'll never know whether the road is wrong though.)
_______________________________________________
Pam-developers mailing list
pam-developers(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/pam-developers@lists.fedorahos...
--
Thorsten Kukuk, Senior Architect SLES & Common Code Base
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
8:25 a.m.
New subject: [Fwd: Re: GCC 6 -Wnonnull is too aggressive]
On Pá, 2016-02-19 at 15:19 +0100, Thorsten Kukuk wrote:
Hi,
On Fri, Feb 19, Tomas Mraz wrote:
> Apparently our usage of __nonnull__ attribute is not completely
> aligned
> with what gcc developers mean with it. Use of the attribute
> declares
> that the parameter can never be NULL which means that the checks in
> the
> IF_NO_PAMH for example can be removed by the compiler during the
> optimization. We would need to remove the __nonnull__ attribute or
> at
> least add -fno-delete-null-pointer-checks to the default CFLAGS.
>
> I'd prefer removing the __nonnull__.
>
> What do you think?
From Linux-PAM code:
#if PAM_GNUC_PREREQ(3,3) && !defined(LIBPAM_COMPILE)
# define PAM_NONNULL(params) __attribute__((__nonnull__ params))
#else
# define PAM_NONNULL(params)
#endif
We don't use it for libpam itself, I fixed that already 10 years
ago.
The problem is well known, from the GCC documentation:
"causes the compiler to check that, in calls to my_memcpy, arguments
dest and src are non-null. If the compiler determines that a null
pointer is passed in an argument slot marked as non-null, and the
-Wnonnull option is enabled, a warning is issued. The compiler may
also choose to make optimizations based on the knowledge that
certain
function arguments will not be null. "
So our usage of the nonnull attribute is matching exactly what
was documented for GCC: the calling code should get a warning
from the compiler, if it is using NULL as argument, and the
called functions in libpam are not compiled with this attribute,
so that the checks are not removed.
I think we only need to check, if we haven't make changes to the
code, so that the nonnull attribute is also used for to be called
code.
Ah, OK, I overlooked the !defined(LIBPAM_COMPILE) condition. So yes,
our use of the attribute should be correct.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
(You'll never know whether the road is wrong though.)
2987
days inactive
2987
days old
pam-developers@lists.fedorahosted.org
2 comments
2 participants
participants (2)
-
Thorsten Kukuk -
Tomas Mraz