func_checker::compare_operand compares non-memory operands with a bare
operand_equal_p and OEP_MATCH_SIDE_EFFECTS. When the operand is an
ADDR_EXPR, operand_equal_p recurses with OEP_ADDRESS_OF set, and for a
COMPONENT_REF under OEP_ADDRESS_OF it accepts two different FIELD_DECLs
whose DECL_FIELD_OFFSET and DECL_FIELD_BIT_OFFSET agree. That is correct
for the value of the address, but ICF is not only comparing an address.
The surviving body carries one of the two field types into every caller,
and get_range_strlen_tree reads TYPE_DOMAIN and TYPE_MAX_VALUE off it.
In the PR two structs hold a char array as their first member, one of 11
and one of 128 elements, with identical operator== bodies. ICF unifies
them, the surviving body carries char[11], and the strlen machinery then
concludes that a 15 character string cannot be there.
Pass OEP_ADDRESS_OF_SAME_FIELD, which tree-core.h already documents for
this case: "For OEP_ADDRESS_OF of COMPONENT_REFs, only consider same
fields as equivalent rather than also different fields with the same
offset." fold-const.cc honours it at the point where the two offsets
would otherwise be accepted.
PR ipa/119006
gcc/ChangeLog:
* ipa-icf-gimple.cc (func_checker::compare_operand): Pass
OEP_ADDRESS_OF_SAME_FIELD to operand_equal_p.
gcc/testsuite/ChangeLog:
* g++.dg/ipa/pr119006.C: New test.
Signed-off-by: Rohith Kapelli <[email protected]>
---
PR119006 is wrong code, filed 2025-02-25 and live on gcc-13, gcc-14,
gcc-15, gcc-16 and trunk. Two structs hold a char array as their first
member, one char[11] and one char[128], with identical operator== bodies.
ICF unifies the two operator==, the surviving body carries char[11], and
get_range_strlen_tree reads TYPE_DOMAIN off that type and concludes that a
15 character string cannot be there. The comparison folds to a constant and
the program takes the wrong branch.
The mechanism is in func_checker::compare_operand. Non-memory operands go
through a bare operand_equal_p with OEP_MATCH_SIDE_EFFECTS. For an
ADDR_EXPR operand_equal_p recurses with OEP_ADDRESS_OF set, and for a
COMPONENT_REF under OEP_ADDRESS_OF fold-const.cc accepts two different
FIELD_DECLs when DECL_FIELD_OFFSET and DECL_FIELD_BIT_OFFSET agree. Both
_str members sit at offset 0, so the fields compare equal and the functions
are unified.
Comment 10 asks for a flag that would check the MEM_REF details even under
OEP_ADDRESS_OF. The divergence here is one step before that: field identity
on the COMPONENT_REF at fold-const.cc:3599, not a MEM_REF detail.
GCC already documents a flag for that fallback, on every branch this patch
targets and gcc-13 included. tree-core.h:
/* For OEP_ADDRESS_OF of COMPONENT_REFs, only consider same fields as
equivalent rather than also different fields with the same offset. */
OEP_ADDRESS_OF_SAME_FIELD = 256,
It has one consumer, fold-const.cc:3599, and three callers, all of them in
front ends: cp/call.cc, c-family/c-warn.cc and c/c-typeck.cc, none in the
middle end. The patch passes it from compare_operand.
Comment 12 asks how often this fires, and notes that merging semantically
equivalent functions from different template instantiations is an important
case of ICF, over 20 percent of LibreOffice code size.
Here is that measurement, and the patch is not free. On
libstdc++-v3/src/c++{98,11,17,20}/*.cc compiled to objects and linked as
one shared library with LTO, 105 objects, it declines 25 merges at -O2 and
29 at -Os while gaining 3 at each level, so 25 of 255 and 29 of 270. Read
off the assembler names in the WPA dump they are almost entirely char
against wchar_t instantiations of stream and locale accessors, rdbuf 9,
is_open 7, do_is_equal 4, operator= 2, fd and file 2, one transactional
error message getter, and one pair from two different class templates
sharing an accessor body. That is the cross-instantiation class comment 12
calls important, and what it costs is 264 bytes of .text at -O2 and 2188 at
-Os, against 1710731 and 1365540 bytes unpatched, so 0.015 and 0.16
percent.
An instrumented compiler that logs this divergence emitted 0 lines over the
same workload at both levels, 525 merges. Its sensitivity was established
first on four constructed divergence shapes and it fires on all four. That
zero is the rate at which this bug fires on this workload, not a claim that
ICF never merges across divergent metadata.
The workload is one library, and libstdc++'s own sources are library
implementation rather than application-scale template instantiation, so
this speaks to comment 12's reference point without reaching it. No
LibreOffice-scale C++ body was available here to measure.
Comment 12 offers two routes, giving up the merge or modifying the merged
body into a form safe for both. This patch takes the first. I prototyped
the second, rewriting the diverging ADDR_EXPR to a MEM_REF and retyping the
SSA name: it fixes all 40 configurations while keeping the merge, recovers
trunk's 255 and 270 merges exactly, and bootstraps, but it fires zero times
across those 525 merges, so every aggregate result validated it dormant and
the finding rests on one instance. Feasibility, not a proposal.
Testing, on aarch64-unknown-linux-gnu at trunk 95c85b404da, patched and
unpatched from the same tree in the same environment with only
ipa-icf-gimple.cc differing. Three stage bootstrap configured
--enable-languages=c,c++,lto --enable-checking=yes,rtl,extra
--disable-multilib --disable-nls --disable-libsanitizer, make compare
reported comparison successful. Shard sums merged with
contrib/dg-extract-results.sh:
tool variant total PASS FAIL UNRESOLVED
gcc trunk 408405 401230 71 0
gcc patch 408405 401230 71 0
g++ trunk 474714 468765 52 0
g++ patch 474777 468828 52 0
The gcc suite is identical line for line. The g++ totals differ by 63 test
names, which is g++.dg/modules churn that appears between unpatched runs as
well; no FAIL and no UNRESOLVED is added or removed.
Runtime libraries, unpatched against patched, comparing sorted PASS, FAIL,
XPASS and UNRESOLVED lines: libstdc++ 18914 PASS and 1 FAIL, libgomp 6273,
libitm 44, libatomic 54, and 0 differing lines in all four. The libstdc++
FAIL is experimental/net/internet/resolver/ops/lookup.cc, which needs DNS
the test machine does not have, and is present unpatched.
A 40 configuration reproducer matrix, 2 testcases by 5 optimisation levels
by 4 flag sets, goes from 6 failing to 0. The same matrix on the release
branches with the patch applied to each:
gcc-16 bf726b75da7 6 failing before, 0 after
gcc-15 ae79106ea90 6 failing before, 0 after
gcc-14 8c5c1a3eca2 6 failing before, 0 after
gcc-13 e1926c31dcb 6 failing before, 0 after
On gcc-13 the hunk applies at offset -1 lines.
The testcase is revert sensitive. One build tree, patch applied, then
reverted in place with only the compiler rebuilt, then reapplied:
applied 12 PASS, 0 FAIL
reverted 3 PASS, 9 FAIL
reapplied 12 PASS, 0 FAIL
The 3 that pass unpatched are the excess errors lines, one per variant.
Variants exercised are gnu++11, gnu++20 and gnu++29.
On the choice: Jakub's comment 11 patch 1 also fixes this, by intercepting
ADDR_EXPR and comparing the operands without OEP_ADDRESS_OF, which restores
the same field rule and the MEM_REF checks too. On libstdc++ it declines
the identical set and produces the same .text, and I could not separate the
two on evidence: across three constructed shapes at four option
combinations every alignment sensitive pair was rejected by compare_ao_refs
before the blind operand_equal_p was reached, and three shapes is not a
proof. I bootstrapped and regression tested his patch here too, with the
same results, and the testcase is revert sensitive against it in the same
way. If it is preferred, it is his and it is ready, the testing above
applies to it unchanged, and I have no argument against it.
Where would this fire at scale, since libstdc++ gives zero?
Must such a rewrite work at WPA, given 8a917760aa4 takes a !flag_wpa exit?
Is retyping an SSA name in place acceptable, or must consumers be checked?
OK for trunk?
gcc/ipa-icf-gimple.cc | 3 ++-
gcc/testsuite/g++.dg/ipa/pr119006.C | 33 +++++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/ipa/pr119006.C
diff --git a/gcc/ipa-icf-gimple.cc b/gcc/ipa-icf-gimple.cc
index a893a3faaa4..720a3a0ce1a 100644
--- a/gcc/ipa-icf-gimple.cc
+++ b/gcc/ipa-icf-gimple.cc
@@ -439,7 +439,8 @@ func_checker::compare_operand (tree t1, tree t2,
operand_access_type access)
}
else
{
- if (operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS))
+ if (operand_equal_p (t1, t2, OEP_MATCH_SIDE_EFFECTS
+ | OEP_ADDRESS_OF_SAME_FIELD))
return true;
return return_false_with_msg
("operand_equal_p failed");
diff --git a/gcc/testsuite/g++.dg/ipa/pr119006.C
b/gcc/testsuite/g++.dg/ipa/pr119006.C
new file mode 100644
index 00000000000..aa975eb3004
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr119006.C
@@ -0,0 +1,33 @@
+// PR ipa/119006
+// { dg-do run { target c++11 } }
+// { dg-options "-O2 -fwhole-program -fdump-ipa-icf-details" }
+
+struct FixedString10 {
+ bool operator==(const char* rhs_) const { return rhs_ and not
__builtin_strcmp(_str, rhs_); }
+ char _str[11];
+};
+struct FixedString127 {
+ 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[128];
+};
+[[gnu::noinline,gnu::used]]
+int fixedStringUser(const FixedString10& lhs, const char* rhs) {
+ return lhs == rhs;
+}
+constexpr const char *t = "StrOverTenChars";
+[[gnu::noinline,gnu::used]]
+void checkString(FixedString127 reason_) {
+ if (reason_ != t) __builtin_trap();
+}
+
+int main(int argc, char** argv) {
+ FixedString127 fs;
+ __builtin_strcpy(fs._str, t);
+ checkString(fs);
+}
+
+// The two operator== bodies are identical but their _str members have
+// different bounds, so ICF must not treat them as equal.
+// { dg-final { scan-ipa-dump-not "Unified" "icf" } }
+// { dg-final { scan-ipa-dump "Equal symbols: 0" "icf" } }
--
2.53.0
