[patch] compile: Fix crash on cv-qualified self-reference

[patch] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

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

Hi,

with this modified testcase GDB would:

compile code struct_object.selffield = &struct_object
./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error)

While the insert_type() assertion looks unclear trying to fix it one ends up
with either GCC crash
	[gcc libcc1] build_qualified_type for self-referencing/incomplete types
	https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html
	c_incomplete_type_error()
or after fixing up the GCC type for proper error reporting one gets:
	gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’
which is the real culprit of this bug as explained in this patch.

This patch is related to the XFAIL introduced by
	[PATCH v3 5/9] compile: Use -Wall, not -w
	https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html
as for proper -Wall happiness the 'volatile' qualifier needs to be added there
- but adding the qualifier has caused this crash.

No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu.


Thanks,
Jan

[-- Attachment #2: selfrefqual.patch --]
[-- Type: text/plain, Size: 3206 bytes --]

gdb/ChangeLog
2015-04-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	compile: Fix crash on cv-qualified self-reference.
	* compile/compile-c-types.c (convert_struct_or_union): Apply
	build_qualified_type.
	(convert_type_basic): Do not apply build_qualified_type for
	TYPE_CODE_STRUCT and TYPE_CODE_UNION.

gdb/testsuite/ChangeLog
2015-04-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	compile: Fix crash on cv-qualified self-reference.
	* gdb.compile/compile.c (struct struct_type): Add volatile for
	selffield.

diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 2b521bc..420f61d 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -166,9 +166,13 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type)
 {
   int i;
   gcc_type result;
+  int quals;
 
   /* First we create the resulting type and enter it into our hash
-     table.  This lets recursive types work.  */
+     table.  This lets recursive types work.  We have to create gcc_type
+     already with its qualifiers to prevent recursively calling
+     build_qualified_type for unfinished TYPE as build_qualified_type
+     creates a copy of the type, remaining unfinished forever.  */
   if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
     result = C_CTX (context)->c_ops->build_record_type (C_CTX (context));
   else
@@ -176,6 +180,15 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type)
       gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION);
       result = C_CTX (context)->c_ops->build_union_type (C_CTX (context));
     }
+  quals = 0;
+  if (TYPE_CONST (type))
+    quals |= GCC_QUALIFIER_CONST;
+  if (TYPE_VOLATILE (type))
+    quals |= GCC_QUALIFIER_VOLATILE;
+  if (TYPE_RESTRICT (type))
+    quals |= GCC_QUALIFIER_RESTRICT;
+  result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context),
+							 result, quals);
   insert_type (context, type, result);
 
   for (i = 0; i < TYPE_NFIELDS (type); ++i)
@@ -329,10 +342,13 @@ static gcc_type
 convert_type_basic (struct compile_c_instance *context, struct type *type)
 {
   /* If we are converting a qualified type, first convert the
-     unqualified type and then apply the qualifiers.  */
+     unqualified type and then apply the qualifiers, except for the
+     types handling qualifiers on their own.  */
   if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
 				     | TYPE_INSTANCE_FLAG_VOLATILE
-				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
+				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0
+      && (TYPE_CODE (type) != TYPE_CODE_STRUCT
+	  && TYPE_CODE (type) != TYPE_CODE_UNION))
     return convert_qualified (context, type);
 
   switch (TYPE_CODE (type))
diff --git a/gdb/testsuite/gdb.compile/compile.c b/gdb/testsuite/gdb.compile/compile.c
index 3d5f20a..41ff087 100644
--- a/gdb/testsuite/gdb.compile/compile.c
+++ b/gdb/testsuite/gdb.compile/compile.c
@@ -42,7 +42,7 @@ struct struct_type {
   float floatfield;
   double doublefield;
   const union union_type *ptrfield;
-  struct struct_type *selffield;
+  volatile struct struct_type *selffield;
   int arrayfield[5];
   _Complex double complexfield;
   _Bool boolfield;

[patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

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

On Sat, 18 Apr 2015 19:28:43 +0200, Jan Kratochvil wrote:
Hi,

with this modified testcase GDB would:

compile code struct_object.selffield = &struct_object
./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error)

While the insert_type() assertion looks unclear trying to fix it one ends up
with either GCC crash
	[gcc libcc1] build_qualified_type for self-referencing/incomplete types
	https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html
	c_incomplete_type_error()
or after fixing up the GCC type for proper error reporting one gets:
	gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’
which is the real culprit of this bug as explained in this patch.

This patch is related to the XFAIL introduced by
	[PATCH v3 5/9] compile: Use -Wall, not -w
	https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html
as for proper -Wall happiness the 'volatile' qualifier needs to be added there
- but adding the qualifier has caused this crash.

No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu.


Thanks,
Jan

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

gdb/ChangeLog
2015-05-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	compile: Fix crash on cv-qualified self-reference.
	* compile/compile-c-types.c (convert_struct_or_union): Apply
	build_qualified_type.
	(convert_type_basic): Do not apply build_qualified_type for
	TYPE_CODE_STRUCT and TYPE_CODE_UNION.

gdb/testsuite/ChangeLog
2015-05-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	compile: Fix crash on cv-qualified self-reference.
	* gdb.compile/compile.c (struct struct_type): Add volatile for
	selffield.
	* gdb.compile/compile.exp
	(compile code struct_object.selffield = &struct_object): Remove XFAIL.

diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 2b521bc..420f61d 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -166,9 +166,13 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type)
 {
   int i;
   gcc_type result;
+  int quals;
 
   /* First we create the resulting type and enter it into our hash
-     table.  This lets recursive types work.  */
+     table.  This lets recursive types work.  We have to create gcc_type
+     already with its qualifiers to prevent recursively calling
+     build_qualified_type for unfinished TYPE as build_qualified_type
+     creates a copy of the type, remaining unfinished forever.  */
   if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
     result = C_CTX (context)->c_ops->build_record_type (C_CTX (context));
   else
@@ -176,6 +180,15 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type)
       gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION);
       result = C_CTX (context)->c_ops->build_union_type (C_CTX (context));
     }
