10:43 a.m.
Hello,
I'm facing problems with new behavior in GCC 6.
Let's assume we have trivial program like this:
$ cat assert.c
#include <assert.h>
#include <stddef.h>
__attribute__((nonnull))
int f(char *txt) {
assert(txt != NULL);
return 0;
}
int main(int argc, char **argv) {
return 0;
}
And because upstream is paranoid, it is being compiled with:
$ gcc -Werror -Wall
This code does not compile anymore on Fedora 24:
$ gcc -Werror -Wall assert.c
In file included from assert.c:1:0:
assert.c: In function ‘f’:
assert.c:6:13: error: nonnull argument ‘txt’ compared to NULL [-Werror=nonnull]
assert(txt != NULL);
^
Did anyone met similar problem? What did you do with it?
__attribute__((nonnull)) is tremendously useful for static code analysis and
helped to uncover a lot of (not-yet triggered) issues in the code, so just
removing the attribute would not make me happy.
On the other hand, assert(var != NULL) checks are tremendously useful at
run-time. They detects problems early/near the place when the problem occurred
and makes debugging easier.
gcc -Wno-nonnull -fnodelete-null-pointer-checks
does not sound like the right solution ...
I'm all ears to hear what is the best solution/workaround for this.
--
Petr Spacek @ Red Hat
10:53 a.m.
On Wed, Feb 17, 2016 at 05:43:51PM +0100, Petr Spacek wrote:
__attribute__((nonnull)) is tremendously useful for static code
analysis and
helped to uncover a lot of (not-yet triggered) issues in the code, so just
removing the attribute would not make me happy.
Sure.
On the other hand, assert(var != NULL) checks are tremendously useful
at
run-time. They detects problems early/near the place when the problem occurred
and makes debugging easier.
That is wrong. Even older gcc versions would just optimize away the
assertion. For debugging code that uses nonnull attribute, to make sure you
aren't violating the assumptions, you can use
-fsanitize=undefined (in particular -fsanitize=nonnull,returns-nonnull).
So, just remove the bogus assertions.
BTW, in the next gcc build the warning option for this will be
-Wnonnull-compare (enabled in -Wall).
Jakub
11:14 a.m.
El mié, 17-02-2016 a las 17:53 +0100, Jakub Jelinek escribió:
That is wrong. Even older gcc versions would just optimize away the
assertion.
Even in -O0 builds?
We've been hesitantly removing ASSERT(this) statements from WebKit to
avoid compiler warnings from clang. Our asserts are enabled only in
debug builds, not in optimized release builds.
11:24 a.m.
On Wed, Feb 17, 2016 at 11:14:29AM -0600, Michael Catanzaro wrote:
El mié, 17-02-2016 a las 17:53 +0100, Jakub Jelinek escribió:
> That is wrong. Even older gcc versions would just optimize away the
> assertion.
Even in -O0 builds?
Probably not, though the warning will be at -O0 too.
We've been hesitantly removing ASSERT(this) statements from
WebKit to
ASSERT(this) is pointless, it is testing if undefined behavior didn't happen
after the fact, that is just too late. As I said, use -fsanitize=undefined
to make sure you don't call methods on nullptr.
Jakub
11:25 a.m.
On 02/17/2016 05:43 PM, Petr Spacek wrote:
Hello,
And because upstream is paranoid, it is being compiled with:
$ gcc -Werror -Wall
Did anyone met similar problem? What did you do with it?
Remove
-Werror.
__attribute__((nonnull)) is tremendously useful for static code
analysis and
helped to uncover a lot of (not-yet triggered) issues in the code, so just
removing the attribute would not make me happy.
-Werror is useful to devs when
actively working on code, but using it in
released production code to be used in packages is plain st***.
Ralf
11:30 a.m.
On Wed, Feb 17, 2016 at 05:43:51PM +0100, Petr Spacek wrote:
Hello,
I'm facing problems with new behavior in GCC 6.
Let's assume we have trivial program like this:
$ cat assert.c
#include <assert.h>
#include <stddef.h>
__attribute__((nonnull))
int f(char *txt) {
assert(txt != NULL);
return 0;
}
int main(int argc, char **argv) {
return 0;
}
And because upstream is paranoid, it is being compiled with:
$ gcc -Werror -Wall
This code does not compile anymore on Fedora 24:
$ gcc -Werror -Wall assert.c
In file included from assert.c:1:0:
assert.c: In function ‘f’:
assert.c:6:13: error: nonnull argument ‘txt’ compared to NULL [-Werror=nonnull]
assert(txt != NULL);
^
Did anyone met similar problem? What did you do with it?
__attribute__((nonnull)) is tremendously useful for static code analysis and
helped to uncover a lot of (not-yet triggered) issues in the code, so just
removing the attribute would not make me happy.
On the other hand, assert(var != NULL) checks are tremendously useful at
run-time. They detects problems early/near the place when the problem occurred
and makes debugging easier.
Instead of using __attribute__((nonnull)) directly in the code, define a
macro for it. When compiling normal builds with gcc, make the macro
expand to nothing, but when compiling with coverity or other static analysis
tools make it expand normally.
This is what we do in libvirt for a while, because we long ago hit the
problem that gcc kills your asserts() if it sees the nonnull annotation :-(
[quote $libvirt/src/internal.h]
/* gcc's handling of attribute nonnull is less than stellar - it does
* NOT improve diagnostics, and merely allows gcc to optimize away
* null code checks even when the caller manages to pass null in spite
* of the attribute, leading to weird crashes. Coverity, on the other
* hand, knows how to do better static analysis based on knowing
* whether a parameter is nonnull. Make this attribute conditional
* based on whether we are compiling for real or for analysis, while
* still requiring correct gcc syntax when it is turned off. See also
* http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 */
# ifndef ATTRIBUTE_NONNULL
# if __GNUC_PREREQ (3, 3)
# if STATIC_ANALYSIS
# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m)))
# else
# define ATTRIBUTE_NONNULL(m) __attribute__(())
# endif
# else
# define ATTRIBUTE_NONNULL(m)
# endif
# endif
[/quote]
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:43 a.m.
On Wed, Feb 17, 2016 at 05:30:27PM +0000, Daniel P. Berrange wrote:
Instead of using __attribute__((nonnull)) directly in the code,
define a
macro for it. When compiling normal builds with gcc, make the macro
expand to nothing, but when compiling with coverity or other static analysis
tools make it expand normally.
Well, nonnull attribute is not primarily a debugging aid, but an
optimization hint, which allows the compiler to optimize, sometimes
significantly, the code that gives the compiler the additional information
it can't figure itself otherwise.
So while it could make sense to disable it in debug builds if you want
your assertions to work, for production builds built without assertions
it is really unnecessary pessimization of the code if you don't define it.
* still requiring correct gcc syntax when it is turned off. See
also
* http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 */
Funny you mention this PR, because the new warning is one of the
warnings requested in that PR. And now some people are complaining that the
warning got implemented.
Jakub
11:51 a.m.
On Wed, 17 Feb 2016 18:25:29 +0100, Ralf Corsepius wrote:
Remove -Werror.
[...]
-Werror is useful to devs when actively working on code, but using it
in
released production code to be used in packages is plain st***.
-Werror has found me many times bugs in Fedora add-on patches not being up to
date with the upstream changes.
Jan
12:20 p.m.
On 02/17/2016 06:51 PM, Jan Kratochvil wrote:
On Wed, 17 Feb 2016 18:25:29 +0100, Ralf Corsepius wrote:
> Remove -Werror.
[...]
> -Werror is useful to devs when actively working on code, but using it in
> released production code to be used in packages is plain st***.
-Werror has found me many times bugs in Fedora add-on patches not being
You are
developing with upstream and are using Fedora a test bed.
This is unlike the situation many packages are in.
For these, all -Werror does is to raise in most case negligible warnings
to errors and to causes FTBFS for negligible reasons, such indentation,
unused vars or other stylishness.
This shows especially in situations like now, when gcc received a major
update starts to raise _warnings_ for a new set of issues, and -Werror
enabled are going to FTBFS in masses.
In other words: WARNINGS are warnings and not errors for a reason.
Ralf
8:12 p.m.
Jakub Jelinek wrote:
ASSERT(this) is pointless, it is testing if undefined behavior
didn't
happen after the fact, that is just too late. As I said, use
-fsanitize=undefined to make sure you don't call methods on nullptr.
Doesn't -fno-delete-null-pointer-checks make such ASSERTs work (and also
explicit "if (this)" type checks)? I read that that flag fixes programs
which rely on "if (this)" checks.
Kevin Kofler
1:50 a.m.
On Fri, Feb 19, 2016 at 03:12:29AM +0100, Kevin Kofler wrote:
Jakub Jelinek wrote:
> ASSERT(this) is pointless, it is testing if undefined behavior didn't
> happen after the fact, that is just too late. As I said, use
> -fsanitize=undefined to make sure you don't call methods on nullptr.
Doesn't -fno-delete-null-pointer-checks make such ASSERTs work (and also
explicit "if (this)" type checks)? I read that that flag fixes programs
which rely on "if (this)" checks.
That switch allows to work around buggy programs, at the cost of optimizing
them less, yes. In any case, such programs should be fixed, this must be
always non-NULL, methods can't be called on NULL pointers.
Jakub
3:49 a.m.
On 17.2.2016 18:43, Jakub Jelinek wrote:
On Wed, Feb 17, 2016 at 05:30:27PM +0000, Daniel P. Berrange wrote:
> Instead of using __attribute__((nonnull)) directly in the code, define a
> macro for it. When compiling normal builds with gcc, make the macro
> expand to nothing, but when compiling with coverity or other static analysis
> tools make it expand normally.
Well, nonnull attribute is not primarily a debugging aid, but an
optimization hint, which allows the compiler to optimize, sometimes
significantly, the code that gives the compiler the additional information
it can't figure itself otherwise.
So while it could make sense to disable it in debug builds if you want
your assertions to work, for production builds built without assertions
it is really unnecessary pessimization of the code if you don't define it.
> * still requiring correct gcc syntax when it is turned off. See also
> * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 */
Funny you mention this PR, because the new warning is one of the
warnings requested in that PR. And now some people are complaining that the
warning got implemented.
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.
In past 15 years, this allowed BIND 9 to avoid exploitable code execution bugs
[1] and turned most of them into mere DoS.
You can compare this with BIND 4 and BIND 8 which were famous for this kind of
the bugs (and didn't have asserts() all over the place). BTW libresolv in
today's glibc was taken from BIND 8 :-)
Quick grep shows 10497 asserts in BIND itself, so it is not realistic to say
'go and rewrite it from scratch', sorry. Apparently BIND is using
-fno-delete-null-pointer-checks from GCC 4.9 times.
Anyway, thank you for your input, I will find my way how to hack around it.
[1] http://www.cvedetails.com/product/144/ISC-Bind.html?vendor_id=64
--
Petr Spacek @ Red Hat
5:16 a.m.
On 17.2.2016 19:20, Ralf Corsepius wrote:
On 02/17/2016 06:51 PM, Jan Kratochvil wrote:
> On Wed, 17 Feb 2016 18:25:29 +0100, Ralf Corsepius wrote:
>> Remove -Werror.
> [...]
>> -Werror is useful to devs when actively working on code, but using it in
>> released production code to be used in packages is plain st***.
>
> -Werror has found me many times bugs in Fedora add-on patches not being
You are developing with upstream and are using Fedora a test bed.
This is unlike the situation many packages are in.
For these, all -Werror does is to raise in most case negligible warnings to
errors and to causes FTBFS for negligible reasons, such indentation, unused
vars or other stylishness.
This shows especially in situations like now, when gcc received a major update
starts to raise _warnings_ for a new set of issues, and -Werror enabled are
going to FTBFS in masses.
In other words: WARNINGS are warnings and not errors for a reason.
In this case the warning which was turned into error saved us asses because
the code would behave differently under too aggressive optimizations.
It is not black-white question.
--
Petr Spacek @ Red Hat
6:04 a.m.
On 19.2.2016 08:50, Jakub Jelinek wrote:
On Fri, Feb 19, 2016 at 03:12:29AM +0100, Kevin Kofler wrote:
> Jakub Jelinek wrote:
>> ASSERT(this) is pointless, it is testing if undefined behavior didn't
>> happen after the fact, that is just too late. As I said, use
>> -fsanitize=undefined to make sure you don't call methods on nullptr.
>
> Doesn't -fno-delete-null-pointer-checks make such ASSERTs work (and also
> explicit "if (this)" type checks)? I read that that flag fixes programs
> which rely on "if (this)" checks.
That switch allows to work around buggy programs, at the cost of optimizing
them less, yes. In any case, such programs should be fixed, this must be
always non-NULL, methods can't be called on NULL pointers.
Could you elaborate on this, please?
What is wrong with
if (ptr != NULL)
?
What standard says that it is wrong?
--
Petr Spacek @ Red Hat
6:07 a.m.
On 02/19/2016 12:16 PM, Petr Spacek wrote:
It is not black-white question.
Absolutely.
But I say, there should not be any room for -Werror in production
SW/packages.
The fact,
* different version of gcc raise different warnings
* gcc on different architectures raise different warnings.
* gcc raises warnings on coding styles.
makes -Werror non-suiteable for long-term maintainance.
Worse, this causing non-experienced packagers to take action on
non-issues and potentially introduce bugs.
Most absurd piece of a spec I saw in Fedora spec recently, was this:
...
export CFLAGS="$RPM_OPT_FLAGS -Wall -Werror -Wno-unused-function
-Wno-unused-variable -Wno-unused-but-set-variable -Wno-array-bounds
-fno-strict-aliasing -Wno-logical-not-parentheses
-Wno-error=misleading-indentation"
%configure
...
Ralf
6:08 a.m.
On 19/02/16 12:04, Petr Spacek wrote:
Could you elaborate on this, please?
What is wrong with
if (ptr != NULL)
?
What standard says that it is wrong?
That isn't what's wrong. What is wrong is the method call that got you
there. You have a method like this:
void foo()
{
if (this != NULL) ...
}
and then you invoke it with a null pointer:
xxx->foo();
and it is that call that is not allowed by the standard when xxx is null.
So the compiler is allowed to assume that this will never be null in a
method, because calling a method on a null pointer is not allowed, so it
can just remove your null test in the method.
Then when you do call it on a null pointer you crash because the method
is not doing the null test and just tries to access members of the object.
Tom
--
Tom Hughes (tom(a)compton.nu)
http://compton.nu/
6:08 a.m.
On Fri, Feb 19, 2016 at 01:04:04PM +0100, Petr Spacek wrote:
On 19.2.2016 08:50, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 03:12:29AM +0100, Kevin Kofler wrote:
>> Jakub Jelinek wrote:
>>> ASSERT(this) is pointless, it is testing if undefined behavior didn't
>>> happen after the fact, that is just too late. As I said, use
>>> -fsanitize=undefined to make sure you don't call methods on nullptr.
>>
>> Doesn't -fno-delete-null-pointer-checks make such ASSERTs work (and also
>> explicit "if (this)" type checks)? I read that that flag fixes
programs
>> which rely on "if (this)" checks.
>
> That switch allows to work around buggy programs, at the cost of optimizing
> them less, yes. In any case, such programs should be fixed, this must be
> always non-NULL, methods can't be called on NULL pointers.
Could you elaborate on this, please?
What is wrong with
if (ptr != NULL)
?
What standard says that it is wrong?
It's about checking whether "this", in C++, is NULL. Since any call on a
null
pointer is undefined behavior, any code relying on such checks is non-standard.
Marek
6:24 a.m.
On 19/02/16 08:50 +0100, Jakub Jelinek wrote:
On Fri, Feb 19, 2016 at 03:12:29AM +0100, Kevin Kofler wrote:
> Jakub Jelinek wrote:
> > ASSERT(this) is pointless, it is testing if undefined behavior didn't
> > happen after the fact, that is just too late. As I said, use
> > -fsanitize=undefined to make sure you don't call methods on nullptr.
>
> Doesn't -fno-delete-null-pointer-checks make such ASSERTs work (and also
> explicit "if (this)" type checks)? I read that that flag fixes programs
> which rely on "if (this)" checks.
That switch allows to work around buggy programs, at the cost of optimizing
them less, yes. In any case, such programs should be fixed, this must be
always non-NULL, methods can't be called on NULL pointers.
Right, using -fno-delete-null-pointer-checks doesn't "fix" the
programs, they still have undefined behaviour.
Checking "if (this)" is completely brain-damaged. If you want to ask
questions like "do I exist?" then practise philosophy not C++.
The other common problem uncovered by the new optimization is in code
like:
foo()->bar();
where foo() happens to return a null pointer. If bar() doesn't access
any members of *this then previously this compiled and ran without
error. This code isn't brain-damaged, it's just got a bug, and should
have checked for null if that is a possible result of foo() e.g.
if (auto p = foo())
p->bar();
else
/* do something appropriate */;
Using -fno-delete-null-pointer-checks will make the code appear to
work correctly again, but obviously it's still wrong if foo() can
return a null pointer. And if bar() was ever changed to access a
member of *this then it would start crashing, even with
-fno-delete-null-pointer-checks.
6:30 a.m.
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.
6:44 a.m.
On 19.2.2016 13:08, Marek Polacek wrote:
On Fri, Feb 19, 2016 at 01:04:04PM +0100, Petr Spacek wrote:
> On 19.2.2016 08:50, Jakub Jelinek wrote:
>> On Fri, Feb 19, 2016 at 03:12:29AM +0100, Kevin Kofler wrote:
>>> Jakub Jelinek wrote:
>>>> ASSERT(this) is pointless, it is testing if undefined behavior
didn't
>>>> happen after the fact, that is just too late. As I said, use
>>>> -fsanitize=undefined to make sure you don't call methods on nullptr.
>>>
>>> Doesn't -fno-delete-null-pointer-checks make such ASSERTs work (and also
>>> explicit "if (this)" type checks)? I read that that flag fixes
programs
>>> which rely on "if (this)" checks.
>>
>> That switch allows to work around buggy programs, at the cost of optimizing
>> them less, yes. In any case, such programs should be fixed, this must be
>> always non-NULL, methods can't be called on NULL pointers.
>
> Could you elaborate on this, please?
>
> What is wrong with
> if (ptr != NULL)
> ?
>
> What standard says that it is wrong?
It's about checking whether "this", in C++, is NULL. Since any call on a
null
pointer is undefined behavior, any code relying on such checks is non-standard.
Ah, okay. I was talking about pure C all the time, so I got confused :-)
--
Petr Spacek @ Red Hat
6:48 a.m.
On Fri, Feb 19, 2016 at 01:44:45PM +0100, Petr Spacek wrote:
> It's about checking whether "this", in C++, is
NULL. Since any call on a null
> pointer is undefined behavior, any code relying on such checks is non-standard.
Ah, okay. I was talking about pure C all the time, so I got confused :-)
It is not really different for C, just that "this" is in C++ implicitly
nonnull. Otherwise, if you use a nonnull attribute, you make the same
promise, this function will never be called with NULL as some particular
pointer argument (or all pointer arguments). So, the undefined behavior
happens if you violate that and call a function with an invalid argument.
Jakub
6:51 a.m.
On 19.2.2016 13:30, Jonathan Wakely wrote:
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.
This is nice in a theory but not in practice. You can give promises about
state of your program you wish, but these promises can 100 % hold only for
bug-free programs using bug-free libraries.
Everything else is going to face real world where various bugs cause that
various 'promises' will not hold.
As I said above in the original question, the (nonnull) attribute is used to
help static analyzers, not as optimization hint.
#if defined (__GNUC__) and -fno-delete-null-pointer-checks
will workaround it for me.
Thank you for your input, guys.
--
Petr Spacek @ Red Hat
7:03 a.m.
On Fri, 19 Feb 2016 13:07:53 +0100, Ralf Corsepius wrote:
But I say, there should not be any room for -Werror in production
SW/packages.
The fact,
* different version of gcc raise different warnings
* gcc on different architectures raise different warnings.
* gcc raises warnings on coding styles.
makes -Werror non-suiteable for long-term maintainance.
You miss here false warnings (errors) are much less costly than a missed
warning (errors). If upstream package is free of warnings thanks to -Werror
then a Fedora packaging should not introduce so many warnings they would not
be manageable by its Fedora maintainer.
Jan
7:28 a.m.
On 02/19/2016 01:07 PM, Ralf Corsepius wrote:
On 02/19/2016 12:16 PM, Petr Spacek wrote:
> It is not black-white question.
Absolutely.
But I say, there should not be any room for -Werror in production
SW/packages.
I agree that blanket -Werror is questionable, especially with -Wall,
which includes warnings dependent on optimization.
But using -Werror selectively makes a lot of sense. For example, for C
code, you could use -Werror=implicit-function-declaration
-Werror=implicit-int.
Florian
2987
days inactive
2989
days old
24 comments
12 participants
participants (12)
-
Daniel P. Berrange -
Florian Weimer -
Jakub Jelinek -
Jan Kratochvil -
Jonathan Wakely -
Kevin Kofler -
Marek Polacek -
Michael Catanzaro -
Petr Spacek -
Ralf Corsepius -
Richard Hughes -
Tom Hughes