Обсуждение: Add support for __attribute__((returns_nonnull))
Here is a patch which adds support for the returns_nonnull attribute alongside all the other attributes we optionally support. I recently wound up in a situation where I was checking for NULL return values of a function that couldn't ever return NULL because the inability to allocate memory was always elog(ERROR)ed (aborted). I didn't go through and mark anything, but I feel like it could be useful for people going forward, including myself. -- Tristan Partin Neon (https://neon.tech)
Вложения
On 19.12.23 21:43, Tristan Partin wrote: > Here is a patch which adds support for the returns_nonnull attribute > alongside all the other attributes we optionally support. > > I recently wound up in a situation where I was checking for NULL return > values of a function that couldn't ever return NULL because the > inability to allocate memory was always elog(ERROR)ed (aborted). > > I didn't go through and mark anything, but I feel like it could be > useful for people going forward, including myself. I think it would be useful if this patch series contained a patch that added some initial uses of this. That way we can check that the proposed definition actually works, and we can observe what it does, with respect to warnings, static analysis, etc.
On Wed Dec 27, 2023 at 6:42 AM CST, Peter Eisentraut wrote: > On 19.12.23 21:43, Tristan Partin wrote: > > Here is a patch which adds support for the returns_nonnull attribute > > alongside all the other attributes we optionally support. > > > > I recently wound up in a situation where I was checking for NULL return > > values of a function that couldn't ever return NULL because the > > inability to allocate memory was always elog(ERROR)ed (aborted). > > > > I didn't go through and mark anything, but I feel like it could be > > useful for people going forward, including myself. > > I think it would be useful if this patch series contained a patch that > added some initial uses of this. That way we can check that the > proposed definition actually works, and we can observe what it does, > with respect to warnings, static analysis, etc. Good point. Patch attached. I tried to find some ways to prove some value, but I couldn't. Take this example for instance: static const char word[] = { 'h', 'e', 'l', 'l', 'o' }; const char * __attribute__((returns_nonnull)) hello() { return word; } int main(void) { const char *r; r = hello(); if (r == NULL) return 1; return 0; } I would have thought I could get gcc or clang to warn on a wasteful NULL check, but alas. I also didn't see any code generation improvements, but I am assuming that the example is too contrived. I couldn't find any good things online that had examples of when such an annotation forced the compiler to warn or create more optimized code. If you return NULL from the hello() function, clang will warn that the attribute doesn't match reality. -- Tristan Partin Neon (https://neon.tech)
Вложения
On Thu, Dec 28, 2023 at 1:20 AM Tristan Partin <tristan@neon.tech> wrote: > I recently wound up in a situation where I was checking for NULL return > values of a function that couldn't ever return NULL because the > inability to allocate memory was always elog(ERROR)ed (aborted). It sounds like you have a ready example to test, so what happens with the patch? As for whether any code generation changed, I'd start by checking if anything in a non-debug binary changed at all.
On Sun Dec 31, 2023 at 9:29 PM CST, John Naylor wrote: > On Thu, Dec 28, 2023 at 1:20 AM Tristan Partin <tristan@neon.tech> wrote: > > I recently wound up in a situation where I was checking for NULL return > > values of a function that couldn't ever return NULL because the > > inability to allocate memory was always elog(ERROR)ed (aborted). > > It sounds like you have a ready example to test, so what happens with the patch? > > As for whether any code generation changed, I'd start by checking if > anything in a non-debug binary changed at all. The idea I had in mind initially was PGLC_localeconv(), but I couldn't prove that anything changed with the annotation added. The second patch in my previous email was attempt at deriving real-world benefit, but nothing I did seemed to change anything. Perhaps you can test it and see if anything changes for you. -- Tristan Partin Neon (https://neon.tech)
On Mon, Jan 08, 2024 at 05:04:58PM -0600, Tristan Partin wrote: > The idea I had in mind initially was PGLC_localeconv(), but I couldn't > prove that anything changed with the annotation added. The second patch > in my previous email was attempt at deriving real-world benefit, but > nothing I did seemed to change anything. Perhaps you can test it and see > if anything changes for you. There are a bunch of places in the tree where we return NULL just to keep the compiler quiet, and where we should never return NULL, as in: return NULL; /* keep compiler quiet */ See bbstreamer_gzip.c as one example for non-gzip builds. Perhaps you could look at one of these places and see if the marker shows benefits? -- Michael