+  quals = 0;
+  if (TYPE_CONST (type))
+    quals |= GCC_QUALIFIER_CONST;
+  if (TYPE_VOLATILE (type))
+    quals |= GCC_QUALIFIER_VOLATILE;
+  if (TYPE_RESTRICT (type))
+    quals |= GCC_QUALIFIER_RESTRICT;
+  result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context),
+							 result, quals);
   insert_type (context, type, result);
 
   for (i = 0; i < TYPE_NFIELDS (type); ++i)
@@ -329,10 +342,13 @@ static gcc_type
 convert_type_basic (struct compile_c_instance *context, struct type *type)
 {
   /* If we are converting a qualified type, first convert the
-     unqualified type and then apply the qualifiers.  */
+     unqualified type and then apply the qualifiers, except for the
+     types handling qualifiers on their own.  */
   if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
 				     | TYPE_INSTANCE_FLAG_VOLATILE
-				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
+				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0
+      && (TYPE_CODE (type) != TYPE_CODE_STRUCT
+	  && TYPE_CODE (type) != TYPE_CODE_UNION))
     return convert_qualified (context, type);
 
   switch (TYPE_CODE (type))
diff --git a/gdb/testsuite/gdb.compile/compile.c b/gdb/testsuite/gdb.compile/compile.c
index 3d5f20a..41ff087 100644
--- a/gdb/testsuite/gdb.compile/compile.c
+++ b/gdb/testsuite/gdb.compile/compile.c
@@ -42,7 +42,7 @@ struct struct_type {
   float floatfield;
   double doublefield;
   const union union_type *ptrfield;
-  struct struct_type *selffield;
+  volatile struct struct_type *selffield;
   int arrayfield[5];
   _Complex double complexfield;
   _Bool boolfield;
diff --git a/gdb/testsuite/gdb.compile/compile.exp b/gdb/testsuite/gdb.compile/compile.exp
index 07276bd..9668be8 100644
--- a/gdb/testsuite/gdb.compile/compile.exp
+++ b/gdb/testsuite/gdb.compile/compile.exp
@@ -189,15 +189,7 @@ gdb_test "p localvar" " = 1"
 # Test setting fields and also many different types.
 #
 
-set test "compile code struct_object.selffield = &struct_object"
-gdb_test_multiple $test $test {
-    -re "^$test\r\n$gdb_prompt $" {
-	pass "$test"
-    }
-    -re "gdb command line:1:25: warning: assignment discards 'volatile' qualifier from pointer target type \\\[-Wdiscarded-qualifiers\\\]\r\n$gdb_prompt $" {
-	xfail "$test (PR compile/18202)"
-    }
-}
+gdb_test_no_output "compile code struct_object.selffield = &struct_object"
 gdb_test "print struct_object.selffield == &struct_object" " = 1"
 
 gdb_test_no_output "compile code struct_object.charfield = 1"

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Joel Brobecker]

Would anyone be in a better position to review this patch?
It's one patch identified as a blocking issue before we create
the GDB 7.10 branch. It looks reasonable to me, but I can only
do a superficial review...

Thanks!

On Sat, May 16, 2015 at 03:25:55PM +0200, Jan Kratochvil wrote:
> On Sat, 18 Apr 2015 19:28:43 +0200, Jan Kratochvil wrote:
> Hi,
> 
> with this modified testcase GDB would:
> 
> compile code struct_object.selffield = &struct_object
> ./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error)
> 
> While the insert_type() assertion looks unclear trying to fix it one ends up
> with either GCC crash
> 	[gcc libcc1] build_qualified_type for self-referencing/incomplete types
> 	https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html
> 	c_incomplete_type_error()
> or after fixing up the GCC type for proper error reporting one gets:
> 	gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’
> which is the real culprit of this bug as explained in this patch.
> 
> This patch is related to the XFAIL introduced by
> 	[PATCH v3 5/9] compile: Use -Wall, not -w
> 	https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html
> as for proper -Wall happiness the 'volatile' qualifier needs to be added there
> - but adding the qualifier has caused this crash.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu.
> 
> 
> Thanks,
> Jan

