c++: Don't mix timevar_start and auto_cond_timevar for TV_NAME_LOOKUP [PR116681]

We currently ICE upon the following testcase when using -ftime-report

=== cut here ===
template < int> using __conditional_t = int;
template < typename _Iter >
concept random_access_iterator = requires { new _Iter; };
template < typename _Iterator >
struct reverse_iterator {
  using iterator_concept =
    __conditional_t< random_access_iterator< _Iterator>>;
};
void RemoveBottom() {
  int iter;
  for (reverse_iterator< int > iter;;)
      ;
}
=== cut here ===

The problem is that qualified_namespace_lookup does a plain start() of
the TV_NAME_LOOKUP timer (that asserts that the timer is not already
started). However this timer has already been cond_start()'d in the call
stack - by pushdecl - so the assert fails.

This patch simply ensures that we always conditionally start this timer
(which is done in all other places that use it).

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/116681

gcc/cp/ChangeLog:

	* name-lookup.cc (qualified_namespace_lookup): Use an
	auto_cond_timer instead of using timevar_start and timevar_stop.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-pr116681.C: New test.

---
 gcc/cp/name-lookup.cc                         |  3 +--
 .../g++.dg/cpp2a/concepts-pr116681.C          | 20 +++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr116681.C

On 9/13/24 1:31 PM, Simon Martin wrote:
> We currently ICE upon the following testcase when using -ftime-report
> 
> === cut here ===
> template < int> using __conditional_t = int;
> template < typename _Iter >
> concept random_access_iterator = requires { new _Iter; };
> template < typename _Iterator >
> struct reverse_iterator {
>    using iterator_concept =
>      __conditional_t< random_access_iterator< _Iterator>>;
> };
> void RemoveBottom() {
>    int iter;
>    for (reverse_iterator< int > iter;;)
>        ;
> }
> === cut here ===
> 
> The problem is that qualified_namespace_lookup does a plain start() of
> the TV_NAME_LOOKUP timer (that asserts that the timer is not already
> started). However this timer has already been cond_start()'d in the call
> stack - by pushdecl - so the assert fails.
> 
> This patch simply ensures that we always conditionally start this timer
> (which is done in all other places that use it).

OK, thanks.

