diff mbox

Fix ICE in modified_type_die (PR debug/80461)

Message ID 20170419142849.GI1809@tucnak
State New
Headers show

Commit Message

Jakub Jelinek April 19, 2017, 2:28 p.m. UTC 
Hi!

On the following testcase we ICE in modified_type_die, because the
search for a non-qualified variant type with the same rvalue quals
etc. finds pretty randomly qualified type, while modified_type_die
expects that it is either an unqualified type or (if there is no such a
type) type itself.  When we are looking for const qualified FUNCTION_TYPE
and find instead volatile qualified FUNCTION_TYPE, then we die on an
assertion that we can't subtract qualifiers from the chosen type to
get to the desired type (only can add them).

The following patch makes sure we pick only the unqualified types.
Bootstrapped/regtested on x86_64-linux and i686-linux with additional
statistics collection which found that both the loops found such an
unqualified type in all cases except for a single fntype in each of the
following testcases:
libstdc++-v3/testsuite/20_util/add_lvalue_reference/value.cc
libstdc++-v3/testsuite/20_util/add_rvalue_reference/value.cc
libstdc++-v3/testsuite/20_util/is_assignable/value.cc
where we didn't find any unqualified types among the variants;
previously we found type itself that way, which is what the patched
compiler does too in that case (see the return lookup_type_die (type)
after it or that type remains the old type).

Ok for trunk?

2017-04-19  Jakub Jelinek  <jakub@redhat.com>

	PR debug/80461
	* dwarf2out.c (modified_type_die, gen_type_die_with_usage):
	Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.

	* g++.dg/debug/pr80461.C: New test.


	Jakub

Comments

Jeff Law April 19, 2017, 2:57 p.m. UTC | #1 
On 04/19/2017 08:28 AM, Jakub Jelinek wrote:
> Hi!
> 
> On the following testcase we ICE in modified_type_die, because the
> search for a non-qualified variant type with the same rvalue quals
> etc. finds pretty randomly qualified type, while modified_type_die
> expects that it is either an unqualified type or (if there is no such a
> type) type itself.  When we are looking for const qualified FUNCTION_TYPE
> and find instead volatile qualified FUNCTION_TYPE, then we die on an
> assertion that we can't subtract qualifiers from the chosen type to
> get to the desired type (only can add them).
> 
> The following patch makes sure we pick only the unqualified types.
> Bootstrapped/regtested on x86_64-linux and i686-linux with additional
> statistics collection which found that both the loops found such an
> unqualified type in all cases except for a single fntype in each of the
> following testcases:
> libstdc++-v3/testsuite/20_util/add_lvalue_reference/value.cc
> libstdc++-v3/testsuite/20_util/add_rvalue_reference/value.cc
> libstdc++-v3/testsuite/20_util/is_assignable/value.cc
> where we didn't find any unqualified types among the variants;
> previously we found type itself that way, which is what the patched
> compiler does too in that case (see the return lookup_type_die (type)
> after it or that type remains the old type).
> 
> Ok for trunk?
> 
> 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/80461
> 	* dwarf2out.c (modified_type_die, gen_type_die_with_usage):
> 	Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.
> 
> 	* g++.dg/debug/pr80461.C: New test.
I'm going to assume your use of TYPE_QUALS_NO_ADDR_SPACE vs TYPE_QUALS 
or TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC is correct.

OK.
jeff
Jakub Jelinek April 19, 2017, 3:10 p.m. UTC | #2 
On Wed, Apr 19, 2017 at 08:57:52AM -0600, Jeff Law wrote:
> > 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR debug/80461
> > 	* dwarf2out.c (modified_type_die, gen_type_die_with_usage):
> > 	Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.
> > 
> > 	* g++.dg/debug/pr80461.C: New test.
> I'm going to assume your use of TYPE_QUALS_NO_ADDR_SPACE vs TYPE_QUALS or
> TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC is correct.

