[PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]

["Kewen.Lin" <linkw@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2147 bytes --]

Hi Richi,

Thanks for the review comments!

on 2022/9/8 15:36, Richard Biener wrote:
> 
> 
>> Am 08.09.2022 um 07:53 schrieb Kewen.Lin <linkw@linux.ibm.com>:
>>
>> Hi,
>>
>> As PR106833 shows, cv-qualified opaque type can cause ICE
>> during LTO.  It exposes that we missd to handle OPAQUE_TYPE
>> well in type verification.  As Richi pointed out, also
>> assuming that target will always define TYPE_MAIN_VARIANT
>> and TYPE_CANONICAL for opaque type, this patch is to check
>> both are OPAQUE_TYPE_P.  Besides, it also checks the only
>> available size and alignment information as well as type
>> mode for TYPE_MAIN_VARIANT.
>>
...
>> +
>> +  if (t != tv)
>> +    {
>> +      verify_match (TREE_CODE, t, tv);
>> +      verify_match (TYPE_MODE, t, tv);
>> +      verify_match (TYPE_SIZE, t, tv);
> 
> TYPE_SIZE is a tree, you should probably 
> Compare this with operand_equal_p.  It’s 
> Not documented to be a constant size?
> Thus some VLA vector mode might be allowed ( a poly_int size), 

Thanks for catching, I was referencing the code in function
verify_type_variant, that corresponding part seems imperfect:

      if (TREE_CODE (TYPE_SIZE (t)) != PLACEHOLDER_EXPR
	  && TREE_CODE (TYPE_SIZE (tv)) != PLACEHOLDER_EXPR)
	verify_variant_match (TYPE_SIZE);

I agree poly_int size is allowed, the patch was updated for it.

BLKmode
> Is ruled out(?), 

Yes, it requires a mode of MODE_OPAQUE class.

the docs say we have
> ‚An MODE_Opaque‘ here but I don’t see
> This being verified?
> 

There is a MODE equality check, I assumed the given t already
has one MODE_OPAQUE mode, but the patch was updated to make
it explicit as you concerned.

> The macro makes this a bit unworldly
> For the only benefit of elaborate diagnostic
> Which I think isn’t really necessary

OK, fixed!

The previous version makes just one check on TYPE_CANONICAL to
be cheap as gimple_canonical_types_compatible_p said, but
since there are just several fields to be check, this updated
version adjusted it to be the same as what's for TYPE_MAIN_VARIANT.
Hope it's fine. :)

Tested as before.

Does this updated patch look good to you?

BR,
Kewen
------

[-- Attachment #2: 0001-Handle-OPAQUE_TYPE-specially-in-verify_type-PR106833.patch --]
[-- Type: text/plain, Size: 4401 bytes --]

From 4a905fcb2abcc4e488d90011dd2c2125fb9e14b2 Mon Sep 17 00:00:00 2001
From: Kewen Lin <linkw@linux.ibm.com>
Date: Thu, 8 Sep 2022 21:34:29 -0500
Subject: [PATCH] Handle OPAQUE_TYPE specially in verify_type [PR106833]

As PR106833 shows, cv-qualified opaque type can cause ICE
during LTO.  It exposes that we missd to handle OPAQUE_TYPE
well in type verification.  As Richi pointed out, also
assuming that target will always define TYPE_MAIN_VARIANT
TYPE_CANONICAL for opaque type, this patch is to check
both are OPAQUE_TYPE_P and their modes are of MODE_OPAQUE
class.  Besides, it also checks the only available size
and alignment information.

	PR middle-end/106833

gcc/ChangeLog:

	* tree.cc (verify_opaque_type): New function.
	(verify_type): Call verify_opaque_type for OPAQUE_TYPE.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr106833.c: New test.
---
 gcc/testsuite/gcc.target/powerpc/pr106833.c | 14 ++++
 gcc/tree.cc                                 | 74 ++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106833.c

diff --git a/gcc/testsuite/gcc.target/powerpc/pr106833.c b/gcc/testsuite/gcc.target/powerpc/pr106833.c
new file mode 100644
index 00000000000..968d75184ff
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr106833.c
@@ -0,0 +1,14 @@
+/* { dg-do link } */
+/* { dg-require-effective-target power10_ok } */
+/* { dg-require-effective-target lto } */
+/* { dg-options "-flto -mdejagnu-cpu=power10" } */
+
+/* Verify there is no ICE in LTO mode.  */
+
+int main ()
+{
+  float *b;
+  const __vector_quad c;
+  __builtin_mma_disassemble_acc (b, &c);
+  return 0;
+}
diff --git a/gcc/tree.cc b/gcc/tree.cc
index fed1434d141..b755cd5083a 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -13670,6 +13670,71 @@ gimple_canonical_types_compatible_p (const_tree t1, const_tree t2,
     }
 }
 
+/* For OPAQUE_TYPE T, it should have only size and alignment information
+   and its mode should be of class MODE_OPAQUE.  This function verifies
+   these properties of T match TV which is the main variant of T and TC
+   which is the canonical of T.  */
+
+static void
+verify_opaque_type (const_tree t, tree tv, tree tc)
+{
+  gcc_assert (OPAQUE_TYPE_P (t));
+  gcc_assert (tv && tv == TYPE_MAIN_VARIANT (tv));
+  gcc_assert (tc && tc == TYPE_CANONICAL (tc));
+
+  /* For an opaque type T1, check if some of its properties match
+     the corresponding ones of the other opaque type T2, emit some
+     error messages for those inconsistent ones.  */
+  auto check_properties_for_opaque_type = [](const_tree t1, tree t2,
+					     const char *kind_msg)
+  {
+    if (!OPAQUE_TYPE_P (t2))
+      {
+	error ("type %s is not an opaque type", kind_msg);
+	debug_tree (t2);
+	return;
+      }
+    if (!OPAQUE_MODE_P (TYPE_MODE (t2)))
+      {
+	error ("type %s is not with opaque mode", kind_msg);
+	debug_tree (t2);
+	return;
+      }
+    if (TYPE_MODE (t1) != TYPE_MODE (t2))
+      {
+	error ("type %s differs by %<TYPE_MODE%>", kind_msg);
+	debug_tree (t2);
+	return;
+      }
+    poly_uint64 t1_size = tree_to_poly_uint64 (TYPE_SIZE (t1));
+    poly_uint64 t2_size = tree_to_poly_uint64 (TYPE_SIZE (t2));
+    if (maybe_ne (t1_size, t2_size))
+      {
+	error ("type %s differs by %<TYPE_SIZE%>", kind_msg);
+	debug_tree (t2);
+	return;
+      }
+    if (TYPE_ALIGN (t1) != TYPE_ALIGN (t2))
+      {
+	error ("type %s differs by %<TYPE_ALIGN%>", kind_msg);
+	debug_tree (t2);
+	return;
+      }
+    if (TYPE_USER_ALIGN (t1) != TYPE_USER_ALIGN (t2))
+      {
+	error ("type %s differs by %<TYPE_USER_ALIGN%>", kind_msg);
+	debug_tree (t2);
+	return;
+      }
+  };
+
+  if (t != tv)
+    check_properties_for_opaque_type (t, tv, "variant");
+
+  if (t != tc)
+    check_properties_for_opaque_type (t, tc, "canonical");
+}
+
 /* Verify type T.  */
 
 void
@@ -13677,6 +13742,14 @@ verify_type (const_tree t)
 {
   bool error_found = false;
   tree mv = TYPE_MAIN_VARIANT (t);
+  tree ct = TYPE_CANONICAL (t);
+
+  if (OPAQUE_TYPE_P (t))
+    {
+      verify_opaque_type (t, mv, ct);
+      return;
+    }
+
   if (!mv)
     {
       error ("main variant is not defined");
@@ -13691,7 +13764,6 @@ verify_type (const_tree t)
   else if (t != mv && !verify_type_variant (t, mv))
     error_found = true;
 
-  tree ct = TYPE_CANONICAL (t);
   if (!ct)
     ;
   else if (TYPE_CANONICAL (ct) != ct)
-- 
2.27.0