https://gcc.gnu.org/g:ce88ece514dcedd41fb9d28943ad37b927ec5621
commit r17-4212-gce88ece514dcedd41fb9d28943ad37b927ec5621 Author: Rohith Kapelli <[email protected]> Date: Sat Aug 15 21:47:34 2026 +0530 ipa-icf: compare the nonnull contract of two functions [PR123227] Two functions with identical bodies can still make different promises about their arguments, and ICF was not looking at those promises. get_nonnull_args derives them from two places: the nonnull attribute on the function type, and a METHOD_TYPE, whose this pointer is nonnull whether or not anything says so. sem_function::equals_wpa compared the type attributes with comp_type_attributes, which by design only reports attributes whose affects_type_identity is set, and nonnull is not one of those, and it never compared FUNCTION_TYPE against METHOD_TYPE outside a devirtualization specific check. Either way the two functions were declared equal and unified. The surviving body keeps its own promise, a later pass drops the null test that promise makes redundant, and a call through the other symbol dereferences the null pointer that symbol was required to accept. Type identity is the wrong question for ICF. The comment above the variable case already says that for functions we do not know which attributes affect code generation, which is why the decl attributes are compared as whole lists. Compare the type attributes the same way, and reject a METHOD_TYPE paired with a FUNCTION_TYPE, which carries the same promise with nothing on the attribute list to compare. PR ipa/123227 gcc/ChangeLog: * ipa-icf.cc (sem_function::equals_wpa): Compare TYPE_ATTRIBUTES as a list rather than through comp_type_attributes. Reject a METHOD_TYPE compared against a FUNCTION_TYPE. gcc/testsuite/ChangeLog: * gcc.dg/ipa/pr123227.c: New test. * g++.dg/ipa/pr123227.C: New test. * gcc.dg/lto/pr123227_0.c: New test. * gcc.dg/lto/pr123227_1.c: New test. * g++.dg/lto/pr123227_0.C: New test. * g++.dg/lto/pr123227_1.C: New test. Signed-off-by: Rohith Kapelli <[email protected]> Diff: --- gcc/ipa-icf.cc | 16 +++++++++-- gcc/testsuite/g++.dg/ipa/pr123227.C | 50 +++++++++++++++++++++++++++++++++ gcc/testsuite/g++.dg/lto/pr123227_0.C | 52 +++++++++++++++++++++++++++++++++++ gcc/testsuite/g++.dg/lto/pr123227_1.C | 24 ++++++++++++++++ gcc/testsuite/gcc.dg/ipa/pr123227.c | 45 ++++++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/lto/pr123227_0.c | 47 +++++++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/lto/pr123227_1.c | 17 ++++++++++++ 7 files changed, 248 insertions(+), 3 deletions(-) diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc index 44501ec760e3..64cc722bd7aa 100644 --- a/gcc/ipa-icf.cc +++ b/gcc/ipa-icf.cc @@ -659,10 +659,20 @@ sem_function::equals_wpa (sem_item *item, return return_false_with_msg ("different number of references"); /* Checking function attributes. - This is quadratic in number of attributes */ - if (comp_type_attributes (TREE_TYPE (decl), - TREE_TYPE (item->decl)) != 1) + This is quadratic in number of attributes. + comp_type_attributes only considers attributes that affect type + identity, but an attribute that leaves the type alone can still let + the body assume something, nonnull being one, so compare the lists + the same way the decl attributes are compared below. */ + if (!attribute_list_equal (TYPE_ATTRIBUTES (TREE_TYPE (decl)), + TYPE_ATTRIBUTES (TREE_TYPE (item->decl)))) return return_false_with_msg ("different type attributes"); + /* A METHOD_TYPE promises a nonnull this pointer without carrying an + attribute that says so, so it is not interchangeable with a + FUNCTION_TYPE that makes no such promise. */ + if ((TREE_CODE (TREE_TYPE (decl)) == METHOD_TYPE) + != (TREE_CODE (TREE_TYPE (item->decl)) == METHOD_TYPE)) + return return_false_with_msg ("METHOD_TYPE and FUNCTION_TYPE mismatch"); if (!attribute_list_equal (DECL_ATTRIBUTES (decl), DECL_ATTRIBUTES (item->decl))) return return_false_with_msg ("different decl attributes"); diff --git a/gcc/testsuite/g++.dg/ipa/pr123227.C b/gcc/testsuite/g++.dg/ipa/pr123227.C new file mode 100644 index 000000000000..180792ba2478 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/pr123227.C @@ -0,0 +1,50 @@ +// PR ipa/123227 +// { dg-do run } +// { dg-options "-Os -fipa-icf -fdump-ipa-icf-details" } + +// Link::get_vals and get_vals have identical bodies, but the member +// function may assume a non-null this while the free function must accept +// a null argument, so ICF must not unify them. + +enum Val { zero = 0 }; + +inline Val& +operator|=(Val& a, Val b) +{ + return a = static_cast<Val>(static_cast<int>(a) | static_cast<int>(b)); +} + +struct Link { + Val get_vals(); + Val val; + Link* next; +}; + +Val +Link::get_vals() +{ + Val v = zero; + for (Link* l = this; l; l = l->next) + v |= l->val; + return v; +} + +Val +get_vals(Link* l) +{ + Val v = zero; + for (; l; l = l->next) + v |= l->val; + return v; +} + +int +main(int, char**) +{ + if (get_vals(0) != zero) + __builtin_abort(); + return 0; +} + +// { dg-final { scan-ipa-dump-not "Unified" "icf" } } +// { dg-final { scan-ipa-dump "Equal symbols: 0" "icf" } } diff --git a/gcc/testsuite/g++.dg/lto/pr123227_0.C b/gcc/testsuite/g++.dg/lto/pr123227_0.C new file mode 100644 index 000000000000..2072c5981025 --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr123227_0.C @@ -0,0 +1,52 @@ +// PR ipa/123227 +// { dg-lto-do run } +// { dg-lto-options {{-Os -flto -fipa-icf -fdump-ipa-icf-details}} } + +// Link::get_vals and get_vals have identical bodies, but the member +// function may assume a non-null this while the free function must accept +// a null argument, so ICF must not unify them at WPA. Both are called so +// that neither is removed before ICF runs, and both are noinline so that +// the calls survive as calls. + +enum Val { zero = 0 }; + +inline Val& +operator|=(Val& a, Val b) +{ + return a = static_cast<Val>(static_cast<int>(a) | static_cast<int>(b)); +} + +struct Link { + Val get_vals(); + Val val; + Link* next; +}; + +Val __attribute__((noinline)) +Link::get_vals() +{ + Val v = zero; + for (Link* l = this; l; l = l->next) + v |= l->val; + return v; +} + +extern Val get_vals(Link* l); + +static Link one = { Val(5), 0 }; + +// Volatile so that the null argument is not propagated into get_vals. +Link *volatile nullp = 0; + +int +main() +{ + if (one.get_vals() != Val(5)) + __builtin_abort(); + if (get_vals(nullp) != zero) + __builtin_abort(); + return 0; +} + +// { dg-final { scan-wpa-ipa-dump-not "Unified" "icf" } } +// { dg-final { scan-wpa-ipa-dump "Equal symbols: 0" "icf" } } diff --git a/gcc/testsuite/g++.dg/lto/pr123227_1.C b/gcc/testsuite/g++.dg/lto/pr123227_1.C new file mode 100644 index 000000000000..bc2a7aa168dc --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr123227_1.C @@ -0,0 +1,24 @@ +// PR ipa/123227 + +enum Val { zero = 0 }; + +inline Val& +operator|=(Val& a, Val b) +{ + return a = static_cast<Val>(static_cast<int>(a) | static_cast<int>(b)); +} + +struct Link { + Val get_vals(); + Val val; + Link* next; +}; + +Val __attribute__((noinline)) +get_vals(Link* l) +{ + Val v = zero; + for (; l; l = l->next) + v |= l->val; + return v; +} diff --git a/gcc/testsuite/gcc.dg/ipa/pr123227.c b/gcc/testsuite/gcc.dg/ipa/pr123227.c new file mode 100644 index 000000000000..e19423508195 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr123227.c @@ -0,0 +1,45 @@ +/* PR ipa/123227 */ +/* { dg-do run } */ +/* { dg-options "-Os -fipa-icf -fdump-ipa-icf-details" } */ + +/* The two functions have identical bodies but only one promises a non-null + argument, so ICF must not unify them: the surviving body is allowed to + drop the null test that the other one needs. */ + +typedef struct Link Link; +struct Link +{ + int val; + Link *next; +}; + +int get_vals_nonnull (Link *) __attribute__((nonnull (1))); + +int +get_vals_nonnull (Link *l) +{ + int v = 0; + for (; l; l = l->next) + v |= l->val; + return v; +} + +int +get_vals (Link *l) +{ + int v = 0; + for (; l; l = l->next) + v |= l->val; + return v; +} + +int +main (void) +{ + if (get_vals ((Link *) 0) != 0) + __builtin_abort (); + return 0; +} + +/* { dg-final { scan-ipa-dump-not "Unified" "icf" } } */ +/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf" } } */ diff --git a/gcc/testsuite/gcc.dg/lto/pr123227_0.c b/gcc/testsuite/gcc.dg/lto/pr123227_0.c new file mode 100644 index 000000000000..60f2d7303053 --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/pr123227_0.c @@ -0,0 +1,47 @@ +/* PR ipa/123227 */ +/* { dg-lto-do run } */ +/* { dg-lto-options {{-Os -flto -fipa-icf -fdump-ipa-icf-details}} } */ + +/* get_vals_nonnull and get_vals have identical bodies but only one + promises a non-null argument, so ICF must not unify them at WPA: the + surviving body is allowed to drop the null test the other one needs. + Both are called so that neither is removed before ICF runs, and both + are noinline so that the calls survive as calls. */ + +typedef struct Link Link; +struct Link +{ + int val; + Link *next; +}; + +int get_vals_nonnull (Link *) __attribute__((nonnull (1))); + +int __attribute__((noinline)) +get_vals_nonnull (Link *l) +{ + int v = 0; + for (; l; l = l->next) + v |= l->val; + return v; +} + +extern int get_vals (Link *); + +static Link one = { 5, 0 }; + +/* Volatile so that the null argument is not propagated into get_vals. */ +Link *volatile nullp = 0; + +int +main (void) +{ + if (get_vals_nonnull (&one) != 5) + __builtin_abort (); + if (get_vals (nullp) != 0) + __builtin_abort (); + return 0; +} + +/* { dg-final { scan-wpa-ipa-dump-not "Unified" "icf" } } */ +/* { dg-final { scan-wpa-ipa-dump "Equal symbols: 0" "icf" } } */ diff --git a/gcc/testsuite/gcc.dg/lto/pr123227_1.c b/gcc/testsuite/gcc.dg/lto/pr123227_1.c new file mode 100644 index 000000000000..677f59a918cc --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/pr123227_1.c @@ -0,0 +1,17 @@ +/* PR ipa/123227 */ + +typedef struct Link Link; +struct Link +{ + int val; + Link *next; +}; + +int __attribute__((noinline)) +get_vals (Link *l) +{ + int v = 0; + for (; l; l = l->next) + v |= l->val; + return v; +}
