Re: [PATCH v2] c++: explicit inst of template method not generated [PR110323]

[Jason Merrill <jason@redhat.com>]

On 3/19/24 15:30, Marek Polacek wrote:
> On Mon, Mar 18, 2024 at 09:10:27PM -0400, Jason Merrill wrote:
>> On 3/15/24 13:48, Marek Polacek wrote:
>>> On Thu, Mar 14, 2024 at 03:39:04PM -0400, Jason Merrill wrote:
>>>> On 3/8/24 12:02, Marek Polacek wrote:
>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>>>
>>>>> -- >8 --
>>>>> Consider
>>>>>
>>>>>      constexpr int VAL = 1;
>>>>>      struct foo {
>>>>>          template <int B>
>>>>>          void bar(typename std::conditional<B==VAL, int, float>::type arg) { }
>>>>>      };
>>>>>      template void foo::bar<1>(int arg);
>>>>>
>>>>> where we since r11-291 fail to emit the code for the explicit
>>>>> instantiation.  That's because cp_walk_subtrees/TYPENAME_TYPE now
>>>>> walks TYPE_CONTEXT ('conditional' here) as well, and in a template
>>>>> finds the B==VAL template argument.  VAL is constexpr, which implies const,
>>>>> which in the global scope implies static.  constrain_visibility_for_template
>>>>> then makes "struct conditional<(B == VAL), int, float>" non-TREE_PUBLIC.
>>>>> Then symtab_node::needed_p checks TREE_PUBLIC, sees it's 0, and we don't
>>>>> emit any code.
>>>>>
>>>>> I thought the fix would be some ODR-esque check to not consider
>>>>> constexpr variables/fns that are used just for their value.  But
>>>>> it turned out to be tricky.  For instance, we can't skip
>>>>> determine_visibility in a template; we can't even skip it for value-dep
>>>>> expressions.  For example, no-linkage-expr1.C has
>>>>>
>>>>>      using P = struct {}*;
>>>>>      template <int N>
>>>>>      void f(int(*)[((P)0, N)]) {}
>>>>>
>>>>> where ((P)0, N) is value-dep, but N is not relevant here: we have to
>>>>> ferret out the anonymous type.  When instantiating, it's already gone.
>>>>
>>>> Hmm, how is that different from the B == VAL case?  In both cases we're
>>>> naming an internal entity that gets folded away.
>>>>
>>>> I guess the difference is that B == VAL falls under the special allowance in
>>>> https://eel.is/c++draft/basic.def.odr#14.5.1 because it's a constant used as
>>>> a prvalue, and therefore is not odr-used under
>>>> https://eel.is/c++draft/basic.def.odr#5.2
>>>>
>>>> So I would limit this change to decl_constant_var_p.  Really we should also
>>>> be checking that the lvalue-rvalue conversion is applied, but that's more
>>>> complicated.
>>>
>>> Thanks.  My previous version had it, but it didn't handle
>>>
>>>     static constexpr int getval () { return 1; }
>>>
>>>     template <int B>
>>>     void baz(typename conditional<B == getval (), int, float>::type arg) { }
>>>
>>> I'd say that "getval()" is one of "manifestly constant-evaluated expressions that
>>> are not value-dependent", so it should be treated the same as B == VAL.
>>
>> But it doesn't satisfy the 14.5 rule that corresponding names need to refer
>> to the same entity; since getval names a function, it doesn't get the
>> special exemption from that rule that VAL gets.
>>
>> So this should not be treated the same as B == VAL.
>   
> Thanks for the explanation.
> 
>>> I don't know if this is important to handle.  Do you want me to poke further or
>>> should we just go with decl_constant_var_p and leave it at that for now?
>>
>> Just decl_constant_var_p.
> 
> Here it is:
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> Consider
> 
>    constexpr int VAL = 1;
>    struct foo {
>        template <int B>
>        void bar(typename std::conditional<B==VAL, int, float>::type arg) { }
>    };
>    template void foo::bar<1>(int arg);
> 
> where we since r11-291 fail to emit the code for the explicit
> instantiation.  That's because cp_walk_subtrees/TYPENAME_TYPE now
> walks TYPE_CONTEXT ('conditional' here) as well, and in a template
> finds the B==VAL template argument.  VAL is constexpr, which implies const,
> which in the global scope implies static.  constrain_visibility_for_template
> then makes "struct conditional<(B == VAL), int, float>" non-TREE_PUBLIC.
> Then symtab_node::needed_p checks TREE_PUBLIC, sees it's 0, and we don't
> emit any code.
> 
> I thought the fix would be some ODR-esque check to not consider
> constexpr variables/fns that are used just for their value.  But
> it turned out to be tricky.  For instance, we can't skip
> determine_visibility in a template; we can't even skip it for value-dep
> expressions.  For example, no-linkage-expr1.C has
> 
>    using P = struct {}*;
>    template <int N>
>    void f(int(*)[((P)0, N)]) {}
> 
> where ((P)0, N) is value-dep, but N is not relevant here: we have to
> ferret out the anonymous type.  When instantiating, it's already gone.
> 
> This patch uses decl_constant_var_p.  This is to implement (an
> approximation) [basic.def.odr]#14.5.1 and [basic.def.odr]#5.2.
> 
> 	PR c++/110323
> 
> gcc/cp/ChangeLog:
> 
> 	* decl2.cc (min_vis_expr_r) <case VAR_DECL>: Do nothing for
> 	decl_constant_var_p VAR_DECLs.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/explicit-instantiation6.C: New test.
> 	* g++.dg/template/explicit-instantiation7.C: New test.
> ---
>   gcc/cp/decl2.cc                               |  6 ++-
>   .../g++.dg/template/explicit-instantiation6.C | 43 +++++++++++++++++++
>   .../g++.dg/template/explicit-instantiation7.C | 22 ++++++++++
>   3 files changed, 70 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/template/explicit-instantiation6.C
>   create mode 100644 gcc/testsuite/g++.dg/template/explicit-instantiation7.C
> 
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 2562d8aeff6..753e45e56ad 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -2718,7 +2718,11 @@ min_vis_expr_r (tree *tp, int */*walk_subtrees*/, void *data)
>         /* Fall through.  */
>       case VAR_DECL:
>       case FUNCTION_DECL:
> -      if (! TREE_PUBLIC (t))
> +      if (decl_constant_var_p (t))
> +	/* In a template, we can't trust VAR_DECLs, either.  It's possible
> +	   they won't be ODR-used, and we could wrongly think the linkage
> +	   is internal (PR110323).  */;

