| Message ID | 20240408141815.127984-3-j@lambda.is |
|---|---|
| State | New |
| Headers | show |
| Series | More condition coverage fixes | expand |
On Mon, 8 Apr 2024, Jørgen Kvalsvik wrote: > Generating the constants used for recording the edges taken for > condition coverage would trigger undefined behavior when an expression > had exactly 64 (== sizeof (1ULL)) conditions, as it would generate the > constant for the next iteration at the end of the loop body, even if there > was never a next iteration. By moving the check and constant generation > to the top of the loop and hoisting the increment flag there is no > opportunity for UB. OK. Thanks, Richard. > PR 114627 > > gcc/ChangeLog: > > * tree-profile.cc (instrument_decisions): Generate constant > at the start of loop. > --- > gcc/tree-profile.cc | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc > index 33ff550a7bc..e58f5c83472 100644 > --- a/gcc/tree-profile.cc > +++ b/gcc/tree-profile.cc > @@ -1049,6 +1049,7 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, > zerocounter[2] = zero; > > unsigned xi = 0; > + bool increment = false; > tree rhs = build_int_cst (gcov_type_node, 1ULL << xi); > for (basic_block current : expr) > { > @@ -1057,7 +1058,14 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, > candidates.safe_push (zerocounter); > counters prev = resolve_counters (candidates); > > - int increment = 0; > + if (increment) > + { > + xi += 1; > + gcc_checking_assert (xi < sizeof (uint64_t) * BITS_PER_UNIT); > + rhs = build_int_cst (gcov_type_node, 1ULL << xi); > + increment = false; > + } > + > for (edge e : current->succs) > { > counters next = prev; > @@ -1072,7 +1080,7 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, > tree m = build_int_cst (gcov_type_node, masks[2*xi + k]); > next[2] = emit_bitwise_op (e, prev[2], BIT_IOR_EXPR, m); > } > - increment = 1; > + increment = true; > } > else if (e->flags & EDGE_COMPLEX) > { > @@ -1085,11 +1093,13 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, > } > table.get_or_insert (e->dest).safe_push (next); > } > - xi += increment; > - if (increment) > - rhs = build_int_cst (gcov_type_node, 1ULL << xi); > } > > + /* Since this is also the return value, the number of conditions, make sure > + to include the increment of the last basic block. */ > + if (increment) > + xi += 1; > + > gcc_assert (xi == bitmap_count_bits (core)); > > const tree relaxed = build_int_cst (integer_type_node, MEMMODEL_RELAXED); >
On 09/04/2024 09:40, Richard Biener wrote: > On Mon, 8 Apr 2024, Jørgen Kvalsvik wrote: > >> Generating the constants used for recording the edges taken for >> condition coverage would trigger undefined behavior when an expression >> had exactly 64 (== sizeof (1ULL)) conditions, as it would generate the >> constant for the next iteration at the end of the loop body, even if there >> was never a next iteration. By moving the check and constant generation >> to the top of the loop and hoisting the increment flag there is no >> opportunity for UB. > > OK. > > Thanks, > Richard. Thanks, committed. > >> PR 114627 >> >> gcc/ChangeLog: >> >> * tree-profile.cc (instrument_decisions): Generate constant >> at the start of loop. >> --- >> gcc/tree-profile.cc | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc >> index 33ff550a7bc..e58f5c83472 100644 >> --- a/gcc/tree-profile.cc >> +++ b/gcc/tree-profile.cc >> @@ -1049,6 +1049,7 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, >> zerocounter[2] = zero; >> >> unsigned xi = 0; >> + bool increment = false; >> tree rhs = build_int_cst (gcov_type_node, 1ULL << xi); >> for (basic_block current : expr) >> { >> @@ -1057,7 +1058,14 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, >> candidates.safe_push (zerocounter); >> counters prev = resolve_counters (candidates); >> >> - int increment = 0; >> + if (increment) >> + { >> + xi += 1; >> + gcc_checking_assert (xi < sizeof (uint64_t) * BITS_PER_UNIT); >> + rhs = build_int_cst (gcov_type_node, 1ULL << xi); >> + increment = false; >> + } >> + >> for (edge e : current->succs) >> { >> counters next = prev; >> @@ -1072,7 +1080,7 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, >> tree m = build_int_cst (gcov_type_node, masks[2*xi + k]); >> next[2] = emit_bitwise_op (e, prev[2], BIT_IOR_EXPR, m); >> } >> - increment = 1; >> + increment = true; >> } >> else if (e->flags & EDGE_COMPLEX) >> { >> @@ -1085,11 +1093,13 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, >> } >> table.get_or_insert (e->dest).safe_push (next); >> } >> - xi += increment; >> - if (increment) >> - rhs = build_int_cst (gcov_type_node, 1ULL << xi); >> } >> >> + /* Since this is also the return value, the number of conditions, make sure >> + to include the increment of the last basic block. */ >> + if (increment) >> + xi += 1; >> + >> gcc_assert (xi == bitmap_count_bits (core)); >> >> const tree relaxed = build_int_cst (integer_type_node, MEMMODEL_RELAXED); >> >
diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc index 33ff550a7bc..e58f5c83472 100644 --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -1049,6 +1049,7 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, zerocounter[2] = zero; unsigned xi = 0; + bool increment = false; tree rhs = build_int_cst (gcov_type_node, 1ULL << xi); for (basic_block current : expr) { @@ -1057,7 +1058,14 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, candidates.safe_push (zerocounter); counters prev = resolve_counters (candidates); - int increment = 0; + if (increment) + { + xi += 1; + gcc_checking_assert (xi < sizeof (uint64_t) * BITS_PER_UNIT); + rhs = build_int_cst (gcov_type_node, 1ULL << xi); + increment = false; + } + for (edge e : current->succs) { counters next = prev; @@ -1072,7 +1080,7 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, tree m = build_int_cst (gcov_type_node, masks[2*xi + k]); next[2] = emit_bitwise_op (e, prev[2], BIT_IOR_EXPR, m); } - increment = 1; + increment = true; } else if (e->flags & EDGE_COMPLEX) { @@ -1085,11 +1093,13 @@ instrument_decisions (array_slice<basic_block> expr, size_t condno, } table.get_or_insert (e->dest).safe_push (next); } - xi += increment; - if (increment) - rhs = build_int_cst (gcov_type_node, 1ULL << xi); } + /* Since this is also the return value, the number of conditions, make sure + to include the increment of the last basic block. */ + if (increment) + xi += 1; + gcc_assert (xi == bitmap_count_bits (core)); const tree relaxed = build_int_cst (integer_type_node, MEMMODEL_RELAXED);