I don't really know.  For address space quals I think one would need to have
pointer-to-members in different code address spaces, not sure if we support
anything like that.  And atomic is C only which doesn't have
pointer-to-members.

	Jakub
Jeff Law April 19, 2017, 3:55 p.m. UTC | #3 
On 04/19/2017 09:10 AM, Jakub Jelinek wrote:
> On Wed, Apr 19, 2017 at 08:57:52AM -0600, Jeff Law wrote:
>>> 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR debug/80461
>>> 	* dwarf2out.c (modified_type_die, gen_type_die_with_usage):
>>> 	Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.
>>>
>>> 	* g++.dg/debug/pr80461.C: New test.
>> I'm going to assume your use of TYPE_QUALS_NO_ADDR_SPACE vs TYPE_QUALS or
>> TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC is correct.
> 
> I don't really know.  For address space quals I think one would need to have
> pointer-to-members in different code address spaces, not sure if we support
> anything like that.  And atomic is C only which doesn't have
> pointer-to-members.
To put it another way, in your message you indicated that 
modified_type_die expects either an unqualified type or the type itself 
and that your patch makes sure we only pick unqualified types.

Using TYPE_QUALS_NO_ADDR_SPACE like you have seems to conflict with 
those statements.  So I'm curious why you allow address space qualifiers 
to pass through, but no others.   It just seems odd.

I'm guessing you're doing that because modified_type_die ignores address 
space qualifiers (and it only conditionally ignores atomics or restrict 
based on dwarf level).  But it'd be good to get a confirmation that's 
what you were thinking.

jeff
Jakub Jelinek April 19, 2017, 4:13 p.m. UTC | #4 
On Wed, Apr 19, 2017 at 09:55:19AM -0600, Jeff Law wrote:
> On 04/19/2017 09:10 AM, Jakub Jelinek wrote:
> > On Wed, Apr 19, 2017 at 08:57:52AM -0600, Jeff Law wrote:
> > > > 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
> > > > 
> > > > 	PR debug/80461
> > > > 	* dwarf2out.c (modified_type_die, gen_type_die_with_usage):
> > > > 	Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.
> > > > 
> > > > 	* g++.dg/debug/pr80461.C: New test.
> > > I'm going to assume your use of TYPE_QUALS_NO_ADDR_SPACE vs TYPE_QUALS or
> > > TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC is correct.
> > 
> > I don't really know.  For address space quals I think one would need to have
> > pointer-to-members in different code address spaces, not sure if we support
> > anything like that.  And atomic is C only which doesn't have
> > pointer-to-members.
> To put it another way, in your message you indicated that modified_type_die
> expects either an unqualified type or the type itself and that your patch
> makes sure we only pick unqualified types.
> 
> Using TYPE_QUALS_NO_ADDR_SPACE like you have seems to conflict with those
> statements.  So I'm curious why you allow address space qualifiers to pass
> through, but no others.   It just seems odd.

I used TYPE_QUALS_NO_ADDR_SPACE because that seems to be used in
modified_type_die elsewhere (dwarf2out.c has only one use of TYPE_QUALS and
even that one ignores addr space bits, as it is masked with qual_mask;
the rest just TYPE_QUALS_NO_ADDR_SPACE).  I think with FUNCTION/METHOD types
with addr space quals we end up in grossly untested territory that most
likely will just not work at all.  I think we don't do anything with the
address space quals right now, in the future DWARF has segment identifiers
and something like that could be emitted, but there needs to be ABI
agreement on what it means.

	Jakub
