Re: [PATCH] c++: suppress all warnings on memper pointers to work around dICE [PR101219]

[Jason Merrill <jason@redhat.com>]

On 8/6/21 11:34 AM, Sergei Trofimovich wrote:
> On Thu, 29 Jul 2021 11:41:39 -0400
> Jason Merrill <jason@redhat.com> wrote:
> 
>> On 7/22/21 7:15 PM, Sergei Trofimovich wrote:
>>> From: Sergei Trofimovich <siarheit@google.com>
>>>
>>> r12-1804 ("cp: add support for per-location warning groups.") among other
>>> things removed warning suppression from a few places including ptrmemfuncs.
>>>
>>> Currently ptrmemfuncs don't have valid BINFO attached which causes ICEs
>>> in access checks:
>>>
>>>       crash_signal
>>>           gcc/toplev.c:328
>>>       perform_or_defer_access_check(tree_node*, tree_node*, tree_node*, int, access_failure_info*)
>>>           gcc/cp/semantics.c:490
>>>       finish_non_static_data_member(tree_node*, tree_node*, tree_node*)
>>>           gcc/cp/semantics.c:2208
>>>       ...
>>>
>>> The change suppresses warnings again until we provide BINFOs for ptrmemfuncs.
>>
>> We don't need BINFOs for PMFs, we need to avoid paths that expect them.
>>
>> It looks like the problem is with tsubst_copy_and_build calling
>> finish_non_static_data_member instead of build_ptrmemfunc_access_expr.
> 
> Sounds good. I'm not sure what would be the best way to match it. Here is
> my attempt seems to survive all regtests:
> 
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -20530,7 +20530,13 @@ tsubst_copy_and_build (tree t,
>          if (member == error_mark_node)
>            RETURN (error_mark_node);
> 
> -       if (TREE_CODE (member) == FIELD_DECL)
> +       if (object_type && TYPE_PTRMEMFUNC_P(object_type)
> +           && TREE_CODE (member) == FIELD_DECL)
> +         {
> +           r = build_ptrmemfunc_access_expr (object, DECL_NAME(member));
> +           RETURN (r);
> +         }
> +       else if (TREE_CODE (member) == FIELD_DECL)
>            {
>              r = finish_non_static_data_member (member, object, NULL_TREE);
>              if (TREE_CODE (r) == COMPONENT_REF)
> 
>>> 	PR c++/101219
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* typeck.c (build_ptrmemfunc_access_expr): Suppress all warnings
>>> 	to avoid ICE.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/torture/pr101219.C: New test.
>>
>> This doesn't need to be in torture; it has nothing to do with optimization.
> 
> Aha, moved to gcc/testsuite/g++.dg/warn/pr101219.C.
> 
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/pr101219.C
> @@ -0,0 +1,11 @@
> +/* PR c++/101219 - ICE on use of uninitialized memfun pointer
> +   { dg-do compile }
> +   { dg-options "-Wall" } */
> +
> +struct S { void m(); };
> +
> +template <int> bool f() {
> +  void (S::*mp)();
> +
> +  return &S::m == mp; // no warning emitted here (no instantiation)
> +}
> 
> Another question: Is it expected that gcc generates no warnings here?
> It's an uninstantiated function (-1 for warn), but from what I
> understand it's guaranteed to generate comparison with uninitialized
> data if it ever gets instantiated. Given that we used to ICE in
> warning code gcc could possibly flag it? (+1 for warn)

Generally it's desirable to diagnose templates for which no valid 
instantiation is possible.  It seems reasonable in most cases to also 
warn about templates for which all instantiations would warn.

But uninitialized warnings rely on flow analysis that we only do on 
instantiated functions, and in any case the ICE doesn't depend on mp 
being uninitialized; I get the same crash if I add = 0 to the declaration.

> +	if (object_type && TYPE_PTRMEMFUNC_P(object_type)

Missing space before (.

> +	    && TREE_CODE (member) == FIELD_DECL)
> +	  {
> +	    r = build_ptrmemfunc_access_expr (object, DECL_NAME(member));

And here.

Jason