Re: [PATCH RFC] c++: lambda mangling alias issues [PR107897]

[Jason Merrill <jason@redhat.com>]

On 3/8/23 10:53, Jan Hubicka wrote:
>> Tested x86_64-pc-linux-gnu.  Does this look good, or do we want to factor the
>> flag clearing into a symtab_node counterpart to cgraph_node::reset?
>>
>> -- 8< --
>>
>> In 107897, by the time we are looking at the mangling clash, the
>> alias has already been removed from the symbol table by analyze_functions,
>> so we can't look at n->cpp_implicit_alias.  So just assume that it's an
>> alias if it's internal.
>>
>> In 108887 the problem is that removing the mangling alias from the symbol
>> table confuses analyze_functions, because it ended up as first_analyzed
>> somehow, so it becomes a dangling pointer.  Fixed by clearing various flags
>> to neutralize the alias.
>>
>> 	PR c++/107897
>> 	PR c++/108887
>>
>> gcc/cp/ChangeLog:
>>
>> 	* decl2.cc (record_mangling): Improve symbol table handling.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/cpp2a/concepts-lambda3.C: Use -flto if supported.
>> 	* g++.dg/cpp0x/lambda/lambda-mangle7.C: New test.
>> ---
>>   gcc/cp/decl2.cc                               | 25 +++++--
>>   .../g++.dg/cpp0x/lambda/lambda-mangle7.C      | 70 +++++++++++++++++++
>>   gcc/testsuite/g++.dg/cpp2a/concepts-lambda3.C |  1 +
>>   3 files changed, 91 insertions(+), 5 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/lambda/lambda-mangle7.C
>>
>> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
>> index 387e24542cd..e6e58b08de4 100644
>> --- a/gcc/cp/decl2.cc
>> +++ b/gcc/cp/decl2.cc
>> @@ -4742,15 +4742,30 @@ record_mangling (tree decl, bool need_warning)
>>       = mangled_decls->find_slot_with_hash (id, IDENTIFIER_HASH_VALUE (id),
>>   					  INSERT);
>>   
>> -  /* If this is already an alias, remove the alias, because the real
>> +  /* If this is already an alias, cancel the alias, because the real
>>        decl takes precedence.  */
>>     if (*slot && DECL_ARTIFICIAL (*slot) && DECL_IGNORED_P (*slot))
>> -    if (symtab_node *n = symtab_node::get (*slot))
>> -      if (n->cpp_implicit_alias)
>> +    {
>> +      if (symtab_node *n = symtab_node::get (*slot))
>>   	{
>> -	  n->remove ();
>> -	  *slot = NULL_TREE;
>> +	  if (n->cpp_implicit_alias)
>> +	    {
>> +	      /* Actually removing the node isn't safe if other code is already
>> +		 holding a pointer to it, so just neutralize it.  */
>> +	      n->remove_from_same_comdat_group ();
>> +	      n->analyzed = false;
>> +	      n->definition = false;
>> +	      n->alias = false;
>> +	      n->cpp_implicit_alias = false;
> We have n->reset () for that which is used in similar situation when
> frontends overwrites extern inline function by its different offline
> implementation.

The problem there is that reset() is a member of cgraph_node, not 
symtab_node, and I need something that works for variables as well.

> reset doesn't call remove_from_same_comdat_group probably because it was
> never used on anything in comdat group.  So I think it would make sense
> to call n->reset() here and add remove_from_same_comdat_group into that.
> 
> OK with that change.
> Honza
>> +	    }
>>   	}
>> +      else
>> +	/* analyze_functions might have already removed the alias from the
>> +	   symbol table if it's internal.  */
>> +	gcc_checking_assert (!TREE_PUBLIC (*slot));
>> +
>> +      *slot = NULL_TREE;
>> +    }
>>   
>>     if (!*slot)
>>       *slot = decl;
>> diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-mangle7.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-mangle7.C
>> new file mode 100644
>> index 00000000000..c7946a2be08
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-mangle7.C
>> @@ -0,0 +1,70 @@
>> +// PR c++/108887
>> +// { dg-do compile { target c++11 } }
>> +
>> +template <int __v> struct integral_constant {
>> +  static constexpr int value = __v;
>> +};
>> +using false_type = integral_constant<false>;
>> +template <bool, bool, typename...> struct __result_of_impl;
>> +template <typename _Functor, typename... _ArgTypes>
>> +struct __result_of_impl<false, false, _Functor, _ArgTypes...> {
>> +  typedef decltype(0) type;
>> +};
>> +template <typename... _ArgTypes>
>> +struct __invoke_result
>> +    : __result_of_impl<false_type::value, false_type::value, _ArgTypes...> {};
>> +template <typename, typename _Fn, typename... _Args>
>> +void __invoke_impl(_Fn __f, _Args... __args) {
>> +  __f(__args...);
>> +}
>> +template <typename, typename _Callable, typename... _Args>
>> +void __invoke_r(_Callable __fn, _Args... __args) {
>> +  using __result = __invoke_result<_Args...>;
>> +  using __type = typename __result::type;
>> +  __invoke_impl<__type>(__fn, __args...);
>> +}
>> +struct QString {
>> +  QString(const char *);
>> +};
>> +template <typename> class function;
>> +template <typename _Functor> struct _Base_manager {
>> +  static _Functor _M_get_pointer(int) { __builtin_abort (); }
>> +};
>> +template <typename, typename> class _Function_handler;
>> +template <typename _Res, typename _Functor, typename... _ArgTypes>
>> +struct _Function_handler<_Res(_ArgTypes...), _Functor> {
>> +  using _Base = _Base_manager<_Functor>;
>> +  static _Res _M_invoke(const int &__functor, _ArgTypes &&...__args) {
>> +    auto __trans_tmp_1 = _Base::_M_get_pointer(__functor);
>> +    __invoke_r<_Res>(__trans_tmp_1, __args...);
>> +  }
>> +};
>> +template <typename _Res, typename... _ArgTypes>
>> +struct function<_Res(_ArgTypes...)> {
>> +  template <typename _Functor>
>> +  using _Handler = _Function_handler<_Res(_ArgTypes...), _Functor>;
>> +  template <typename _Functor> function(_Functor) {
>> +    using _My_handler = _Handler<_Functor>;
>> +    _M_invoker = _My_handler::_M_invoke;
>> +  }
>> +  using _Invoker_type = _Res (*)(const int &, _ArgTypes &&...);
>> +  _Invoker_type _M_invoker;
>> +};
>> +struct QRegularExpression {
>> +  QRegularExpression(QString);
>> +};
>> +struct AbstractAccount {
>> +  void get(function<void(AbstractAccount *)>,
>> +           function<void(AbstractAccount *)>);
>> +};
>> +struct AbstractTimelineModel {
>> +  AbstractAccount m_account;
>> +};
>> +struct LinkPaginationTimelineModel : AbstractTimelineModel {
>> +  void fillTimeline();
>> +};
>> +void LinkPaginationTimelineModel::fillTimeline() {
>> +  [] {};
>> +  m_account.get([](AbstractAccount *) { static QRegularExpression re(""); },
>> +                [](AbstractAccount *) {});
>> +}
>> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-lambda3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-lambda3.C
>> index 291e451ca1a..a7cb6c5dece 100644
>> --- a/gcc/testsuite/g++.dg/cpp2a/concepts-lambda3.C
>> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-lambda3.C
>> @@ -1,4 +1,5 @@
>>   // { dg-do run { target c++20 } }
>> +// { dg-additional-options "-flto" { target lto } } (PR107897)
>>   
>>   template<typename T>
>>   concept C1 = __is_same_as(T, int)
>>
>> base-commit: 4f181f9c7ee3efc509d185fdfda33be9018f1611
>> -- 
>> 2.31.1
>>
>