> gdb/ChangeLog
> 2015-05-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	compile: Fix crash on cv-qualified self-reference.
> 	* compile/compile-c-types.c (convert_struct_or_union): Apply
> 	build_qualified_type.
> 	(convert_type_basic): Do not apply build_qualified_type for
> 	TYPE_CODE_STRUCT and TYPE_CODE_UNION.
> 
> gdb/testsuite/ChangeLog
> 2015-05-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	compile: Fix crash on cv-qualified self-reference.
> 	* gdb.compile/compile.c (struct struct_type): Add volatile for
> 	selffield.
> 	* gdb.compile/compile.exp
> 	(compile code struct_object.selffield = &struct_object): Remove XFAIL.
> 
> diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
> index 2b521bc..420f61d 100644
> --- a/gdb/compile/compile-c-types.c
> +++ b/gdb/compile/compile-c-types.c
> @@ -166,9 +166,13 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type)
>  {
>    int i;
>    gcc_type result;
> +  int quals;
>  
>    /* First we create the resulting type and enter it into our hash
> -     table.  This lets recursive types work.  */
> +     table.  This lets recursive types work.  We have to create gcc_type
> +     already with its qualifiers to prevent recursively calling
> +     build_qualified_type for unfinished TYPE as build_qualified_type
> +     creates a copy of the type, remaining unfinished forever.  */
>    if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
>      result = C_CTX (context)->c_ops->build_record_type (C_CTX (context));
>    else
> @@ -176,6 +180,15 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type)
>        gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION);
>        result = C_CTX (context)->c_ops->build_union_type (C_CTX (context));
>      }
> +  quals = 0;
> +  if (TYPE_CONST (type))
> +    quals |= GCC_QUALIFIER_CONST;
> +  if (TYPE_VOLATILE (type))
> +    quals |= GCC_QUALIFIER_VOLATILE;
> +  if (TYPE_RESTRICT (type))
> +    quals |= GCC_QUALIFIER_RESTRICT;
> +  result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context),
> +							 result, quals);
>    insert_type (context, type, result);
>  
>    for (i = 0; i < TYPE_NFIELDS (type); ++i)
> @@ -329,10 +342,13 @@ static gcc_type
>  convert_type_basic (struct compile_c_instance *context, struct type *type)
>  {
>    /* If we are converting a qualified type, first convert the
> -     unqualified type and then apply the qualifiers.  */
> +     unqualified type and then apply the qualifiers, except for the
> +     types handling qualifiers on their own.  */
>    if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
>  				     | TYPE_INSTANCE_FLAG_VOLATILE
> -				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
> +				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0
> +      && (TYPE_CODE (type) != TYPE_CODE_STRUCT
> +	  && TYPE_CODE (type) != TYPE_CODE_UNION))
>      return convert_qualified (context, type);
>  
>    switch (TYPE_CODE (type))
> diff --git a/gdb/testsuite/gdb.compile/compile.c b/gdb/testsuite/gdb.compile/compile.c
> index 3d5f20a..41ff087 100644
> --- a/gdb/testsuite/gdb.compile/compile.c
> +++ b/gdb/testsuite/gdb.compile/compile.c
> @@ -42,7 +42,7 @@ struct struct_type {
>    float floatfield;
>    double doublefield;
>    const union union_type *ptrfield;
> -  struct struct_type *selffield;
> +  volatile struct struct_type *selffield;
>    int arrayfield[5];
>    _Complex double complexfield;
>    _Bool boolfield;
> diff --git a/gdb/testsuite/gdb.compile/compile.exp b/gdb/testsuite/gdb.compile/compile.exp
> index 07276bd..9668be8 100644
> --- a/gdb/testsuite/gdb.compile/compile.exp
> +++ b/gdb/testsuite/gdb.compile/compile.exp
> @@ -189,15 +189,7 @@ gdb_test "p localvar" " = 1"
>  # Test setting fields and also many different types.
>  #
>  
> -set test "compile code struct_object.selffield = &struct_object"
> -gdb_test_multiple $test $test {
> -    -re "^$test\r\n$gdb_prompt $" {
> -	pass "$test"
> -    }
> -    -re "gdb command line:1:25: warning: assignment discards 'volatile' qualifier from pointer target type \\\[-Wdiscarded-qualifiers\\\]\r\n$gdb_prompt $" {
> -	xfail "$test (PR compile/18202)"
> -    }
> -}
> +gdb_test_no_output "compile code struct_object.selffield = &struct_object"
>  gdb_test "print struct_object.selffield == &struct_object" " = 1"
>  
>  gdb_test_no_output "compile code struct_object.charfield = 1"


-- 
Joel

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Phil Muldoon]

