[RFC,/,RFH] Re-opened C++/51213 (access control under SFINAE)

Hi,

On 08/01/2012 04:32 PM, Jason Merrill wrote:
> I think the problem is that we're deferring access control due to
> tentative parsing on line 11, and not on line 13.  I guess we need a
>
> push_deferring_access_checks (dk_no_deferred);
> pop_deferring_access_checks ();
>
> around the substitution of default template args in
> type_unification_real.
Great, thanks. Thus, I have been testing the attached and it definitely 
works for the testcases I discussed so far. Testsuite seems also Ok 
(lightly tested so far).

However, something weird is going on for this variant, using decltype 
(wanted to consistently extend sfinae37.C):

class C {
    typedef int type;
};

template<class T>
auto g(int) -> decltype(typename T::type(), char());

template<class>
auto g(...) -> char (&)[2];

static_assert(sizeof(g<C>(0)) == 2, "Ouch");          // line 11

typedef int testg[sizeof(g<C>(0)) == 2 ? 1 : -1];      // line 13

what happens is that line 13 is mishandled:

sfinae37_red.C:13:48: error: size of array ‘testg’ is negative

However, *if I comment out line 11*, things work for line 13! If I swap 
line 11 and line 13 then the declaration of testg is accepted and the 
static_assert triggers. In any case, only the first evaluation of the 
sizeof is correct, the next are incorrect. The issue seems so weird that 
it should be easy to fix... ;)

Final important observation: in fact, this variant with decltype is 
handled in the same way with or without the push_deferring_access_checks 
/ pop_deferring_access_checks calls.

Paolo.

////////////////////////

On 08/01/2012 11:49 AM, Paolo Carlini wrote:
> static_assert(sizeof(g<C>(0)) == 2, "Ouch");          // line 11
>
> typedef int testg[sizeof(g<C>(0)) == 2 ? 1 : -1];      // line 13
>
> what happens is that line 13 is mishandled:
>
> sfinae37_red.C:13:48: error: size of array ‘testg’ is negative
>
> However, *if I comment out line 11*, things work for line 13! If I swap
> line 11 and line 13 then the declaration of testg is accepted and the
> static_assert triggers.

Curious.  I guess that the second time we see the call the compiler 
thinks it already has the candidate it needs, but I don't know why that 
would be.  Are we not getting to type_unification_real from 
add_template_candidate the second time?

Jason

On 08/01/2012 07:35 PM, Jason Merrill wrote:
> On 08/01/2012 11:49 AM, Paolo Carlini wrote:
>> static_assert(sizeof(g<C>(0)) == 2, "Ouch");          // line 11
>>
>> typedef int testg[sizeof(g<C>(0)) == 2 ? 1 : -1];      // line 13
>>
>> what happens is that line 13 is mishandled:
>>
>> sfinae37_red.C:13:48: error: size of array ‘testg’ is negative
>>
>> However, *if I comment out line 11*, things work for line 13! If I swap
>> line 11 and line 13 then the declaration of testg is accepted and the
>> static_assert triggers.
>
> Curious.  I guess that the second time we see the call the compiler 
> thinks it already has the candidate it needs, but I don't know why 
> that would be.  Are we not getting to type_unification_real from 
> add_template_candidate the second time?
So, I'm in the middle of this (got distracted earlier today). I can tell 
you what I have.

For the second evaluation, the second time we call 
instantiate_template_1, thus for the interesting g(int) overload, here:

   spec = retrieve_specialization (gen_tmpl, targ_ptr, 0);

   gcc_assert (tmpl == gen_tmpl
           || ((fndecl = retrieve_specialization (tmpl, orig_args, 0))
           == spec)
           || fndecl == NULL_TREE);

   if (spec != NULL_TREE)
     {
       if (FNDECL_RECHECK_ACCESS_P (spec) && (complain & tf_error))
     recheck_decl_substitution (spec, gen_tmpl, targ_ptr);
       return spec;
     }

things are completely different, because spec != NULL_TREE and, more 
importantly, complain is tf_none, thus recheck_decl_substitution is not 
called, we just return immediately.

Compare to the first evaluation: in that case we call enforce_access 
*way* below, with the perform_deferred_access_checks call near the end 
of instantiate_template_1.

Thus, looks like the recheck_decl_substitution mechanism is not working 
by design because of complain == tf_none?!?

Note that while I'm debugging this, I see instantiate_template_1 always 
getting complain == tf_none, something seems weird about the && 
(complain & tf_error) above...

Paolo.

Hi again,

On 08/01/2012 08:40 PM, Paolo Carlini wrote:
> For the second evaluation, the second time we call 
> instantiate_template_1, thus for the interesting g(int) overload, here:
>
>   spec = retrieve_specialization (gen_tmpl, targ_ptr, 0);
>
>   gcc_assert (tmpl == gen_tmpl
>           || ((fndecl = retrieve_specialization (tmpl, orig_args, 0))
>           == spec)
>           || fndecl == NULL_TREE);
>
>   if (spec != NULL_TREE)
>     {
>       if (FNDECL_RECHECK_ACCESS_P (spec) && (complain & tf_error))
>     recheck_decl_substitution (spec, gen_tmpl, targ_ptr);
>       return spec;
>     }
>
> things are completely different, because spec != NULL_TREE and, more 
> importantly, complain is tf_none, thus recheck_decl_substitution is 
> not called, we just return immediately.
So, it is possible that when spec != NULL_TREE and we are once more in a 
SFINAE context, we have to actually call perform_deferred_access_checks 
(complain) and either return error_mark_node or the spec depending on 
the return value?

Paolo.

On 08/01/2012 02:57 PM, Paolo Carlini wrote:
> So, it is possible that when spec != NULL_TREE and we are once more in a
> SFINAE context, we have to actually call perform_deferred_access_checks
> (complain) and either return error_mark_node or the spec depending on
> the return value?

I don't think we need to redo the access check in SFINAE context; we 
know that there's an error, so we can just return error_mark_node in 
that case.  I guess we should change the name of FNDECL_RECHECK_ACCESS_P 
to something like FNDECL_HAS_ACCESS_ERRORS to make it clearer.

Jason