When compiled with -O2 -flto (or more minimally: -O -finline -fipa-icf-functions -foptimize-strlen -ftree-dse -ftree-vrp -flto), GCC miscompiles the following code: template<unsigned N> struct FixedString { FixedString(const char* str_) { *this = str_; } bool operator==(const char* rhs_) const { return rhs_ and not __builtin_strcmp(_str, rhs_); } bool operator!=(const char* rhs_) const { return !(*this == rhs_); } char _str[N + 1]; }; int fixedStringUser(const FixedString<10>& lhs, const char* rhs) { return lhs == rhs; } void checkString(FixedString<127> reason_) { if (reason_ != "StrOverTenChars") asm volatile("nop"); } int main(int argc, char** argv) { FixedString<127> fs = argv[1]; if (argc > 2) checkString(fs); } During LTO WPA, ICF replaces FixedString<127>::operator== with FixedString<10>::operator==, and then LTRANS reasons that strcmp of a buffer which can hold a string up to length 10 cannot be equal to a string longer than 10 characters, forgetting that the LHS is actually max length 127. This results in `checkString` being reduced to the body of the `if` condition, i.e. just the `asm volatile("nop")`. Testing on godbolt (https://gcc.godbolt.org/z/bv3YqzTG5) shows that this has been present since GCC11, and is still present in trunk.
This is related to a dup of bug 113907 or the other one.
Further simplified: template<unsigned N> struct FixedString { bool operator==(const char* rhs_) const { return rhs_ and not __builtin_strcmp(_str, rhs_); } bool operator!=(const char* rhs_) const { return !(*this == rhs_); } char _str[N + 1]; }; int fixedString10User(const FixedString<10>& lhs, const char* rhs) { return lhs == rhs; } void checkString(FixedString<127> reason_) { if (reason_ != "StrOverTenChars") asm volatile("nop"); } int main() {} I also had to build with `-rdynamic` to repro without `checkString` being omitted entirely. Observed on x86-64.
Your source does not work either: FixedString(const char* str_) { *this = str_; } Is an infinite loop. It should be: FixedString(const char* str_) { __builtin_strcpy (this->_str, str_); }
Created attachment 60579 [details] Runtime testcase Fails with `-O2 -g0 -fwhole-program` but works without -fwhole-program. So no need for LTO.
Created attachment 60580 [details] Further reduced Note I removed the template so it is easier to see FixedString127/FixedString10 name in the dump file.
Confirmed. As I mentioned LTO is not needed with my testcase; only -fwhole-program. Confirmed.
Bisecting.
r11-5181-g0862d007b564ec
struct A { bool operator== (const char *x) const { return x and not __builtin_strcmp (a, x); } char a[11]; }; struct B { bool operator== (const char *x) const { return x and not __builtin_strcmp (a, x); } bool operator!= (const char *x) const { return !(*this == x); } char a[128]; }; [[gnu::noinline,gnu::used]] int foo (const A& lhs, const char* rhs) { return lhs == rhs; } constexpr const char *t = "abcdefghijklmno"; [[gnu::noinline,gnu::used]] void bar (B x) { if (x != t) __builtin_abort (); } int main () { B b; __builtin_strcpy (b.a, t); bar (b); } Seems ICF performs only very limited type comparison for the argument types: sem_function::equals_private does if (!types_compatible_p (TREE_TYPE (arg1), TREE_TYPE (arg2))) return return_false_with_msg ("argument types are not compatible"); but that doesn't check much, in GIMPLE all pointer types are compatible except for TYPE_ADDR_SPACE differences or data vs. function pointer types. And then /* Perform additional checks for used parameters. */ if (!compatible_parm_types_p (TREE_TYPE (arg1), TREE_TYPE (arg2))) return false; but that pretty much repeats those, punts on TREE_CODE differences (pointer vs. reference), TYPE_RESTRICT changes and compatible_types_p just repets what has been checked already. Now, the question is if optimizations derived anything from the type pointer arguments to functions point to. In this testcase, I wonder if it isn't more about _1 = &this_5(D)->a; vs. _1 = &this_5(D)->a; where in one case const struct B * const this and in another case const struct A * const this isn't different, i.e. whether we shouldn't be checking type on COMPONENT_REF. Because during strlen1 it is __builtin_strcmp (&MEM[(const struct B *)&x].a, "abcdefghijklmno") vs. __builtin_strcmp (&MEM[(const struct A *)&x].a, "abcdefghijklmno") that results in the lhs have the [irange] int [-INF, -1][1, +INF] range computed just in the latter case and not the former. Or maybe even better compare it on MEM_REF? The MEM_REF handling does right now: case MEM_REF: { tree x1 = TREE_OPERAND (t1, 0); tree x2 = TREE_OPERAND (t2, 0); tree y1 = TREE_OPERAND (t1, 1); tree y2 = TREE_OPERAND (t2, 1); if (!func_checker::compatible_types_p (TREE_TYPE (x1), TREE_TYPE (x2))) return return_false (); /* Type of the offset on MEM_REF does not matter. */ return return_with_debug (sem_variable::equals (x1, x2) && known_eq (wi::to_poly_offset (y1), wi::to_poly_offset (y2))); } That is really weird and unexpected. The types of x1 and x2 should be pretty much always be compatible (exceptions are restrict/address space I think). But it doesn't compare TREE_TYPE (t1) vs. TREE_TYPE (t2), and for aliasing also the important TREE_TYPE (y1), TREE_TYPE (y2).
--- gcc/ipa-icf.cc.jj 2025-01-02 11:23:19.997468267 +0100 +++ gcc/ipa-icf.cc 2025-02-27 13:48:15.069430554 +0100 @@ -1810,6 +1810,9 @@ sem_variable::equals (tree t1, tree t2) tree y1 = TREE_OPERAND (t1, 1); tree y2 = TREE_OPERAND (t2, 1); + if (!func_checker::compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))) + return return_false (); + if (!func_checker::compatible_types_p (TREE_TYPE (x1), TREE_TYPE (x2))) return return_false (); doesn't fix that though, I guess the MEM_REFs are compared with operand_equal_p instead. And, operand_equal_p normally compares the types of the MEM_REFs and checks alias_ptr_types_compatible_p for the types of the MEM_REF offsets. But doesn't do that in the OEP_ADDRESS_OF case, which is what happens here, _1 = &this_5(D)->a; because everything is in ADDR_EXPR. Such details are irrelevant for the value of the ADDR_EXPR, but get_range_strlen_tree still cares about. So, shall we introduce some new OEP_* flag used for the ICF operand_equal_p that will make it check the MEM_REF details even when OEP_ADDRESS_OF? Or check for that manually in ipa-icf-gimple.cc (compare_operand)? Perhaps all we care about is if toplevel t1 is ADDR_EXPR check if the operand is equal too?
--- gcc/ipa-icf-gimple.cc.jj 2025-02-01 00:50:02.080774328 +0100 +++ gcc/ipa-icf-gimple.cc 2025-02-27 14:13:27.446426589 +0100 @@ -437,12 +437,23 @@ func_checker::compare_operand (tree t1, ("compare_ao_refs failed (dependence clique difference)"); gcc_unreachable (); } + else if (TREE_CODE (t1) == ADDR_EXPR && TREE_CODE (t2) == ADDR_EXPR) + { + /* For ADDR_EXPR compare the operands of the ADDR_EXPR rather than + the ADDR_EXPRs themselves. operand_equal_p will compare the + operands with OEP_ADDRESS_OF and only care about the value + of the ADDR_EXPR, rather than e.g. types of MEM_REFs in there. + Some optimizations use such details though, see PR119006. */ + if (operand_equal_p (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0), + OEP_MATCH_SIDE_EFFECTS)) + return true; + return return_false_with_msg ("operand_equal_p failed"); + } else { if (operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS)) return true; - return return_false_with_msg - ("operand_equal_p failed"); + return return_false_with_msg ("operand_equal_p failed"); } } fixes this. And so does --- gcc/ipa-icf-gimple.cc.jj 2025-02-01 00:50:02.080774328 +0100 +++ gcc/ipa-icf-gimple.cc 2025-02-27 14:24:20.815358621 +0100 @@ -440,9 +440,39 @@ func_checker::compare_operand (tree t1, else { if (operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS)) - return true; - return return_false_with_msg - ("operand_equal_p failed"); + { + if (TREE_CODE (t1) == ADDR_EXPR && TREE_CODE (t2) == ADDR_EXPR) + { + /* For ADDR_EXPR operand_equal_p compares the operands of + it using OEP_ADDRESS_OF and only care about the value + of the ADDR_EXPR, rather than e.g. types of MEM_REFs in + there. Some optimizations use such details though, see + PR119006. */ + tree base1 = TREE_OPERAND (t1, 0); + tree base2 = TREE_OPERAND (t2, 0); + do + { + while (handled_component_p (base1)) + base1 = TREE_OPERAND (base1, 0); + while (handled_component_p (base2)) + base2 = TREE_OPERAND (base2, 0); + if (!compatible_types_p (TREE_TYPE (base1), + TREE_TYPE (base2))) + return return_false_with_msg ("ADDR_EXPR base type " + "difference"); + if (TREE_CODE (base1) != MEM_REF + || TREE_CODE (TREE_OPERAND (base1, 0)) != ADDR_EXPR + || TREE_CODE (base2) != MEM_REF + || TREE_CODE (TREE_OPERAND (base2, 0)) != ADDR_EXPR) + break; + base1 = TREE_OPERAND (TREE_OPERAND (base1, 0), 0); + base2 = TREE_OPERAND (TREE_OPERAND (base2, 0), 0); + } + while (1); + } + return true; + } + return return_false_with_msg ("operand_equal_p failed"); } } Any preference between the two? The first one is certainly safer, on the other side might punt even e.g. on alias set differences etc. which the strlen opts in gimple-fold.cc don't care about.
I think I should measure how often this fires. This is the usual situation where ICF merges something that is semantically equivalent in final code, but carries different metadata used later in optimization. One option is to give up on merging, other option is to modify the merged body into form safe for both cases. I hope to find time to implement infrastructure for this, and we should keep track of those issues and see how often they fires. Merging semantically equivalent functions coming from different template instantiations is important case of ICF (originally accounting for over 20% of code size of LibreOffice if I recall) Just yesterday I was filing up PR119033 which has struct foo { int a[3]; int b[3]; }; and mixes up &foo.a[3] with &foo.b[0] in PHI-opt. I suppose ICF will be happy to do same mistake...
I could easily gather statistics on how many ICF foldings were successful during bootstrap/regtest with/without the patch.
GCC 12 branch is being closed.