Maybe "The ODR allows definitions in different TUs to refer to distinct 
constant variables with internal or no linkage, so such a reference 
shouldn't affect visibility (PR110323).  FIXME but only if the 
lvalue-rvalue conversion is applied."

OK with that change.

> +      else if (! TREE_PUBLIC (t))
>   	tpvis = VISIBILITY_ANON;
>         else
>   	tpvis = DECL_VISIBILITY (t);
> diff --git a/gcc/testsuite/g++.dg/template/explicit-instantiation6.C b/gcc/testsuite/g++.dg/template/explicit-instantiation6.C
> new file mode 100644
> index 00000000000..8b77c9deb20
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/explicit-instantiation6.C
> @@ -0,0 +1,43 @@
> +// PR c++/110323
> +// { dg-do compile { target c++14 } }
> +
> +template<bool B, class T, class F>
> +struct conditional { using type = T; };
> +
> +template<class T, class F>
> +struct conditional<false, T, F> { using type = F; };
> +
> +constexpr int VAL = 1;
> +
> +static constexpr int getval () { return 1; }
> +
> +template<typename>
> +constexpr int TVAL = 1;
> +
> +static struct S {
> +  constexpr operator bool() { return true; }
> +} s;
> +
> +struct foo {
> +    template <int B>
> +    void bar(typename conditional<B == VAL, int, float>::type arg) { }
> +
> +    template <int B>
> +    void qux(typename conditional<B == TVAL<int>, int, float>::type arg) { }
> +
> +    template <int B>
> +    void sox(typename conditional<B == noexcept (VAL), int, float>::type arg) { }
> +
> +    template <int B>
> +    void nim(typename conditional<B != sizeof (VAL), int, float>::type arg) { }
> +};
> +
> +template void foo::bar<1>(int arg);
> +template void foo::qux<1>(int arg);
> +template void foo::sox<1>(int arg);
> +template void foo::nim<1>(int arg);
> +
> +// { dg-final { scan-assembler "_ZN3foo3barILi1EEEvN11conditionalIXeqT_L_ZL3VALEEifE4typeE" } }
> +// { dg-final { scan-assembler "_ZN3foo3quxILi1EEEvN11conditionalIXeqT_L_Z4TVALIiEEEifE4typeE" } }
> +// { dg-final { scan-assembler "_ZN3foo3soxILi1EEEvN11conditionalIXeqT_nxL_ZL3VALEEifE4typeE" } }
> +// { dg-final { scan-assembler "_ZN3foo3nimILi1EEEvN11conditionalIXneT_szL_ZL3VALEEifE4typeE" } }
> diff --git a/gcc/testsuite/g++.dg/template/explicit-instantiation7.C b/gcc/testsuite/g++.dg/template/explicit-instantiation7.C
> new file mode 100644
> index 00000000000..9a870e808fa
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/explicit-instantiation7.C
> @@ -0,0 +1,22 @@
> +// PR c++/110323
> +// { dg-do compile { target c++11 } }
> +
> +using P = struct { }*;
> +using N = struct A { }*;
> +
> +template<bool B, class T, class F>
> +struct conditional { using type = T; };
> +
> +struct foo {
> +    template <int B>
> +    void bar(typename conditional<((P) 0, B), int, float>::type arg) { }
> +
> +    template <int B>
> +    void baz(typename conditional<((N) 0, B), int, float>::type arg) { }
> +};
> +
> +template void foo::bar<1>(int arg);
> +template void foo::baz<1>(int arg);
> +
> +// { dg-final { scan-assembler-not "_ZN3foo3barILi1EEEvN11conditionalIXcmcvP1XLi0EneT_Li0EEifE4typeE" } }
> +// { dg-final { scan-assembler "_ZN3foo3bazILi1EEEvN11conditionalIXcmcvP1ALi0EneT_Li0EEifE4typeE" } }
> 
> base-commit: bc91e3870e9c984c180b478a3449a9a2e56cd107