| Message ID | 20210728050444.3843432-1-siddhesh@gotplt.org |
|---|---|
| State | New |
| Headers | show |
| Series | analyzer: Recognize __builtin_free as a matching deallocator | expand |
On Wed, 2021-07-28 at 10:34 +0530, Siddhesh Poyarekar wrote: > Recognize __builtin_free as being equivalent to free when passed into > __attribute__((malloc ())), similar to how it is treated when it is > encountered as a call. This fixes spurious warnings in glibc where > xmalloc family of allocators as well as reallocarray, memalign, > etc. are declared to have __builtin_free as the free function. > > gcc/analyzer/ChangeLog: > * sm-malloc.cc > (malloc_state_machine::get_or_create_deallocator): Recognize > __builtin_free. > > gcc/testsuite/ChangeLog: > * gcc.dg/analyzer/attr-malloc-1.c (compatible_alloc, > compatible_alloc2): New extern allocator declarations. > (test_9, test_10): New tests. Looks good to me, thanks Dave
On 7/28/21 1:44 PM, David Malcolm via Gcc-patches wrote: > On Wed, 2021-07-28 at 10:34 +0530, Siddhesh Poyarekar wrote: >> Recognize __builtin_free as being equivalent to free when passed into >> __attribute__((malloc ())), similar to how it is treated when it is >> encountered as a call. This fixes spurious warnings in glibc where >> xmalloc family of allocators as well as reallocarray, memalign, >> etc. are declared to have __builtin_free as the free function. >> >> gcc/analyzer/ChangeLog: >> * sm-malloc.cc >> (malloc_state_machine::get_or_create_deallocator): Recognize >> __builtin_free. >> >> gcc/testsuite/ChangeLog: >> * gcc.dg/analyzer/attr-malloc-1.c (compatible_alloc, >> compatible_alloc2): New extern allocator declarations. >> (test_9, test_10): New tests. > > Looks good to me, thanks > Dave > > Please could this be backported to all active branches?
On 8/25/21 5:44 PM, Matthias Klose wrote: > On 7/28/21 1:44 PM, David Malcolm via Gcc-patches wrote: >> On Wed, 2021-07-28 at 10:34 +0530, Siddhesh Poyarekar wrote: >>> Recognize __builtin_free as being equivalent to free when passed into >>> __attribute__((malloc ())), similar to how it is treated when it is >>> encountered as a call. This fixes spurious warnings in glibc where >>> xmalloc family of allocators as well as reallocarray, memalign, >>> etc. are declared to have __builtin_free as the free function. >>> >>> gcc/analyzer/ChangeLog: >>> * sm-malloc.cc >>> (malloc_state_machine::get_or_create_deallocator): Recognize >>> __builtin_free. >>> >>> gcc/testsuite/ChangeLog: >>> * gcc.dg/analyzer/attr-malloc-1.c (compatible_alloc, >>> compatible_alloc2): New extern allocator declarations. >>> (test_9, test_10): New tests. >> >> Looks good to me, thanks >> Dave >> >> > > > Please could this be backported to all active branches? > Sure, it looks like only gcc11 needs this since malloc attribute matching seems recent. David, I've never done a backport before, may I just cherry-pick, push and post a [committed] patch on list or does it need to go through review? Thanks, Siddhesh
On 8/25/2021 9:16 AM, Siddhesh Poyarekar wrote: > On 8/25/21 5:44 PM, Matthias Klose wrote: >> On 7/28/21 1:44 PM, David Malcolm via Gcc-patches wrote: >>> On Wed, 2021-07-28 at 10:34 +0530, Siddhesh Poyarekar wrote: >>>> Recognize __builtin_free as being equivalent to free when passed into >>>> __attribute__((malloc ())), similar to how it is treated when it is >>>> encountered as a call. This fixes spurious warnings in glibc where >>>> xmalloc family of allocators as well as reallocarray, memalign, >>>> etc. are declared to have __builtin_free as the free function. >>>> >>>> gcc/analyzer/ChangeLog: >>>> * sm-malloc.cc >>>> (malloc_state_machine::get_or_create_deallocator): Recognize >>>> __builtin_free. >>>> >>>> gcc/testsuite/ChangeLog: >>>> * gcc.dg/analyzer/attr-malloc-1.c (compatible_alloc, >>>> compatible_alloc2): New extern allocator declarations. >>>> (test_9, test_10): New tests. >>> >>> Looks good to me, thanks >>> Dave >>> >>> >> >> >> Please could this be backported to all active branches? >> > > Sure, it looks like only gcc11 needs this since malloc attribute > matching seems recent. David, I've never done a backport before, may > I just cherry-pick, push and post a [committed] patch on list or does > it need to go through review? In general it's fine to cherry-pick fixes from the mainline to a release branch that fix regressions, incorrect code and the like. This doesn't fall into one of those categories -- but given this is limited to the analyzer, I think it's fine to cherry-pick into the release branch as long as David is OK with it as well. jeff
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc index 9707a6863ce..1d69d57df0e 100644 --- a/gcc/analyzer/sm-malloc.cc +++ b/gcc/analyzer/sm-malloc.cc @@ -1511,7 +1511,8 @@ malloc_state_machine::get_or_create_deallocator (tree deallocator_fndecl) /* Reuse "free". */ deallocator *d; if (is_named_call_p (deallocator_fndecl, "free") - || is_std_named_call_p (deallocator_fndecl, "free")) + || is_std_named_call_p (deallocator_fndecl, "free") + || is_named_call_p (deallocator_fndecl, "__builtin_free")) d = &m_free.m_deallocator; else { diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c index 3de32b1b14b..a9a2a3dee85 100644 --- a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c @@ -11,6 +11,12 @@ extern struct foo *foo_acquire (void) extern void use_foo (const struct foo *) __attribute__((nonnull)); +extern struct foo *compatible_alloc (void) + __attribute__ ((malloc (__builtin_free))); + +extern struct foo *compatible_alloc2 (void) + __attribute__ ((malloc (free))); + void test_1 (void) { struct foo *p = foo_acquire (); @@ -73,3 +79,16 @@ int test_8 (struct foo *p) foo_release (p); return p->m_int; /* { dg-warning "use after 'foo_release' of 'p'" } */ } + +/* Recognize that __builtin_free and free are the same thing. */ +void test_9 (void) +{ + struct foo *p = compatible_alloc (); + free (p); +} + +void test_10 (void) +{ + struct foo *p = compatible_alloc2 (); + __builtin_free (p); +}