Jeff Law April 19, 2017, 4:29 p.m. UTC | #5 
On 04/19/2017 10:13 AM, Jakub Jelinek wrote:
> On Wed, Apr 19, 2017 at 09:55:19AM -0600, Jeff Law wrote:
>> On 04/19/2017 09:10 AM, Jakub Jelinek wrote:
>>> On Wed, Apr 19, 2017 at 08:57:52AM -0600, Jeff Law wrote:
>>>>> 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
>>>>>
>>>>> 	PR debug/80461
>>>>> 	* dwarf2out.c (modified_type_die, gen_type_die_with_usage):
>>>>> 	Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.
>>>>>
>>>>> 	* g++.dg/debug/pr80461.C: New test.
>>>> I'm going to assume your use of TYPE_QUALS_NO_ADDR_SPACE vs TYPE_QUALS or
>>>> TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC is correct.
>>>
>>> I don't really know.  For address space quals I think one would need to have
>>> pointer-to-members in different code address spaces, not sure if we support
>>> anything like that.  And atomic is C only which doesn't have
>>> pointer-to-members.
>> To put it another way, in your message you indicated that modified_type_die
>> expects either an unqualified type or the type itself and that your patch
>> makes sure we only pick unqualified types.
>>
>> Using TYPE_QUALS_NO_ADDR_SPACE like you have seems to conflict with those
>> statements.  So I'm curious why you allow address space qualifiers to pass
>> through, but no others.   It just seems odd.
> 
> I used TYPE_QUALS_NO_ADDR_SPACE because that seems to be used in
> modified_type_die elsewhere (dwarf2out.c has only one use of TYPE_QUALS and
> even that one ignores addr space bits, as it is masked with qual_mask;
> the rest just TYPE_QUALS_NO_ADDR_SPACE).  I think with FUNCTION/METHOD types
> with addr space quals we end up in grossly untested territory that most
> likely will just not work at all.  I think we don't do anything with the
> address space quals right now, in the future DWARF has segment identifiers
> and something like that could be emitted, but there needs to be ABI
> agreement on what it means.
OK.  So let's go with your patch -- my reading of modified_type_die was 
that it would ignore address-space qualifiers.  So I think your patch is 
safe, it was the inconsistency in the message and the patch itself I was 
most concerned about.

jeff
Jason Merrill May 3, 2017, 7:04 p.m. UTC | #6 
On Wed, Apr 19, 2017 at 12:29 PM, Jeff Law <law@redhat.com> wrote:
> On 04/19/2017 10:13 AM, Jakub Jelinek wrote:
>>
>> On Wed, Apr 19, 2017 at 09:55:19AM -0600, Jeff Law wrote:
>>>
>>> On 04/19/2017 09:10 AM, Jakub Jelinek wrote:
>>>>
>>>> On Wed, Apr 19, 2017 at 08:57:52AM -0600, Jeff Law wrote:
>>>>>>
>>>>>> 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
>>>>>>
>>>>>>         PR debug/80461
>>>>>>         * dwarf2out.c (modified_type_die, gen_type_die_with_usage):
>>>>>>         Check for t with zero TYPE_QUALS_NO_ADDR_SPACE.
>>>>>>
>>>>>>         * g++.dg/debug/pr80461.C: New test.
>>>>>
>>>>> I'm going to assume your use of TYPE_QUALS_NO_ADDR_SPACE vs TYPE_QUALS
>>>>> or
>>>>> TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC is correct.
>>>>
>>>>
>>>> I don't really know.  For address space quals I think one would need to
>>>> have
>>>> pointer-to-members in different code address spaces, not sure if we
>>>> support
>>>> anything like that.  And atomic is C only which doesn't have
>>>> pointer-to-members.
>>>
>>> To put it another way, in your message you indicated that
>>> modified_type_die
>>> expects either an unqualified type or the type itself and that your patch
>>> makes sure we only pick unqualified types.
>>>
>>> Using TYPE_QUALS_NO_ADDR_SPACE like you have seems to conflict with those
>>> statements.  So I'm curious why you allow address space qualifiers to
>>> pass
>>> through, but no others.   It just seems odd.
>>
>>
>> I used TYPE_QUALS_NO_ADDR_SPACE because that seems to be used in
>> modified_type_die elsewhere (dwarf2out.c has only one use of TYPE_QUALS
>> and
>> even that one ignores addr space bits, as it is masked with qual_mask;
>> the rest just TYPE_QUALS_NO_ADDR_SPACE).  I think with FUNCTION/METHOD
>> types
>> with addr space quals we end up in grossly untested territory that most
>> likely will just not work at all.  I think we don't do anything with the
>> address space quals right now, in the future DWARF has segment identifiers
>> and something like that could be emitted, but there needs to be ABI
>> agreement on what it means.
>
> OK.  So let's go with your patch -- my reading of modified_type_die was that
> it would ignore address-space qualifiers.  So I think your patch is safe, it
> was the inconsistency in the message and the patch itself I was most
> concerned about.

