[2/2] Generate constant at start of loop, without UB

  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.

	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(-)
  

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);
>>
>