On 30/06/15 20:57, Joel Brobecker wrote:
> Would anyone be in a better position to review this patch?
> It's one patch identified as a blocking issue before we create
> the GDB 7.10 branch. It looks reasonable to me, but I can only
> do a superficial review...
>
>
I think its fine, but no, not really. It's just Jan and I on the GDB side of the compile command.

A maintainer will have to sign off on it.

Cheers

Phil

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 05/16/2015 02:25 PM, Jan Kratochvil wrote:
> On Sat, 18 Apr 2015 19:28:43 +0200, Jan Kratochvil wrote:
> Hi,
> 
> with this modified testcase GDB would:
> 
> compile code struct_object.selffield = &struct_object
> ./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error)
> 
> While the insert_type() assertion looks unclear trying to fix it one ends up
> with either GCC crash
> 	[gcc libcc1] build_qualified_type for self-referencing/incomplete types
> 	https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html
> 	c_incomplete_type_error()
> or after fixing up the GCC type for proper error reporting one gets:
> 	gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’
> which is the real culprit of this bug as explained in this patch.
> 
> This patch is related to the XFAIL introduced by
> 	[PATCH v3 5/9] compile: Use -Wall, not -w
> 	https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html
> as for proper -Wall happiness the 'volatile' qualifier needs to be added there
> - but adding the qualifier has caused this crash.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu.

OK.  Sorry for the delay.

Thanks,
Pedro Alves

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> +  quals = 0;
> +  if (TYPE_CONST (type))
> +    quals |= GCC_QUALIFIER_CONST;
> +  if (TYPE_VOLATILE (type))
> +    quals |= GCC_QUALIFIER_VOLATILE;
> +  if (TYPE_RESTRICT (type))
> +    quals |= GCC_QUALIFIER_RESTRICT;
> +  result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context),
> +							 result, quals);
>    insert_type (context, type, result);

Can we use convert_qualified instead?  I find code we added here is
quite similar to convert_qualified.

>  
>    for (i = 0; i < TYPE_NFIELDS (type); ++i)
> @@ -329,10 +342,13 @@ static gcc_type
>  convert_type_basic (struct compile_c_instance *context, struct type *type)
>  {
>    /* If we are converting a qualified type, first convert the
> -     unqualified type and then apply the qualifiers.  */
> +     unqualified type and then apply the qualifiers, except for the
> +     types handling qualifiers on their own.  */
>    if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
>  				     | TYPE_INSTANCE_FLAG_VOLATILE
> -				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
> +				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0
> +      && (TYPE_CODE (type) != TYPE_CODE_STRUCT
> +	  && TYPE_CODE (type) != TYPE_CODE_UNION))
>      return convert_qualified (context, type);

It looks right to me, however, isn't cleaner to do in this way?

   /* Comments on why we do this first */
   if (TYPE_CODE (type) == TYPE_CODE_STRUCT
       || TYPE_CODE (type) == TYPE_CODE_UNION)
       return convert_struct_or_union (context, type);

  /* If we are converting a qualified type, first convert the
     unqualified type and then apply the qualifiers.  */
  if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
				     | TYPE_INSTANCE_FLAG_VOLATILE
				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
    return convert_qualified (context, type);

  switch (TYPE_CODE (type))
   {
      /* Don't need to handle TYPE_CODE_STRUCT and TYPE_CODE_UNION
         here.  */
   }

Otherwise, the patch looks good to me.

-- 
Yao (齐尧)

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 01 Jul 2015 13:21:32 +0200, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> > +  quals = 0;
> > +  if (TYPE_CONST (type))
> > +    quals |= GCC_QUALIFIER_CONST;
> > +  if (TYPE_VOLATILE (type))
> > +    quals |= GCC_QUALIFIER_VOLATILE;
> > +  if (TYPE_RESTRICT (type))
> > +    quals |= GCC_QUALIFIER_RESTRICT;
> > +  result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context),
> > +							 result, quals);
> >    insert_type (context, type, result);
> 
> Can we use convert_qualified instead?  I find code we added here is
> quite similar to convert_qualified.

Not directly to use convert_qualified() but convert_qualified() could be split
in halves and one half could be used here, I agree.