> Successfully tested on x86_64-pc-linux-gnu.
> 
> 	PR c++/116681
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (qualified_namespace_lookup): Use an
> 	auto_cond_timer instead of using timevar_start and timevar_stop.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-pr116681.C: New test.
> 
> ---
>   gcc/cp/name-lookup.cc                         |  3 +--
>   .../g++.dg/cpp2a/concepts-pr116681.C          | 20 +++++++++++++++++++
>   2 files changed, 21 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr116681.C
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index bfe17b7cb2f..c7a693e02d5 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -7421,10 +7421,9 @@ tree lookup_qualified_name (tree t, const char *p, LOOK_want w, bool c)
>   static bool
>   qualified_namespace_lookup (tree scope, name_lookup *lookup)
>   {
> -  timevar_start (TV_NAME_LOOKUP);
> +  auto_cond_timevar tv (TV_NAME_LOOKUP);
>     query_oracle (lookup->name);
>     bool found = lookup->search_qualified (ORIGINAL_NAMESPACE (scope));
> -  timevar_stop (TV_NAME_LOOKUP);
>     return found;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr116681.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr116681.C
> new file mode 100644
> index 00000000000..f1b47797f1e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr116681.C
> @@ -0,0 +1,20 @@
> +// PR c++/116681
> +// { dg-do compile { target c++20 } }
> +// { dg-additional-options "-ftime-report" }
> +// { dg-allow-blank-lines-in-output 1 }
> +// { dg-prune-output "Time variable" }
> +// { dg-prune-output "k" }
> +// { dg-prune-output "\[0-9\]+%" }
> +
> +template < int> using __conditional_t = int;
> +template < typename _Iter >
> +concept random_access_iterator = requires { new _Iter; };
> +template < typename _Iterator > struct reverse_iterator {
> +  using iterator_concept = __conditional_t< random_access_iterator< _Iterator >>;
> +};
> +void RemoveBottom()
> +{
> +  int iter;
> +  for (reverse_iterator< int > iter;;)
> +      ;
> +}

Hi,

On Sat Sep 14, 2024 at 10:00 AM CEST, Jason Merrill wrote:
> On 9/13/24 1:31 PM, Simon Martin wrote:
>> We currently ICE upon the following testcase when using -ftime-report
>> 
>> === cut here ===
>> template < int> using __conditional_t = int;
>> template < typename _Iter >
>> concept random_access_iterator = requires { new _Iter; };
>> template < typename _Iterator >
>> struct reverse_iterator {
>>    using iterator_concept =
>>      __conditional_t< random_access_iterator< _Iterator>>;
>> };
>> void RemoveBottom() {
>>    int iter;
>>    for (reverse_iterator< int > iter;;)
>>        ;
>> }
>> === cut here ===
>> 
>> The problem is that qualified_namespace_lookup does a plain start() of
>> the TV_NAME_LOOKUP timer (that asserts that the timer is not already
>> started). However this timer has already been cond_start()'d in the call
>> stack - by pushdecl - so the assert fails.
>> 
>> This patch simply ensures that we always conditionally start this timer
>> (which is done in all other places that use it).
>
> OK, thanks.
Richard B suggested in the ticket to try to backport since that bug can
get in the way of some investigations.

I have checked and the patch applies cleanly on GCC 13 and 14, and tests
are successful for both on x86_64-pc-linux-gnu.

OK for backporting there?

Thanks, Simon

On 3/23/25 1:55 AM, Simon Martin wrote:
> Hi,
> 
> On Sat Sep 14, 2024 at 10:00 AM CEST, Jason Merrill wrote:
>> On 9/13/24 1:31 PM, Simon Martin wrote:
>>> We currently ICE upon the following testcase when using -ftime-report
>>>
>>> === cut here ===
>>> template < int> using __conditional_t = int;
>>> template < typename _Iter >
>>> concept random_access_iterator = requires { new _Iter; };
>>> template < typename _Iterator >
>>> struct reverse_iterator {
>>>     using iterator_concept =
>>>       __conditional_t< random_access_iterator< _Iterator>>;
>>> };
>>> void RemoveBottom() {
>>>     int iter;
>>>     for (reverse_iterator< int > iter;;)
>>>         ;
>>> }
>>> === cut here ===
>>>
>>> The problem is that qualified_namespace_lookup does a plain start() of
>>> the TV_NAME_LOOKUP timer (that asserts that the timer is not already
>>> started). However this timer has already been cond_start()'d in the call
>>> stack - by pushdecl - so the assert fails.
>>>
>>> This patch simply ensures that we always conditionally start this timer
>>> (which is done in all other places that use it).
>>
>> OK, thanks.
> Richard B suggested in the ticket to try to backport since that bug can
> get in the way of some investigations.
> 
> I have checked and the patch applies cleanly on GCC 13 and 14, and tests
> are successful for both on x86_64-pc-linux-gnu.
> 
> OK for backporting there?

OK.

Hi,

On Sun Mar 23, 2025 at 8:30 PM CET, Jason Merrill wrote:
> On 3/23/25 1:55 AM, Simon Martin wrote:
>> Hi,
>> 
>> On Sat Sep 14, 2024 at 10:00 AM CEST, Jason Merrill wrote:
>>> On 9/13/24 1:31 PM, Simon Martin wrote:
>>>> We currently ICE upon the following testcase when using -ftime-report
>>>>
>>>> === cut here ===
>>>> template < int> using __conditional_t = int;
>>>> template < typename _Iter >
>>>> concept random_access_iterator = requires { new _Iter; };
>>>> template < typename _Iterator >
>>>> struct reverse_iterator {
>>>>     using iterator_concept =
>>>>       __conditional_t< random_access_iterator< _Iterator>>;
>>>> };
>>>> void RemoveBottom() {
>>>>     int iter;
>>>>     for (reverse_iterator< int > iter;;)
>>>>         ;
>>>> }
>>>> === cut here ===
>>>>
>>>> The problem is that qualified_namespace_lookup does a plain start() of
>>>> the TV_NAME_LOOKUP timer (that asserts that the timer is not already
>>>> started). However this timer has already been cond_start()'d in the call
>>>> stack - by pushdecl - so the assert fails.
>>>>
>>>> This patch simply ensures that we always conditionally start this timer
>>>> (which is done in all other places that use it).
>>>
>>> OK, thanks.
>> Richard B suggested in the ticket to try to backport since that bug can
>> get in the way of some investigations.
>> 
>> I have checked and the patch applies cleanly on GCC 13 and 14, and tests
>> are successful for both on x86_64-pc-linux-gnu.
>> 
>> OK for backporting there?
>
> OK.
Thanks. Merged as r13-9448-g73db20707fd60f303313fda3c9245a06037f312a and
r14-11442-g8bac22817192349cad15c88e01a73798e2a783ad.

Simon