BTW, I think it would be a bit more correct to compare the quals with
those of "main" rather than compare them to 0, though I suspect there
aren't currently any types for which that produces a different result.

Jason
diff mbox

Patch

--- gcc/dwarf2out.c.jj	2017-04-18 18:58:13.000000000 +0200
+++ gcc/dwarf2out.c	2017-04-19 13:05:31.109271480 +0200
@@ -12690,7 +12690,9 @@  modified_type_die (tree type, int cv_qua
 	     but try to canonicalize.  */
 	  tree main = TYPE_MAIN_VARIANT (type);
 	  for (tree t = main; t; t = TYPE_NEXT_VARIANT (t))
-	    if (check_base_type (t, main) && check_lang_type (t, type))
+	    if (TYPE_QUALS_NO_ADDR_SPACE (t) == 0
+		&& check_base_type (t, main)
+		&& check_lang_type (t, type))
 	      return lookup_type_die (t);
 	  return lookup_type_die (type);
 	}
@@ -24580,13 +24582,13 @@  gen_type_die_with_usage (tree type, dw_d
 	 but try to canonicalize.  */
       tree main = TYPE_MAIN_VARIANT (type);
       for (tree t = main; t; t = TYPE_NEXT_VARIANT (t))
-	{
-	  if (check_base_type (t, main) && check_lang_type (t, type))
-	    {
-	      type = t;
-	      break;
-	    }
-	}
+	if (TYPE_QUALS_NO_ADDR_SPACE (t) == 0
+	    && check_base_type (t, main)
+	    && check_lang_type (t, type))
+	  {
+	    type = t;
+	    break;
+	  }
     }
   else if (TREE_CODE (type) != VECTOR_TYPE
 	   && TREE_CODE (type) != ARRAY_TYPE)
--- gcc/testsuite/g++.dg/debug/pr80461.C.jj	2017-04-19 13:27:36.101829399 +0200
+++ gcc/testsuite/g++.dg/debug/pr80461.C	2017-04-19 13:27:46.618690922 +0200
@@ -0,0 +1,42 @@ 
+// PR debug/80461
+// { dg-do compile }
+// { dg-options "-g -O" }
+
+template <typename> class A;
+struct B
+{
+  template <typename T, typename U>
+  static bool foo (U T::*) {}
+};
+template <typename, typename> class J;
+template <typename T, typename U, typename V, typename... W>
+class J<V (W...), U T::*> : public J<void(), U T::*> {};
+template <typename T, typename U, typename... W>
+class J<void(W...), U T::*> : public B {};
+template <typename V, typename... W> struct A<V (W...)>
+{
+  template <typename, typename> using K = int;
+  template <typename L, typename = K<int, void>, typename = K<int, void>> A (L);
+};
+template <typename V, typename... W>
+template <typename L, typename, typename>
+A<V (W...)>::A (L x) { J<V (), L>::foo (x); }
+struct N;
+volatile int v;
+
+template <class O, class P>
+void
+bar ()
+{
+  O q;
+  A<P> f = q;
+  v++;
+}
+
+void
+baz ()
+{
+  bar<int (N::*) (...) &, int()> ();
+  bar<int (N::*) (...) const &, int()> ();
+  bar<int (N::*) (...) volatile &, int()> ();
+}