> >    for (i = 0; i < TYPE_NFIELDS (type); ++i)
> > @@ -329,10 +342,13 @@ static gcc_type
> >  convert_type_basic (struct compile_c_instance *context, struct type *type)
> >  {
> >    /* If we are converting a qualified type, first convert the
> > -     unqualified type and then apply the qualifiers.  */
> > +     unqualified type and then apply the qualifiers, except for the
> > +     types handling qualifiers on their own.  */
> >    if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
> >  				     | TYPE_INSTANCE_FLAG_VOLATILE
> > -				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
> > +				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0
> > +      && (TYPE_CODE (type) != TYPE_CODE_STRUCT
> > +	  && TYPE_CODE (type) != TYPE_CODE_UNION))
> >      return convert_qualified (context, type);
> 
> It looks right to me, however, isn't cleaner to do in this way?
> 
>    /* Comments on why we do this first */
>    if (TYPE_CODE (type) == TYPE_CODE_STRUCT
>        || TYPE_CODE (type) == TYPE_CODE_UNION)
>        return convert_struct_or_union (context, type);
> 
>   /* If we are converting a qualified type, first convert the
>      unqualified type and then apply the qualifiers.  */
>   if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
> 				     | TYPE_INSTANCE_FLAG_VOLATILE
> 				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
>     return convert_qualified (context, type);
> 
>   switch (TYPE_CODE (type))
>    {
>       /* Don't need to handle TYPE_CODE_STRUCT and TYPE_CODE_UNION
>          here.  */
>    }

I can change it that way but when you ask "isn't cleaner" then no, I think
your hack is even a bit more ugly than my ugly hack.

There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))'
and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by
TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though).


> Otherwise, the patch looks good to me.

OK, thanks.  Just it causes a regression with latest GCC now as I have asked
Alexandre Oliva off-list how it really should be fixed.


Jan

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 07/01/2015 02:24 PM, Jan Kratochvil wrote:

> I can change it that way but when you ask "isn't cleaner" then no, I think
> your hack is even a bit more ugly than my ugly hack.
> 
> There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))'
> and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by
> TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though).

What would be the method name?  There's nothing
preventing adding a new type_FOO function that takes a type pointer as
parameter and hides the TYPE_CODE checks inside.  From the caller's
perspective, it'll be the same.  Once we get to C++ and if we consider
objectifying type, then converting that function to a method will
be trivial.

Thanks,
Pedro Alves

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> I can change it that way but when you ask "isn't cleaner" then no, I think
> your hack is even a bit more ugly than my ugly hack.
>

OK, that is sort of personal preference, that is fine to me to leave it
as is.

> There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))'
> and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by
> TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though).

If we change to virtual methods, we don't need "switch" at all.  Each type
can have its virtual method to handle this separately.

-- 
Yao (齐尧)

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 01 Jul 2015 15:54:21 +0200, Pedro Alves wrote:
> On 07/01/2015 02:24 PM, Jan Kratochvil wrote:
> 
> > I can change it that way but when you ask "isn't cleaner" then no, I think
> > your hack is even a bit more ugly than my ugly hack.
> > 
> > There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))'
> > and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by
> > TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though).
> 
> What would be the method name?

class Type {
protected:
  virtual GccType convert_unqualified ()=0;
public:
  explicit virtual GccType operator() {
    if (instance_flags==0) return convert_unqualified();
    ...
  }
};

class StructType:public Type {
protected:
  virtual GccType convert_unqualified () { assert(0) }
public:
  explicit virtual GccType operator() override { ... }
};

class IntegerType:public Type {
protected:
  virtual GccType convert_unqualified () { assert(instance_flags==0); ... }
};

Althoughth qualifications could be possibly also subclassed which would look
differently again.

My point was that the current code does not make much sense to clean up as it
cannot be clean in C.


> There's nothing preventing adding a new type_FOO function that takes a type
> pointer as parameter and hides the TYPE_CODE checks inside.  From the
> caller's perspective, it'll be the same.  Once we get to C++ and if we
> consider objectifying type, then converting that function to a method will
> be trivial.

Do you mean simulation of C++ virtual method table by a struct of pointers,
like in other cases in GDB?


Jan

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 07/01/2015 03:10 PM, Jan Kratochvil wrote:
> On Wed, 01 Jul 2015 15:54:21 +0200, Pedro Alves wrote:
>> > On 07/01/2015 02:24 PM, Jan Kratochvil wrote:
>> > 
>>> > > I can change it that way but when you ask "isn't cleaner" then no, I think
>>> > > your hack is even a bit more ugly than my ugly hack.
>>> > > 
>>> > > There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))'
>>> > > and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by
>>> > > TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though).
>> > 
>> > What would be the method name?

> class Type {
> protected:
>   virtual GccType convert_unqualified ()=0;
> public:
>   explicit virtual GccType operator() {
>     if (instance_flags==0) return convert_unqualified();
>     ...
>   }
> };
> 
> class StructType:public Type {
> protected:
>   virtual GccType convert_unqualified () { assert(0) }
> public:
>   explicit virtual GccType operator() override { ... }
> };
> 
> class IntegerType:public Type {
> protected:
>   virtual GccType convert_unqualified () { assert(instance_flags==0); ... }
> };
> 

Well, I'd say that having the core GDB Type be aware of GccType
directly would be a misdesign, not a feature.

> Althoughth qualifications could be possibly also subclassed which would look
> differently again.

