* _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) [not found] ` <1efbe0b2dd8fefffc945c6734222c7d6e04cf465.camel@xry111.site> @ 2023-07-10 20:14 ` Alejandro Colomar 2023-07-10 20:16 ` Alejandro Colomar 0 siblings, 1 reply; 12+ messages in thread From: Alejandro Colomar @ 2023-07-10 20:14 UTC (permalink / raw) To: Xi Ruoyao, Andrew Pinski, GNU libc development Cc: Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc [-- Attachment #1.1: Type: text/plain, Size: 1359 bytes --] [CC += Andrew] Hi Xi, Andrew, On 7/10/23 20:41, Xi Ruoyao wrote: > Maybe we should have a weaker version of nonnull which only performs the > diagnostic, not the optimization. But it looks like they hate the idea: > https://gcc.gnu.org/PR110617. > This is the one thing that makes me use both Clang and GCC to compile, because while any of them would be enough to build, I want as much static analysis as I can get, and so I want -fanalyzer (so I need GCC), but I also use _Nullable (so I need Clang). If GCC had support for _Nullable, I would have in GCC the superset of features that I need from both in a single vendor. Moreover, Clang's static analyzer is brain-damaged (sorry, but it doesn't have a simple command line to run it, contrary to GCC's easy -fanalyzer), so having GCC's analyzer get over those _Nullable qualifiers would be great. Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer mode, as there are many cases where the compiler doesn't have enough information, and the analyzer can get rid of false negatives and positives. See: <https://github.com/llvm/llvm-project/issues/57546> I'll back the ask for the qualifiers in GCC, for compatibility with Clang. Thanks, Alex -- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) 2023-07-10 20:14 ` _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) Alejandro Colomar @ 2023-07-10 20:16 ` Alejandro Colomar 2023-08-08 10:01 ` Martin Uecker 0 siblings, 1 reply; 12+ messages in thread From: Alejandro Colomar @ 2023-07-10 20:16 UTC (permalink / raw) To: Xi Ruoyao, Andrew Pinski, GNU libc development Cc: Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc [-- Attachment #1.1: Type: text/plain, Size: 1706 bytes --] On 7/10/23 22:14, Alejandro Colomar wrote: > [CC += Andrew] > > Hi Xi, Andrew, > > On 7/10/23 20:41, Xi Ruoyao wrote: >> Maybe we should have a weaker version of nonnull which only performs the >> diagnostic, not the optimization. But it looks like they hate the idea: >> https://gcc.gnu.org/PR110617. >> > This is the one thing that makes me use both Clang and GCC to compile, > because while any of them would be enough to build, I want as much > static analysis as I can get, and so I want -fanalyzer (so I need GCC), > but I also use _Nullable (so I need Clang). > > If GCC had support for _Nullable, I would have in GCC the superset of > features that I need from both in a single vendor. Moreover, Clang's > static analyzer is brain-damaged (sorry, but it doesn't have a simple > command line to run it, contrary to GCC's easy -fanalyzer), so having > GCC's analyzer get over those _Nullable qualifiers would be great. > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer > mode, as there are many cases where the compiler doesn't have enough > information, and the analyzer can get rid of false negatives and > positives. See: <https://github.com/llvm/llvm-project/issues/57546> > > I'll back the ask for the qualifiers in GCC, for compatibility with > Clang. BTW, Bionic libc is adding those qualifiers: <https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45> <https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability> > > Thanks, > Alex > -- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) 2023-07-10 20:16 ` Alejandro Colomar @ 2023-08-08 10:01 ` Martin Uecker 2023-08-09 0:14 ` enh 0 siblings, 1 reply; 12+ messages in thread From: Martin Uecker @ 2023-08-08 10:01 UTC (permalink / raw) To: Alejandro Colomar, Xi Ruoyao, Andrew Pinski, GNU libc development Cc: Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc Am Montag, dem 10.07.2023 um 22:16 +0200 schrieb Alejandro Colomar via Gcc: > On 7/10/23 22:14, Alejandro Colomar wrote: > > [CC += Andrew] > > > > Hi Xi, Andrew, > > > > On 7/10/23 20:41, Xi Ruoyao wrote: > > > Maybe we should have a weaker version of nonnull which only performs the > > > diagnostic, not the optimization. But it looks like they hate the idea: > > > https://gcc.gnu.org/PR110617. > > > > > This is the one thing that makes me use both Clang and GCC to compile, > > because while any of them would be enough to build, I want as much > > static analysis as I can get, and so I want -fanalyzer (so I need GCC), > > but I also use _Nullable (so I need Clang). > > > > If GCC had support for _Nullable, I would have in GCC the superset of > > features that I need from both in a single vendor. Moreover, Clang's > > static analyzer is brain-damaged (sorry, but it doesn't have a simple > > command line to run it, contrary to GCC's easy -fanalyzer), so having > > GCC's analyzer get over those _Nullable qualifiers would be great. > > > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer > > mode, as there are many cases where the compiler doesn't have enough > > information, and the analyzer can get rid of false negatives and > > positives. See: <https://github.com/llvm/llvm-project/issues/57546> > > > > I'll back the ask for the qualifiers in GCC, for compatibility with > > Clang. > > BTW, Bionic libc is adding those qualifiers: > > <https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45> > <https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability> > > I am sceptical about these qualifiers because they do not fit well with this language mechanism. Qualifiers take effect at accesses to objects and are discarded at lvalue conversion. So a qualifier should ideally describe a property that is relevant for accessing objects but is not relevant for values. Also there are built-in conversion rules a qualifier should conform to. A pointer pointing to a type without qualifier can implicitely convert to a pointer to a type with qualifier, e.g. int* can be converted to const int*. Together, this implies that a qualifier should add constraints to a type that are relevant to how an object is accessed. "const" and "volatile" or "_Atomic" are good examples. ("restricted" does not quite follow these rules and is considered weird and difficult to understand.) In contrast, "nonnull" and "nullable" are properties that affect the set of values of the pointer, but not how the pointer itself is accessed. Martin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) 2023-08-08 10:01 ` Martin Uecker @ 2023-08-09 0:14 ` enh 2023-08-09 1:11 ` Siddhesh Poyarekar 2023-08-09 7:26 ` Martin Uecker 0 siblings, 2 replies; 12+ messages in thread From: enh @ 2023-08-09 0:14 UTC (permalink / raw) To: Martin Uecker Cc: Alejandro Colomar, Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc (bionic maintainer here, mostly by accident...) yeah, we tried the GCC attributes once before with _disastrous_ results (because GCC saw it as an excuse to delete explicit null checks, it broke all kinds of things). the clang attributes are "better" in that they don't confuse two entirely separate notions ... but they're not "good" because the analysis is so limited. i think we were hoping for something more like the "used but not set" kind of diagnostic, but afaict it really only catches _direct_ use of a literal nullptr. so: foo(nullptr); // caught void* p = nullptr; foo(p); // not caught without getting on to anything fancy like: void* p; if (a) p = bar(); foo(p); we've done all the annotations anyway, but we're not expecting to actually catch many bugs with them, and in fact did not catch any real bugs in AOSP while adding the annotations. (though we did find several "you're not _wrong_, but ..." bits of code :-) ) what i really want for christmas is the annotation that lets me say "this size_t argument tells you the size of this array argument" and actually does something usefully fortify-like with that. i've seen proposals like https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/ but i haven't seen anything i can use yet, so you -- who do use GCC which aiui has something along these lines already -- will find out what's good/bad about it before i do... On Tue, Aug 8, 2023 at 3:01 AM Martin Uecker <muecker@gwdg.de> wrote: > > Am Montag, dem 10.07.2023 um 22:16 +0200 schrieb Alejandro Colomar via Gcc: > > On 7/10/23 22:14, Alejandro Colomar wrote: > > > [CC += Andrew] > > > > > > Hi Xi, Andrew, > > > > > > On 7/10/23 20:41, Xi Ruoyao wrote: > > > > Maybe we should have a weaker version of nonnull which only performs the > > > > diagnostic, not the optimization. But it looks like they hate the idea: > > > > https://gcc.gnu.org/PR110617. > > > > > > > This is the one thing that makes me use both Clang and GCC to compile, > > > because while any of them would be enough to build, I want as much > > > static analysis as I can get, and so I want -fanalyzer (so I need GCC), > > > but I also use _Nullable (so I need Clang). > > > > > > If GCC had support for _Nullable, I would have in GCC the superset of > > > features that I need from both in a single vendor. Moreover, Clang's > > > static analyzer is brain-damaged (sorry, but it doesn't have a simple > > > command line to run it, contrary to GCC's easy -fanalyzer), so having > > > GCC's analyzer get over those _Nullable qualifiers would be great. > > > > > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer > > > mode, as there are many cases where the compiler doesn't have enough > > > information, and the analyzer can get rid of false negatives and > > > positives. See: <https://github.com/llvm/llvm-project/issues/57546> > > > > > > I'll back the ask for the qualifiers in GCC, for compatibility with > > > Clang. > > > > BTW, Bionic libc is adding those qualifiers: > > > > <https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45> > > <https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability> > > > > > > I am sceptical about these qualifiers because they do > not fit well with this language mechanism. Qualifiers take > effect at accesses to objects and are discarded at lvalue > conversion. So a qualifier should ideally describe a property > that is relevant for accessing objects but is not relevant > for values. > > Also there are built-in conversion rules a qualifier should > conform to. A pointer pointing to a type without qualifier > can implicitely convert to a pointer to a type with qualifier, > e.g. int* can be converted to const int*. > > Together, this implies that a qualifier should add constraints > to a type that are relevant to how an object is accessed. > > "const" and "volatile" or "_Atomic" are good examples. > ("restricted" does not quite follow these rules and is > considered weird and difficult to understand.) > > In contrast, "nonnull" and "nullable" are properties that > affect the set of values of the pointer, but not how the > pointer itself is accessed. > > > Martin > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) 2023-08-09 0:14 ` enh @ 2023-08-09 1:11 ` Siddhesh Poyarekar 2023-08-09 7:26 ` Martin Uecker 1 sibling, 0 replies; 12+ messages in thread From: Siddhesh Poyarekar @ 2023-08-09 1:11 UTC (permalink / raw) To: enh, Martin Uecker Cc: Alejandro Colomar, Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Zack Weinberg, gcc On 2023-08-08 20:14, enh wrote: > (bionic maintainer here, mostly by accident...) > > yeah, we tried the GCC attributes once before with _disastrous_ > results (because GCC saw it as an excuse to delete explicit null > checks, it broke all kinds of things). the clang attributes are AFAICT based on earlier discussions around this patch, the NULL check deletion anomalies in gcc seem to have been fixed recently. > "better" in that they don't confuse two entirely separate notions ... > but they're not "good" because the analysis is so limited. i think we > were hoping for something more like the "used but not set" kind of > diagnostic, but afaict it really only catches _direct_ use of a > literal nullptr. so: > > foo(nullptr); // caught > > void* p = nullptr; > foo(p); // not caught > > without getting on to anything fancy like: > > void* p; > if (a) p = bar(); > foo(p); > > we've done all the annotations anyway, but we're not expecting to > actually catch many bugs with them, and in fact did not catch any real > bugs in AOSP while adding the annotations. (though we did find several > "you're not _wrong_, but ..." bits of code :-) ) I believe it did catch a few issues in the glibc test cases when we enabled fortification internally in addition to flagging several "you're not _wrong_, but..." bits. In any case, it's only enabled with fortification since we use that as a hint for wanting safer code. > what i really want for christmas is the annotation that lets me say > "this size_t argument tells you the size of this array argument" and > actually does something usefully fortify-like with that. i've seen > proposals like https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/ > but i haven't seen anything i can use yet, so you -- who do use GCC > which aiui has something along these lines already -- will find out > what's good/bad about it before i do... I think a lot of this is covered by __access__, __alloc_size__ and the upcoming __counted_by__ attributes. There are still gaps that something like -fbounds-safety would cover (I think -fsanitize=bounds and -fsanitize=object-size further cover some of those gaps but there's a general lack of confidence in using them in production due to performance concerns; fbounds-safety has those concerns too AFAICT) but that gap is getting narrower. Thanks, Sid ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) 2023-08-09 0:14 ` enh 2023-08-09 1:11 ` Siddhesh Poyarekar @ 2023-08-09 7:26 ` Martin Uecker 2023-08-09 10:42 ` ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) Alejandro Colomar 2023-08-11 23:34 ` _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) enh 1 sibling, 2 replies; 12+ messages in thread From: Martin Uecker @ 2023-08-09 7:26 UTC (permalink / raw) To: enh Cc: Alejandro Colomar, Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc Am Dienstag, dem 08.08.2023 um 17:14 -0700 schrieb enh: > (bionic maintainer here, mostly by accident...) > > yeah, we tried the GCC attributes once before with _disastrous_ > results (because GCC saw it as an excuse to delete explicit null > checks, it broke all kinds of things). Thanks! I am currently exploring different options and what could be done to improve the situation from the language as well as compile side. It looks we have some partial solution but they do not work quite right or do not fit together perfectly. > the clang attributes are > "better" in that they don't confuse two entirely separate notions ... > but they're not "good" because the analysis is so limited. i think we > were hoping for something more like the "used but not set" kind of > diagnostic, but afaict it really only catches _direct_ use of a > literal nullptr. so: > > foo(nullptr); // caught > > void* p = nullptr; > foo(p); // not caught > > without getting on to anything fancy like: > > void* p; > if (a) p = bar(); > foo(p); > > we've done all the annotations anyway, but we're not expecting to > actually catch many bugs with them, and in fact did not catch any real > bugs in AOSP while adding the annotations. (though we did find several > "you're not _wrong_, but ..." bits of code :-) ) > > what i really want for christmas is the annotation that lets me say > "this size_t argument tells you the size of this array argument" and > actually does something usefully fortify-like with that. > What is your opinion about the access attribute? https://godbolt.org/z/PPfajefvP it is a bit cumbersome to use, but one can use [static] instead, which gives you the same static warnings. [static] does not work with __builtin_dynamic_object_size, but maybe this could be changed (there is a bug filed.) I am not sure whether [static] should imply [[gnu::nonnull]] which would then also trigger the optimization. I think clang uses it for optimization. Martin > i've seen > proposals like https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/ > but i haven't seen anything i can use yet, so you -- who do use GCC > which aiui has something along these lines already -- will find out > what's good/bad about it before i do... > > On Tue, Aug 8, 2023 at 3:01 AM Martin Uecker <muecker@gwdg.de> wrote: > > > > Am Montag, dem 10.07.2023 um 22:16 +0200 schrieb Alejandro Colomar via Gcc: > > > On 7/10/23 22:14, Alejandro Colomar wrote: > > > > [CC += Andrew] > > > > > > > > Hi Xi, Andrew, > > > > > > > > On 7/10/23 20:41, Xi Ruoyao wrote: > > > > > Maybe we should have a weaker version of nonnull which only performs the > > > > > diagnostic, not the optimization. But it looks like they hate the idea: > > > > > https://gcc.gnu.org/PR110617. > > > > > > > > > This is the one thing that makes me use both Clang and GCC to compile, > > > > because while any of them would be enough to build, I want as much > > > > static analysis as I can get, and so I want -fanalyzer (so I need GCC), > > > > but I also use _Nullable (so I need Clang). > > > > > > > > If GCC had support for _Nullable, I would have in GCC the superset of > > > > features that I need from both in a single vendor. Moreover, Clang's > > > > static analyzer is brain-damaged (sorry, but it doesn't have a simple > > > > command line to run it, contrary to GCC's easy -fanalyzer), so having > > > > GCC's analyzer get over those _Nullable qualifiers would be great. > > > > > > > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer > > > > mode, as there are many cases where the compiler doesn't have enough > > > > information, and the analyzer can get rid of false negatives and > > > > positives. See: <https://github.com/llvm/llvm-project/issues/57546> > > > > > > > > I'll back the ask for the qualifiers in GCC, for compatibility with > > > > Clang. > > > > > > BTW, Bionic libc is adding those qualifiers: > > > > > > <https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45> > > > <https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability> > > > > > > > > > > I am sceptical about these qualifiers because they do > > not fit well with this language mechanism. Qualifiers take > > effect at accesses to objects and are discarded at lvalue > > conversion. So a qualifier should ideally describe a property > > that is relevant for accessing objects but is not relevant > > for values. > > > > Also there are built-in conversion rules a qualifier should > > conform to. A pointer pointing to a type without qualifier > > can implicitely convert to a pointer to a type with qualifier, > > e.g. int* can be converted to const int*. > > > > Together, this implies that a qualifier should add constraints > > to a type that are relevant to how an object is accessed. > > > > "const" and "volatile" or "_Atomic" are good examples. > > ("restricted" does not quite follow these rules and is > > considered weird and difficult to understand.) > > > > In contrast, "nonnull" and "nullable" are properties that > > affect the set of values of the pointer, but not how the > > pointer itself is accessed. > > > > > > Martin > > > > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) 2023-08-09 7:26 ` Martin Uecker @ 2023-08-09 10:42 ` Alejandro Colomar 2023-08-09 12:03 ` Martin Uecker 2023-08-09 13:46 ` Xi Ruoyao 2023-08-11 23:34 ` _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) enh 1 sibling, 2 replies; 12+ messages in thread From: Alejandro Colomar @ 2023-08-09 10:42 UTC (permalink / raw) To: Martin Uecker Cc: Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc, enh [-- Attachment #1.1: Type: text/plain, Size: 2551 bytes --] Hi Martin, On 2023-08-09 09:26, Martin Uecker wrote: > it is a bit cumbersome to use, but one can use [static] > instead, which gives you the same static warnings. > > [static] does not work with __builtin_dynamic_object_size, > but maybe this could be changed (there is a bug filed.) > > I am not sure whether [static] should imply [[gnu::nonnull]] I have a gripe with ISO C's [static]. As you mention, ISO conflated two functionalities in [static]: - The size of the array passed as argument must not be less than the size specified in the parameter's []. - The pointer must not be NULL. And there are valid cases where you may want the first but not the second. Or the second but not the first (that's the case for _Nonnull, of course). In fact, it's so badly damaged, that it prompted a proposal to ISO C of using [static 1] as an equivalent of _Nonnull in the prototypes that accepted a pointer that should not be NULL. However, that proposal didn't include the functions that actually take arrays as input (because they are taken in the opposite order, so array syntax is not legal). Don't you find it ironic that ISO C could have used array syntax for pointers and pointer syntax for arrays? I do. As for when one would want to mean the first (size of array) but not _Nonnull: for a function where you may pass either an array (which should not be smaller than the size), or a sentinel NULL value. Nevertheless, I floated the idea that [static] is completely unnecessary, and nobody has yet been against it. GCC could perfectly add a warning for the following case: void foo(size_t n, int a[n]); int main(void) { int a[7]; foo(42, a); } Nobody in their right mind would specify a size of an array in a parameter and expect that passing a smaller array than that can produce a valid program. So, why not make that a Wall warning? And so [static] would be irrelevant in GNU C, because well, what does it add? In fact, I like that [static] is so badly designed, because then we can repurpose plain [size] to mean the right thing, which would produce cleaner programs ([static] just adds noise to the source). What do you think of giving [42] a meaning, instead of just ignoring it? Cheers, Alex > which would then also trigger the optimization. I think > clang uses it for optimization. > > Martin -- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) 2023-08-09 10:42 ` ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) Alejandro Colomar @ 2023-08-09 12:03 ` Martin Uecker 2023-08-09 12:37 ` Alejandro Colomar 2023-08-09 13:46 ` Xi Ruoyao 1 sibling, 1 reply; 12+ messages in thread From: Martin Uecker @ 2023-08-09 12:03 UTC (permalink / raw) To: Alejandro Colomar Cc: Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc, enh Hi Alejandro! Am Mittwoch, dem 09.08.2023 um 12:42 +0200 schrieb Alejandro Colomar: ... > > As for when one would want to mean the first (size of array) > but not _Nonnull: for a function where you may pass either > an array (which should not be smaller than the size), or a > sentinel NULL value. > > Nevertheless, I floated the idea that [static] is completely > unnecessary, and nobody has yet been against it. > > GCC could perfectly add a warning for the following case: > > void foo(size_t n, int a[n]); > > int > main(void) > { > int a[7]; > > foo(42, a); > } > > Nobody in their right mind would specify a size of an array > in a parameter and expect that passing a smaller array than > that can produce a valid program. So, why not make that a > Wall warning? But we have this warning! is even activated by default without -Wall and already since GCC 11: https://godbolt.org/z/sMbTon458 But this is for minimum required elements. How do we differentiate between null and non-null? We have: int[] or int* // no bound, nullable int[N] // at least N, nullable int[static N] // at least N, nonnull The 'static' implies nonnull, so we could use 'static' to diffentiate between nonnull and nullable. What is missing something which implies bounds also inside the callee. You can use the "access" attribute or we extend the meaning of int[N] and int[static N] also imply a maximum bound. Martin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) 2023-08-09 12:03 ` Martin Uecker @ 2023-08-09 12:37 ` Alejandro Colomar 2023-08-09 14:24 ` Martin Uecker 0 siblings, 1 reply; 12+ messages in thread From: Alejandro Colomar @ 2023-08-09 12:37 UTC (permalink / raw) To: Martin Uecker Cc: Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc, enh [-- Attachment #1.1: Type: text/plain, Size: 3822 bytes --] Hi Martin! On 2023-08-09 14:03, Martin Uecker wrote: [...] >> GCC could perfectly add a warning for the following case: >> >> void foo(size_t n, int a[n]); >> >> int >> main(void) >> { >> int a[7]; >> >> foo(42, a); >> } >> >> Nobody in their right mind would specify a size of an array >> in a parameter and expect that passing a smaller array than >> that can produce a valid program. So, why not make that a >> Wall warning? > > But we have this warning! is even activated by > default without -Wall and already since GCC 11: Ahh, I hadn't realized that was added (relatievly) recently. That's great news! Thanks! > https://godbolt.org/z/sMbTon458 > > But this is for minimum required elements. How do > we differentiate between null and non-null? > > We have: > > int[] or int* // no bound, nullable > int[N] // at least N, nullable > int[static N] // at least N, nonnull > > The 'static' implies nonnull, so we could > use 'static' to diffentiate between nonnull > and nullable. Since static is only for arrays (okay, they're all pointers, but it's only usable with array syntax), and nullability is a thing of pointers, I think static is not the right choice. I oppose using array syntax for pointers, as it can confuse programmers. In any case, [static] is not that different from [_Nonnull], and _Nonnull is more flexible, since you can also use it as *_Nonnull. Regarding the issues you have with _Nonnull being a qualifier, I've been thinking about it for a long time and don't yet have a concrete answer. The more I think about it, the more I'm convinced it is like `restrict`. You can't have null correctness as we can do with `const`. With const, we know it's always bad to store a const object in a non-const pointer. With NULL, it depends on the context: if you've checked for NULL in the lines above, then it's fine to do the conversion. There is a proposal for Clang to add `_Optional`, a qualifier to the pointee, as a more correct version of _Nullable. The following would be equivalent: int *_Nullable i; _Optional int *j; However, I don't believe it's a good choice, because of the above. Instead, I think _Nullable or _Nonnull should be like `restrict` and used only in function prototypes. Let the analyzer do the job. I know `restrict` is hard to grasp, but I couldn't think of anything better. > > What is missing something which implies bounds > also inside the callee. You can use the "access" > attribute or we extend the meaning of int[N] > and int[static N] also imply a maximum bound. Why is the upper bound important? It's usually irrelevant. In the case where one wants to make sure that an array is of an exact size (instead of just "at least"), pointers to arrays can be used. They're just not often used, because you don't need that so often. I don't think we need a new addition to the language, do we? $ cat ap.c #include <stddef.h> void foo(size_t n, int (*a)[n]) { for (size_t i = 0; i < n; i++) (*a)[i] = 42; } $ gcc-13 -S -Wall -Wextra ap.c $ In the function above, n is not a lower bound, but an exact bound. GCC should add a warning for the following code: $ cat ap2.c #include <stddef.h> void foo(size_t n, int (*a)[n]); void bar(void) { int x[7]; foo(3, &x); foo(9, &x); } $ gcc-13 -S -Wall -Wextra ap2.c $ Neither GCC nor Clang warn about that. Not even with -fanalyzer. Cheers, Alex -- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) 2023-08-09 12:37 ` Alejandro Colomar @ 2023-08-09 14:24 ` Martin Uecker 0 siblings, 0 replies; 12+ messages in thread From: Martin Uecker @ 2023-08-09 14:24 UTC (permalink / raw) To: Alejandro Colomar Cc: Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc, enh Am Mittwoch, dem 09.08.2023 um 14:37 +0200 schrieb Alejandro Colomar: > Hi Martin! > > On 2023-08-09 14:03, Martin Uecker wrote: > > Regarding the issues you have with _Nonnull being a qualifier, > I've been thinking about it for a long time and don't yet have > a concrete answer. The more I think about it, the more I'm > convinced it is like `restrict`. You can't have null correctness > as we can do with `const`. With const, we know it's always bad > to store a const object in a non-const pointer. With NULL, it > depends on the context: if you've checked for NULL in the lines > above, then it's fine to do the conversion. > > There is a proposal for Clang to add `_Optional`, a qualifier to > the pointee, as a more correct version of _Nullable. The > following would be equivalent: > > int *_Nullable i; > _Optional int *j; > > However, I don't believe it's a good choice, because of the above. > Instead, I think _Nullable or _Nonnull should be like `restrict` > and used only in function prototypes. Let the analyzer do the > job. I know `restrict` is hard to grasp, but I couldn't think of > anything better. I think this would be bad. More confusion is the least we need. I would definitely prefer an attribute over a confusing qualifier which is not really a qualifier. _Optional is a something else and maybe not directly comparable. I think it is an interesting idea. First, it would fit well with rules the language. One gets conversion errors exactly as with other qualifiers. So one could add it to selected pointer in an existing code base and incrementally improve the code by adding checks. It would not be useful for existing interfaces that need stay compatible. > > > > What is missing something which implies bounds > > also inside the callee. You can use the "access" > > attribute or we extend the meaning of int[N] > > and int[static N] also imply a maximum bound. > > Why is the upper bound important? It's usually irrelevant. > > In the case where one wants to make sure that an array is of an > exact size (instead of just "at least"), pointers to arrays can be > used. They're just not often used, because you don't need that > so often. I don't think we need a new addition to the language, > do we? > > $ cat ap.c > #include <stddef.h> > > void > foo(size_t n, int (*a)[n]) > { > for (size_t i = 0; i < n; i++) > (*a)[i] = 42; > } > $ gcc-13 -S -Wall -Wextra ap.c > $ > > In the function above, n is not a lower bound, but an exact bound. > > GCC should add a warning for the following code: > > $ cat ap2.c > #include <stddef.h> > > void foo(size_t n, int (*a)[n]); > > void > bar(void) > { > int x[7]; > > foo(3, &x); > foo(9, &x); > } > $ gcc-13 -S -Wall -Wextra ap2.c > $ > > Neither GCC nor Clang warn about that. Not even with -fanalyzer. I know. One can already get bounds checking side the callee with this using UBSan. I have UBSan patch which would protect the call itself and make sure the bounds are correct. Compile-time warnings would also be good. In any case, this does not work for existing interfaces. Martin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) 2023-08-09 10:42 ` ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) Alejandro Colomar 2023-08-09 12:03 ` Martin Uecker @ 2023-08-09 13:46 ` Xi Ruoyao 1 sibling, 0 replies; 12+ messages in thread From: Xi Ruoyao @ 2023-08-09 13:46 UTC (permalink / raw) To: Alejandro Colomar, Martin Uecker Cc: Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc, enh On Wed, 2023-08-09 at 12:42 +0200, Alejandro Colomar wrote: > I have a gripe with ISO C's [static]. As you mention, ISO > conflated two functionalities in [static]: > > - The size of the array passed as argument must not be less > than the size specified in the parameter's []. > > - The pointer must not be NULL. https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625559.html If this patch is merged, they'll just become __nonnull and have all advantages and disadvantages as __nonnull. -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) 2023-08-09 7:26 ` Martin Uecker 2023-08-09 10:42 ` ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) Alejandro Colomar @ 2023-08-11 23:34 ` enh 1 sibling, 0 replies; 12+ messages in thread From: enh @ 2023-08-11 23:34 UTC (permalink / raw) To: Martin Uecker Cc: Alejandro Colomar, Xi Ruoyao, Andrew Pinski, GNU libc development, Adhemerval Zanella, Carlos O'Donell, Andreas Schwab, Siddhesh Poyarekar, Zack Weinberg, gcc On Wed, Aug 9, 2023 at 12:26 AM Martin Uecker <muecker@gwdg.de> wrote: > > Am Dienstag, dem 08.08.2023 um 17:14 -0700 schrieb enh: > > (bionic maintainer here, mostly by accident...) > > > > yeah, we tried the GCC attributes once before with _disastrous_ > > results (because GCC saw it as an excuse to delete explicit null > > checks, it broke all kinds of things). > > Thanks! I am currently exploring different options and what > could be done to improve the situation from the language > as well as compile side. It looks we have some partial > solution but they do not work quite right or do not > fit together perfectly. > > > > the clang attributes are > > "better" in that they don't confuse two entirely separate notions ... > > but they're not "good" because the analysis is so limited. i think we > > were hoping for something more like the "used but not set" kind of > > diagnostic, but afaict it really only catches _direct_ use of a > > literal nullptr. so: > > > > foo(nullptr); // caught > > > > void* p = nullptr; > > foo(p); // not caught > > > > without getting on to anything fancy like: > > > > void* p; > > if (a) p = bar(); > > foo(p); > > > > we've done all the annotations anyway, but we're not expecting to > > actually catch many bugs with them, and in fact did not catch any real > > bugs in AOSP while adding the annotations. (though we did find several > > "you're not _wrong_, but ..." bits of code :-) ) > > > > what i really want for christmas is the annotation that lets me say > > "this size_t argument tells you the size of this array argument" and > > actually does something usefully fortify-like with that. > > > > What is your opinion about the access attribute? > > https://godbolt.org/z/PPfajefvP > > it is a bit cumbersome to use, but one can use [static] > instead, which gives you the same static warnings. yeah, "that's hard to read" was actually my first reaction. maybe it's just because i'm a libc maintainer used to the printf_like attribute, but i actually find the regular __attribute__() style more readable. you probably need to ask people who _consume_ more headers than they write :-) > [static] does not work with __builtin_dynamic_object_size, > but maybe this could be changed (there is a bug filed.) > > I am not sure whether [static] should imply [[gnu::nonnull]] > which would then also trigger the optimization. I think > clang uses it for optimization. yeah, that was what made us revert the old gcc nonnull annotations --- we don't have any use case for "please use this to omit checks" because we're generally dealing with public API. having the compiler dead-code eliminate our attempts to return sensible errors was counter to our goals, and seems like it would be in any place where we'd use a bounds annotation too --- it'll be those worried about security (who want to fail fast, and _before_ adding a potentially caller-controlled index to an array base address!) who i'd expect to see using this kind of thing first. > Martin > > > > i've seen > > proposals like https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/ > > but i haven't seen anything i can use yet, so you -- who do use GCC > > which aiui has something along these lines already -- will find out > > what's good/bad about it before i do... > > > > > > > On Tue, Aug 8, 2023 at 3:01 AM Martin Uecker <muecker@gwdg.de> wrote: > > > > > > Am Montag, dem 10.07.2023 um 22:16 +0200 schrieb Alejandro Colomar via Gcc: > > > > On 7/10/23 22:14, Alejandro Colomar wrote: > > > > > [CC += Andrew] > > > > > > > > > > Hi Xi, Andrew, > > > > > > > > > > On 7/10/23 20:41, Xi Ruoyao wrote: > > > > > > Maybe we should have a weaker version of nonnull which only performs the > > > > > > diagnostic, not the optimization. But it looks like they hate the idea: > > > > > > https://gcc.gnu.org/PR110617. > > > > > > > > > > > This is the one thing that makes me use both Clang and GCC to compile, > > > > > because while any of them would be enough to build, I want as much > > > > > static analysis as I can get, and so I want -fanalyzer (so I need GCC), > > > > > but I also use _Nullable (so I need Clang). > > > > > > > > > > If GCC had support for _Nullable, I would have in GCC the superset of > > > > > features that I need from both in a single vendor. Moreover, Clang's > > > > > static analyzer is brain-damaged (sorry, but it doesn't have a simple > > > > > command line to run it, contrary to GCC's easy -fanalyzer), so having > > > > > GCC's analyzer get over those _Nullable qualifiers would be great. > > > > > > > > > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer > > > > > mode, as there are many cases where the compiler doesn't have enough > > > > > information, and the analyzer can get rid of false negatives and > > > > > positives. See: <https://github.com/llvm/llvm-project/issues/57546> > > > > > > > > > > I'll back the ask for the qualifiers in GCC, for compatibility with > > > > > Clang. > > > > > > > > BTW, Bionic libc is adding those qualifiers: > > > > > > > > <https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45> > > > > <https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability> > > > > > > > > > > > > > > I am sceptical about these qualifiers because they do > > > not fit well with this language mechanism. Qualifiers take > > > effect at accesses to objects and are discarded at lvalue > > > conversion. So a qualifier should ideally describe a property > > > that is relevant for accessing objects but is not relevant > > > for values. > > > > > > Also there are built-in conversion rules a qualifier should > > > conform to. A pointer pointing to a type without qualifier > > > can implicitely convert to a pointer to a type with qualifier, > > > e.g. int* can be converted to const int*. > > > > > > Together, this implies that a qualifier should add constraints > > > to a type that are relevant to how an object is accessed. > > > > > > "const" and "volatile" or "_Atomic" are good examples. > > > ("restricted" does not quite follow these rules and is > > > considered weird and difficult to understand.) > > > > > > In contrast, "nonnull" and "nullable" are properties that > > > affect the set of values of the pointer, but not how the > > > pointer itself is accessed. > > > > > > > > > Martin > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-08-11 23:34 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230710161300.1678172-1-xry111@xry111.site> [not found] ` <a3a0c195-1149-461b-807e-46eaa3d68fcc@app.fastmail.com> [not found] ` <ed86d013-1df5-2880-3e39-0caf8f49a999@gotplt.org> [not found] ` <1efbe0b2dd8fefffc945c6734222c7d6e04cf465.camel@xry111.site> 2023-07-10 20:14 ` _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) Alejandro Colomar 2023-07-10 20:16 ` Alejandro Colomar 2023-08-08 10:01 ` Martin Uecker 2023-08-09 0:14 ` enh 2023-08-09 1:11 ` Siddhesh Poyarekar 2023-08-09 7:26 ` Martin Uecker 2023-08-09 10:42 ` ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer) Alejandro Colomar 2023-08-09 12:03 ` Martin Uecker 2023-08-09 12:37 ` Alejandro Colomar 2023-08-09 14:24 ` Martin Uecker 2023-08-09 13:46 ` Xi Ruoyao 2023-08-11 23:34 ` _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h) enh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).