I would think that when we see a duplicated base we should just drop
the duplicates and continue.
If the type of b1 is error_mark_node, why isn't the type of b2 also
error_mark_node?
Jason
On Thu, Jun 9, 2016 at 1:01 PM, Paolo Carlini <paolo.carl...@oracle.com> wrote:
> Hi again,
>
> thus today I had to revert my first try at resolving this error recovery
> issue because it caused c++/71465. In retrospect it was a bit heavy handed
> anyway, the zeroing of the type served us well for many years...
>
> Today I tried to investigate the issue a bit more. I remind you that we are
> crashing on the gcc_unreachable () at the end of build_simple_base_path
> while, during error recovery, we are processing the initialization of the
> final 'a' in:
>
> class A
> {
> virtual void foo () { }
> };
> class B : public A, A { }; // { dg-error "duplicate base type" }
>
> B b1, &b2 = b1;
> A a = b2;
>
> Now the first comment I have about this, about which I'd like to have some
> feedback, is that the gcc_unreachable () at the end of
> build_simple_base_path without a return seems a bit weird to me, because the
> function is *supposed to return a tree*, not void. I guess some front-ends
> would even warn on this?!? Anyway, if we change:
>
>
> /* */
>
> gcc_unreachable ();
> }
>
> to:
>
> /* */
>
> gcc_assert (seen_error ());
> return error_mark_node;
> }
>
> we get something which, by and large is equally safe in terms of
> miscompilations and helps a lot error recovery, ie, we would not have
> c++/70202. It's also true however that we would not make much progress in
> trying to understand if we could solve the specific problem at issue in a
> more insightful way.
>
> Then my next observation is the following: if we change the testcase to:
>
> class A
> {
> virtual void foo () { }
> };
> class B : public A, A { }; // { dg-error "duplicate base type" }
>
> B b1, &b2 = b1;
> A a = b1;
>
> thus everything is the same but at the end we have 'b1', not 'b2', we also
> don't crash. We are saved not by something particularly smart ;) but by this
> check at the beginning of ocp_convert:
>
> if (error_operand_p (e) || type == error_mark_node)
> return error_mark_node;
>
> that is, the TREE_TYPE of 'e', the VAR_DECL for 'b1', is error_mark_node and
> we return early the error_mark_node, we don't reach the problematic
> build_base_path. With 'b2' we have a more complicated REFERENCE_REF which we
> don't catch, but otherwise everything is the same, we reach build_base_path
> and we crash on the gcc_unreachable. In practice, if I add at the beginning
> of ocp_convert:
>
> + if (REFERENCE_REF_P (expr)
> + && VAR_P (TREE_OPERAND (expr, 0))
> + && DECL_INITIAL (TREE_OPERAND (expr, 0)) == error_mark_node)
> + return error_mark_node;
>
> it also passes testing. Then the final observation is that we also don't
> crash if the type of 'A' is simpler, thus we don't have the declaration of
>
> virtual void foo () { }
>
> Then I gather that when the type is "complex" enough we are not cleaning
> things up completely enough when we encounter it as a duplicated base. Maybe
> as optimal error recovery when we see a duplicated base we should clean up
> everything having to do with bases end up with something equivalent to an
> incomplete class B; or something like that?!? I don't know what we want to
> try short / medium /long term...
>
> Thanks!
> Paolo.
>
>