Yes, the "possibly" here is the crux.  Subclassing isn't always the
best choice.  There are trade offs.

>> > There's nothing preventing adding a new type_FOO function that takes a type
>> > pointer as parameter and hides the TYPE_CODE checks inside.  From the
>> > caller's perspective, it'll be the same.  Once we get to C++ and if we
>> > consider objectifying type, then converting that function to a method will
>> > be trivial.
> Do you mean simulation of C++ virtual method table by a struct of pointers,
> like in other cases in GDB?
> 

No.  I meant a straightforward conversion of your C++ methods
to C functions implemented in terms of switch/TYPE_CODE.

IIUC, in your C++ class tree you'd do:

 gcctype = (GccType) type;


So a trivial 1-1 conversion or your code would be:

// Type::convert_unqualified ()
// StructType::convert_unqualified ()

gcc_type
type_convert_unqualified (struct type *)
{
  switch (TYPE_CODE (type))
   {
      case TYPE_CODE_STRUCT:
         assert(0);
      default:
         ...
   }
}

// Type::GccType operator()

gcc_type
type_as_gcc_type (struct type *type)
{
  if (TYPE_INSTANCE_FLAGS (instance_flags) == 0)
    return type_convert_unqualified (type);
  ...
}

Then the caller in question would use:

 gcctype = type_as_gcc_type (type);


I'm very much looking forward to C++ as well, but switch/case vs
virtual methods here is really more about syntax sugar than design.
You can easily end up with a broken class inheritance tree
just as well.  There's a lot more to it beyond design than language.

Thanks,
Pedro Alves

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 01 Jul 2015 16:59:37 +0200, Pedro Alves wrote:
> Well, I'd say that having the core GDB Type be aware of GccType
> directly would be a misdesign, not a feature.

You are right, it would be reversed, anyway not the insance crap of
TYPE_CODE_*.


> So a trivial 1-1 conversion or your code would be:

I find it complicates the code even more without C++.


> I'm very much looking forward to C++ as well,

So why that does not happen?  You have added a disadvantage of more
requirements for the code compliance with the new exception macros but there
is still no advantage from that.


> but switch/case vs
> virtual methods here is really more about syntax sugar than design.

C++ may look so to someone but is not just a syntax sugar.


Jan

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 07/01/2015 04:12 PM, Jan Kratochvil wrote:

>> I'm very much looking forward to C++ as well,
> 
> So why that does not happen?

For myself, just lack of time ATM.  I've called out for help before.
Yao helped a bit.  The current status is in the wiki page.

 https://sourceware.org/gdb/wiki/cxx-conversion

Basically the next step is tacking patches from:

 git@github.com:palves/gdb.git palves/cxx-conversion-attempt-part-2-no-fpermissive

and polishing/submitting them upstream.

> You have added a disadvantage of more
> requirements for the code compliance with the new exception macros but there
> is still no advantage from that.

Why would you say that...  I don't see what disadvantage you talk about.

Thanks,
Pedro Alves

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 01 Jul 2015 17:24:38 +0200, Pedro Alves wrote:
> For myself, just lack of time ATM.  I've called out for help before.
> Yao helped a bit.  The current status is in the wiki page.
> 
>  https://sourceware.org/gdb/wiki/cxx-conversion
> 
> Basically the next step is tacking patches from:
> 
>  git@github.com:palves/gdb.git palves/cxx-conversion-attempt-part-2-no-fpermissive
> 
> and polishing/submitting them upstream.

OK, thanks for the status updated.


> > You have added a disadvantage of more
> > requirements for the code compliance with the new exception macros but there
> > is still no advantage from that.
> 
> Why would you say that...  I don't see what disadvantage you talk about.

From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches.
When GDB codebase finally gets compiled by clang++ I agree it will be an
improvement.  I see there are no real roadblocks, just the time availability.


Thanks,
Jan

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 07/01/2015 04:29 PM, Jan Kratochvil wrote:
> From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches.
> When GDB codebase finally gets compiled by clang++ I agree it will be an
> improvement.  

Seriously, that's a depressing and demotivating comment.
You want it done, but you also want it not done.

Thanks,
Pedro Alves

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 01 Jul 2015 17:35:10 +0200, Pedro Alves wrote:
> On 07/01/2015 04:29 PM, Jan Kratochvil wrote:
> > From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches.
> > When GDB codebase finally gets compiled by clang++ I agree it will be an
> > improvement.  
> 
> Seriously, that's a depressing and demotivating comment.
> You want it done, but you also want it not done.

Do you mean the first sentence or the second sentence?

I agree the first sentence is demotivating but I feel the same spending a lot
of time rebasing patches again and again on changes that end up half-way or
that are questionable whether they really cleaned up the code.  Although
specifically for this TRY_CATCH->TRY case I had fortunately only two such
cases.

