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

Joel Brobecker
2015-06-30T19:57:13Z

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