Add __attribute__nonnull__() for non-DEBUGGING buils #23641
Conversation
khwilliamson
force-pushed
the
attribute_nonnull
branch
from
f27e796 to
bb147b4
Compare
tried Looks like a bug with POSIX Perl's Imagine is the wrong word b/c I've done this b4 for private biz XS. So lets imagine, I am a CPAN XS module running on ithread-ed WinPerl, with only 1 Perl thread ( So I am an XS->PP event handler executing inside a TP Thd, The root thread is frozen (blocked until I release control back to the root thread manually). At the end of my TP thread runner, after the I'm avoiding an accident if some enumeration API gets smart (not really) and runs my C callback fn ptr asynchronously, in parallel, on multiple cores on multiple TP OS threads. Bad hygiene to leave your de allocated void ptrs in TLS. So I would definitely want Or this is an optimization to const fold away all machine code associated with PERL_SET_CONTEXT() on single-threaded perls builds. But then the question is, why was You also wrote, or were the last person to clean it up. but the null test existed before the commit above, ill stop git blaming at this point. |
regen/embed.pl
Outdated
| } | ||
| else { | ||
| push @asserts, "PERL_ASSUME_NON_NULL($argname)"; | ||
| push @attrs, "__attribute__nonnull__($n)"; |
There was a problem hiding this comment.
I think this can make the compiler optimise away the assert()s we generate, and from looking at the generated code that appears to be the case.
I had a look, with ./Configure -des -Dusedevel -DDEBUGGING && make mg.s Perl_mg_magical (first name I saw it in the proto.h diff).
blead:
Perl_mg_magical:
subq $8, %rsp
testq %rdi, %rdi ; check sv
je .L136 ; jump to assert code if it is NULL
...
.L136:
leaq __PRETTY_FUNCTION__.110(%rip), %rcx
movl $136, %edx
leaq .LC0(%rip), %rsi
leaq .LC1(%rip), %rdi
call __assert_fail@PLT
This PR:
Perl_mg_magical:
movl 12(%rdi), %eax ; note directly fetches from the sv flags without the check
movl %eax, %edx
andl $-14680065, %edx
movl %edx, 12(%rdi)
I think we'd need to only generate the __attribute__nonnull__ for non-DEBUGGING builds.
I'll admit to being a little uncomfortable with automatically generated ASSUME()s, since they produce runtime UB if the assumption is false, but I think it's reasonable here, assuming we fix the assert() problem.
There was a problem hiding this comment.
I think we'd need to only generate the attribute__nonnull for non-DEBUGGING builds.
I don't follow this. What's the downside of generating it for all builds?
There was a problem hiding this comment.
What's the downside of generating it for all builds?
It turns all the asserts into no-ops.
void foo(int *) __attribute__((__nonnull__(1)));
void foo(int *ptr) {
assert(ptr);
...
}Since ptr is marked "nonnull", the compiler "knows" that ptr cannot be null in the function body, so it turns assert(ptr) into a no-op.
Or as Tony wrote:
this can make the compiler optimise away the assert()s we generate
There was a problem hiding this comment.
Yeah, that was exactly my concern. This attribute having both internal and an external effect is most unfortunate. We only want one of them here.
There was a problem hiding this comment.
I still don't see the problem. Part of the motivation here is to optimize away those asserts. Since these are known at compile time, shouldn't compilations fail if called wrongly, so the asserts aren't needed?
There was a problem hiding this comment.
I still don't see the problem. Part of the motivation here is to optimize away those asserts. Since these are known at compile time, shouldn't compilations fail if called wrongly, so the asserts aren't needed?
As mauke points out the compiler won't always warn about this by default, but it also won't perform across compilation-unit (CU) checks, if f(NULLOK SV *sv) in a.c calls g(NN SV *sv) in b.c with that SV without checking for NULL the compiler won't have the information needed to complain.
Coverity can detect this type of issue across CUs, but I think it's only based on usage (it sees g() dereferences sv) rather than on these non-standard function attributes.
One big limitation with __attribute__nonnull__ is while we're marking parameters as not-null this doesn't indicate nullable for unmarked parameters - the compiler can't assume such parameters are nullable since not all code (think libraries) uses that attribute. clang's nullability attributes (_Nullable, _Nonnull, _Null_unspecified) let you provide that information, which may allow[3] the compiler to detect my f()/g() example, but we're not doing that.
None of these tools are perfect, we've seen Coverity go down strange logic chains (eg. assuming two contradictory conditions /cry), and IIRC codechecker[1] had some strange results too. (clang-tidy, cppcheck[2] don't support cross-CU checks), so I don't think we can remove (or optimize away) the asserts.
Also not all callers are perl itself, those asserts() aren't just for perl, they're also for XS and embedders.
For your PERL_SET_NON_tTHX_CONTEXT(i) warning, I suspect it would go away if you made that into an inline function with the parameter NULLOK, and the optimizer could still optimize away the check if the caller's value was nonnull.
[1] WebUI/cross-CU tool for the clang static analyzer
[2] one talk I saw on cppcheck suggested combining all of the CUs into one (all.c #includes every other .c) to perform cross-CU checks
[3] I don't know if it actually does, google felt the need to add the clang-SA checks mentioned in the C++now talk I linked to
There was a problem hiding this comment.
I generally agree with you, but the nonnull information is available across translation units because it's in the prototype. GCC can and does make (limited) use of it. Especially with -fanalyzer, but even -Wnonnull will catch some cases, especially with optimisations enabled. Optimisations are important here, the compiler does more analysis when they're enabled.
For example, this warns with -fanalyzer:
#include <stdlib.h>
__attribute__((nonnull(1)))
int foo(char *foo);
int getint();
char *getstring(int arg) {
if (arg)
return "abc";
else
return NULL;
}
int main () {
foo(getstring(getint()));
return 0;
}This simpler case warns with just -O2 -Wall:
#include <stdlib.h>
__attribute__((nonnull(1)))
int foo(char *foo);
int getint() {
return 0;
}
char *getstring(int arg) {
if (arg)
return "abc";
else
return NULL;
}
int main () {
foo(getstring(getint()));
return 0;
}There was a problem hiding this comment.
I did over-react, sorry.
I do think we don't want assert()s optimised away.
There was a problem hiding this comment.
What about optimizing them away for functions that aren't visible outside the perl core? If that is ok, what about those that are visible only in perl extensions?
There was a problem hiding this comment.
Why optimise them away at all? debugging builds (and I often use -O0 for debugging builds) aren't that painfully slow.
Why take the risk?
For xenu's example, if you move the definition of getstring() and foo() to another CU, you'll only get a warning from -fanalyzer -flto builds, the detection isn't perfect:
tony@venus:.../perl/git$ cat 23641c.c
#include <stdlib.h>
__attribute__((nonnull(1)))
int foo(char *foo);
char *getstring(int arg);
int getint() {
return 0;
}
int main () {
foo(getstring(getint()));
return 0;
}
tony@venus:.../perl/git$ cat 23641d.c
#include <stdlib.h>
#include <stdio.h>
__attribute__((nonnull(1)))
int foo(char *foo);
char *getstring(int arg) {
if (arg)
return "abc";
else
return NULL;
}
int foo(char *s) {
puts(s);
return 1;
}
tony@venus:.../perl/git$ gcc -Wall -O2 23641c.c 23641d.c
tony@venus:.../perl/git$ gcc -flto -Wall -O2 23641c.c 23641d.c
tony@venus:.../perl/git$ gcc -fanalyzer -Wall -O2 23641c.c 23641d.c
tony@venus:.../perl/git$ gcc -flto -fanalyzer -Wall -O2 23641c.c 23641d.c
23641c.c: In function ‘main’:
23641c.c:13:5: warning: use of NULL where non-null expected [CWE-476] [-Wanalyzer-null-argument]
13 | foo(getstring(getint()));
| ^
‘main’: events 1-2
|
...
I tried a build with -Dcc='gcc -fanalyzer -flto', I killed the link step for miniperl after several minutes because it was using over 55GB of resident space (around which point the machine started to swap.)
|
Sort of related https://www.youtube.com/watch?v=3zQ4zw4GNV0 which talks about static analysis of the clang nullability attributes. |
This got duplicated in recent rebasing
The next commit will want this to be available earlier.
Though code later strips this off, it's best to not put it in in the first place
proto.h contains a generated PERL_ARGS_ASSERT macro for every function. It asserts that each parameter that isn't allowed to be NULL actually isn't. These asserts are disabled when not DEBUGGING. But many compilers allow a compile-time assertion to be made for this situation, so we can add an extra measure of protection for free. And this gives hints to the compiler for optimizations when the asserts() aren't there. Because of complications, this commit only does this for functions that don't have a thread context.
khwilliamson
force-pushed
the
attribute_nonnull
branch
from
bb147b4 to
738ad2e
Compare
|
I changed to use the attribute_nonnull only for non-DEBUGGING builds. I also removed the change to use ASSUME. On DEBUGGING builds, the asserts() give clues to the compiler; and on non-DEBUGGING ones, the attribute_nonnull lines give the same clues. There are other assertions besides the non-NULL ones that go away in non-DEBUGGING, but they are insignificant in comparison with the NULL issues. |
khwilliamson
changed the title
khwilliamson
changed the title
Are you aware of the |
proto.h contains a generated PERL_ARGS_ASSERT macro for every function. It asserts that each parameter that isn't allowed to be NULL actually isn't.
These asserts are disabled when not DEBUGGING. But many compilers allow a compile-time assertion to be made for this situation, so we can add an extra measure of protection for free. And this gives hints to the compiler for optimizations when the asserts() aren't there.