For the second sentence I hope everybody - and at least Google - agrees, that
G++ is not usable for C++ development as in many cases particularly involving
templates the error messages only say something is wrong in the source,
without giving the invalid source line number.  I was bisecting the source
before I found clang++ can just report the invalid code location.  I have
filed many diagnostics PRs for GCC but almost none of them are fixed yet.
G++ is great to get a bit better performing final code after all the
compilation errors are resolved by clang++.


Jan

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 07/01/2015 05:07 PM, Jan Kratochvil wrote:
> On Wed, 01 Jul 2015 17:35:10 +0200, Pedro Alves wrote:
>> On 07/01/2015 04:29 PM, Jan Kratochvil wrote:
>>> From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches.
>>> When GDB codebase finally gets compiled by clang++ I agree it will be an
>>> improvement.  
>>
>> Seriously, that's a depressing and demotivating comment.
>> You want it done, but you also want it not done.
> 
> Do you mean the first sentence or the second sentence?
> 

The whole of it.  If we had converted to pure C++ in one go, you'd have
had to revolve the same conflicts too.  And the conflicts resulting from
the final conversion from TRY/CATCH to native try/catch will be super
trivial to resolve, as that won't move code around.

> I agree the first sentence is demotivating but I feel the same spending a lot
> of time rebasing patches again and again on changes that end up half-way or
> that are questionable whether they really cleaned up the code.  Although
> specifically for this TRY_CATCH->TRY case I had fortunately only two such
> cases.

And still, you complain.

This is truly pointless.  Sorry, I'm out of here.

Thanks,
Pedro Alves

Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 01 Jul 2015 15:24:06 +0200, Jan Kratochvil wrote:
> Just it causes a regression with latest GCC now as I have asked
> Alexandre Oliva off-list how it really should be fixed.

I have removed requirement of this fix for the branching as there is currently
no valid fix at hand and after all the 'compile' feature probably should not
be a release blocker.

Still hopefully it gets fixed before the 7.10 release.


Jan

[patchv3] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

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

On Thu, 02 Jul 2015 14:34:01 +0200, Jan Kratochvil wrote:
> On Wed, 01 Jul 2015 15:24:06 +0200, Jan Kratochvil wrote:
> > Just it causes a regression with latest GCC now as I have asked
> > Alexandre Oliva off-list how it really should be fixed.
> 
> I have removed requirement of this fix for the branching as there is currently
> no valid fix at hand and after all the 'compile' feature probably should not
> be a release blocker.

So the bug was not in GDB but in the GCC part interfacing with GDB.

Alexandre Oliva has fixed it the right way:
	https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;a=commitdiff;h=072dfdba0ea62abb65514cb3a90cdf3868efe286
	git://gcc.gnu.org/git/gcc.git
	aoliva/libcp1

Attaching this GDB testsuite update + info to user s/he should upgrade GCC.
After Alex upstreams the fix I can update the message to contain the specific
GCC release.


Jan

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

gdb/ChangeLog
2015-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR compile/18484
	* compile/compile-c-types.c (insert_type): Change gdb_assert to error.

gdb/testsuite/ChangeLog
2015-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR compile/18484
	* gdb.compile/compile.c (struct struct_type): Add volatile to
	selffield's type.
	* gdb.compile/compile.exp
	(compile code struct_object.selffield = &struct_object): Skip futher
	struct_object tests if this one xfails.

diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 22f5a9d..1ad3dd9 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -78,7 +78,8 @@ insert_type (struct compile_c_instance *context, struct type *type,
   add = *slot;
   /* The type might have already been inserted in order to handle
      recursive types.  */
-  gdb_assert (add == NULL || add->gcc_type == gcc_type);
+  if (add != NULL && add->gcc_type != gcc_type)
+    error (_("Unexpected type id from GCC, check you use recent enough GCC."));
 
   if (add == NULL)
     {
diff --git a/gdb/testsuite/gdb.compile/compile.c b/gdb/testsuite/gdb.compile/compile.c
index 3d5f20a..41ff087 100644
--- a/gdb/testsuite/gdb.compile/compile.c
+++ b/gdb/testsuite/gdb.compile/compile.c
@@ -42,7 +42,7 @@ struct struct_type {
   float floatfield;
   double doublefield;
   const union union_type *ptrfield;
-  struct struct_type *selffield;
+  volatile struct struct_type *selffield;
   int arrayfield[5];
   _Complex double complexfield;
   _Bool boolfield;
diff --git a/gdb/testsuite/gdb.compile/compile.exp b/gdb/testsuite/gdb.compile/compile.exp
index dd46a5f..a416e9e 100644
--- a/gdb/testsuite/gdb.compile/compile.exp
+++ b/gdb/testsuite/gdb.compile/compile.exp
@@ -189,51 +189,61 @@ gdb_test "p localvar" " = 1"
 # Test setting fields and also many different types.
 #
 
+set skip_struct_object 0
 set test "compile code struct_object.selffield = &struct_object"
 gdb_test_multiple $test $test {
     -re "^$test\r\n$gdb_prompt $" {
 	pass "$test"
     }
-    -re "gdb command line:1:25: warning: assignment discards 'volatile' qualifier from pointer target type \\\[-Wdiscarded-qualifiers\\\]\r\n$gdb_prompt $" {
+    -re " error: Unexpected type id from GCC, check you use recent enough GCC\\.\r\n.*\r\n$gdb_prompt $" {
 	xfail "$test (PR compile/18202)"
+
+	# All following tests will break with the same error message.
+	set skip_struct_object 1
     }
 }
-gdb_test "print struct_object.selffield == &struct_object" " = 1"
-
-gdb_test_no_output "compile code struct_object.charfield = 1"
-gdb_test "print struct_object.charfield" " = 1 '\\\\001'"
-gdb_test_no_output "compile code struct_object.ucharfield = 1"
-gdb_test "print struct_object.ucharfield" " = 1 '\\\\001'"
-
-foreach {field value} {
-    shortfield -5
-    ushortfield 5
-    intfield -7
-    uintfield 7
-    bitfield 2
-    longfield -9
-    ulongfield 9
-    enumfield ONE
-    floatfield 1
-    doublefield 2
-} {
-    gdb_test_no_output "compile code struct_object.$field = $value"
-    gdb_test "print struct_object.$field" " = $value"
-}
 
-gdb_test_no_output "compile code struct_object.arrayfield\[2\] = 7"
-gdb_test "print struct_object.arrayfield" \
-    " = \\{0, 0, 7, 0, 0\\}"
+if {$skip_struct_object} {
+    untested "all struct_object tests"
+} else {
+    gdb_test "print struct_object.selffield == &struct_object" " = 1"
+
+    gdb_test_no_output "compile code struct_object.charfield = 1"
+    gdb_test "print struct_object.charfield" " = 1 '\\\\001'"
+    gdb_test_no_output "compile code struct_object.ucharfield = 1"
+    gdb_test "print struct_object.ucharfield" " = 1 '\\\\001'"
+
+    foreach {field value} {
+	shortfield -5
+	ushortfield 5
+	intfield -7
+	uintfield 7
+	bitfield 2
+	longfield -9
+	ulongfield 9
+	enumfield ONE
+	floatfield 1
+	doublefield 2
+    } {
+	gdb_test_no_output "compile code struct_object.$field = $value"
+	gdb_test "print struct_object.$field" " = $value"
+    }
 
-gdb_test_no_output "compile code struct_object.complexfield = 7 + 5i"
-gdb_test "print struct_object.complexfield" " = 7 \\+ 5 \\* I"
+    gdb_test_no_output "compile code struct_object.arrayfield\[2\] = 7"
+    gdb_test "print struct_object.arrayfield" \
+	" = \\{0, 0, 7, 0, 0\\}"
 
-gdb_test_no_output "compile code struct_object.boolfield = 1"
-gdb_test "print struct_object.boolfield" " = true"
+    gdb_test_no_output "compile code struct_object.complexfield = 7 + 5i"
+    gdb_test "print struct_object.complexfield" " = 7 \\+ 5 \\* I"
 
-gdb_test_no_output "compile code struct_object.vectorfield\[2\] = 7"
-gdb_test "print struct_object.vectorfield" \
-    " = \\{0, 0, 7, 0\\}"
+    gdb_test_no_output "compile code struct_object.boolfield = 1"
+    gdb_test "print struct_object.boolfield" " = true"
+
+    gdb_test_no_output "compile code struct_object.vectorfield\[2\] = 7"
+    gdb_test "print struct_object.vectorfield" \
+	" = \\{0, 0, 7, 0\\}"
+
+}
 
 gdb_test_no_output "compile code union_object.typedeffield = 7"
 gdb_test "print union_object.typedeffield" " = 7"

Re: [patchv3] compile: Fix crash on cv-qualified self-reference

[Pedro Alves]

On 07/04/2015 06:11 PM, Jan Kratochvil wrote:

> gdb/ChangeLog
> 2015-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	PR compile/18484
> 	* compile/compile-c-types.c (insert_type): Change gdb_assert to error.
> 
> gdb/testsuite/ChangeLog
> 2015-07-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	PR compile/18484
> 	* gdb.compile/compile.c (struct struct_type): Add volatile to
> 	selffield's type.
> 	* gdb.compile/compile.exp
> 	(compile code struct_object.selffield = &struct_object): Skip futher
> 	struct_object tests if this one xfails.

typo: "further"

OK.

Thanks,
Pedro Alves

[commit] [patchv3] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil]

On Wed, 08 Jul 2015 11:28:55 +0200, Pedro Alves wrote:
> typo: "further"

Fixed.

> OK.

Checked in:
	084641963dba63283bf2eca227f4f77c2598b172
7.10 branch:
	91affdd27ee8e244df417686ead2b546ee1449bf


Thanks,
Jan