Patchwork Mark some tests as XFAIL/UNSUPPORTED hidden due to GCC's omission of typedefs in inheritance.

login
register
mail settings
Submitter David Blaikie
Date April 25, 2014, 2:48 a.m.
Message ID <CAENS6EvDSA7+GAa9kVFfQVv5DA8dcC4fDKM7m3_wQ1_mFo_FiA@mail.gmail.com>
Download mbox | patch
Permalink /patch/673/
State Under Review
Headers show

Comments

David Blaikie - April 25, 2014, 2:48 a.m.
On Wed, Apr 23, 2014 at 3:46 PM, Doug Evans <dje@google.com> wrote:
> David Blaikie writes:
>  > gdb.cp/impl-this.exp is testing the ability to scope names within a
>  > class and includes cases where the base class was specified via a
>  > typedef.
>  >
>  > Due to GCC's PR14819 these tests weren't actually testing this case -
>  > GCC produces the same debug info regardless of whether there's a
>  > typedef used in the base specifier.
>  >
>  > Clang correctly produces the typedef debug info for the base type and
>  > exposes a variety of failures/limitations in these test cases.
>  >
>  > The attached patch updates the tests to flag these cases as
>  > unsupported under GCC and xfail as appropriate under Clang.
>  > commit 7fa92f9a15f440129dd5a989511f3bbda646afa5
>  > Author: David Blaikie <dblaikie@gmail.com>
>  > Date:   Sun Apr 13 11:15:44 2014 -0700
>  >
>  >     Mark some tests as XFAIL/UNSUPPORTED hidden due to GCC's omission of typedefs in inheritance.
>  >
>  >     gdb/testsuite/
>  >      * gdb.cp/impl-this.exp: Mark several tests XFAIL/UNSUPPORTED due to
>  >      PR16841 and GCC PR60833
>
> There are several calls to setup_kfail passing gdb/x and gdb/y.
> x and y need to be fixed.

Ah, right, sorry - jumbled up a few bug numbers. The GDB bug is
PR16841, I've updated all the kfails (both 'x' and 'y' - at first I'd
thought they might be separate bugs, but I'm guessing they're the same
one - I've mentioned the 'y' case down the bottom of PR16841,
manifesting as "Cannot access memory at address 0x0") to refer to that
GDB PR.

> According to gdb/testsuite/README I think (!) setup_xfail should
> be used here instead of setup_kfail, since the problem originates
> with gcc.
>
> kfail = known gdb problem
> xfail = known environment problem (e.g. compiler)

Yep, that's my understanding as well - it's just that in this case
these are actually GDB problems, so far as I can see, and should
correctly be marked as KFAILs. Based on Pedro Alves's suggestion I've
added some more of the detail that I mentioned in my first email, to
the commit message (in the attached updated patch).

The GCC bug is exposed in the suite as UNSUPPORTED - since GCC doesn't
use the typedef in the inheritance, GCC doesn't actually exercise this
part of the compiler. The non-typedef'd base tests can be exercised
with either Clang or GCC's output, but since GCC doesn't use the
typedef in the inheritance, testing that in GDB won't actually test
anything other than redundantly test the non-typedef path again.

> Ok with those changes.
> [Though I can well imagine I'm missing something.
> This part of the testsuite and c++ is a bit beyond me.]

Let me know if there's some more information you'd like/I can provide.
Happy to try to explain in more detail.

Updated patch attached with
* more detailed commit message
* kfail with GDB bug number (GDB 16841) instead of placeholders
commit bbb659b200a11a81853f1573f69a42e135b37244
Author: David Blaikie <dblaikie@gmail.com>
Date:   Sun Apr 13 11:15:44 2014 -0700

    Due to GCC's PR60833 several tests weren't providing their intended coverage, thus obscuring GDB PR16841
    
    GCC PR60833 causes GCC to emit inheritance by referencing the base type
    directly, omitting any typedefs used in the original source.
    
    This bug makes it impossible to test that GCC can handle this situation
    and the test coverage added in gdb.cp/impl-this.exp was just redundantly
    testing the no-typedef case.
    
    Clang emits and uses the typedef in the inheritance and thus exposes the
    limitation in GDB (filed as PR16841) when encountering this situation.
    
    KFAIL tests under Clang and mark them UNSUPPORTED under GCC (since GCC's
    output cannot doesn't include the intended DWARF necessary to exercise
    GCC's behavior here).
    
    gdb/testsuite/
    	* gdb.cp/impl-this.exp: Mark several tests KFAIL/UNSUPPORTED due to
    	GDB PR16841 and GCC PR60833
Pedro Alves - May 1, 2014, 11:07 a.m.
re-reading this, I actually have comments now.  :-)

On 04/25/2014 03:48 AM, David Blaikie wrote:
> -	"Cannot reference non-static field \"i\""
> -    gdb_test "print B<int>::A<int>::i" "Cannot reference non-static field \"i\""
> +    if {[test_compiler_info {gcc-*-*}]} {
> +        unsupported "gdb/16841"

Making this "unsupported" and skipping the actual tests means that we'll
just end up never ever again testing this against gcc, because I guarantee
you that if gcc changes, nobody will ever remember to adjust this test.
I don't think we should do that.

> Due to GCC's PR14819 these tests weren't actually testing this case -

GCC's PR14819 doesn't seem to be related.  Did you mean some other bug?
Or did you mean _GDB_'s PR14819?

> GCC produces the same debug info regardless of whether there's a
> typedef used in the base specifier.

> +    } else {
> +        setup_kfail gdb/16841 *-*-*
> +        gdb_test "print D::B<int>::i" "Cannot reference non-static field \"i\""
> +        gdb_test "print D::Bint::i" "Cannot reference non-static field \"i\""
> +        gdb_test "print B<int>::i" "Cannot reference non-static field \"i\""
> +        setup_kfail gdb/16841 *-*-*
> +        gdb_test "print D::B<int>::A<int>::i" \
> +

Patch

diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
index 0cd9d83..7e360ae 100644
--- gdb/testsuite/ChangeLog
+++ gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2014-04-24  David Blaikie  <dblaikie@gmail.com>
+
+	* gdb.cp/impl-this.exp: Mark several tests KFAIL/UNSUPPORTED due to
+	GDB PR16841 and GCC PR60833
+
 2014-04-25  Yao Qi  <yao@codesourcery.com>
 
 	* gdb.dwarf2/dwz.exp (Dwarf::assemble): Remove unused
diff --git gdb/testsuite/gdb.cp/impl-this.exp gdb/testsuite/gdb.cp/impl-this.exp
index dd1bc64..3191806 100644
--- gdb/testsuite/gdb.cp/impl-this.exp
+++ gdb/testsuite/gdb.cp/impl-this.exp
@@ -30,13 +30,22 @@  if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
 with_test_prefix "before run" {
     gdb_test "print i" "No symbol \"i\" in current context."
     gdb_test "print D::i" "Cannot reference non-static field \"i\""
-    gdb_test "print D::B<int>::i" "Cannot reference non-static field \"i\""
-    gdb_test "print B<int>::i" "Cannot reference non-static field \"i\""
     gdb_test "print D::C::i" "Cannot reference non-static field \"i\""
     gdb_test "print C::i" "Cannot reference non-static field \"i\""
-    gdb_test "print D::B<int>::A<int>::i" \
-	"Cannot reference non-static field \"i\""
-    gdb_test "print B<int>::A<int>::i" "Cannot reference non-static field \"i\""
+    if {[test_compiler_info {gcc-*-*}]} {
+        unsupported "gdb/16841"
+    } else {
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::i" "Cannot reference non-static field \"i\""
+        gdb_test "print D::Bint::i" "Cannot reference non-static field \"i\""
+        gdb_test "print B<int>::i" "Cannot reference non-static field \"i\""
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::A<int>::i" \
+            "Cannot reference non-static field \"i\""
+        gdb_test "print D::Bint::A<int>::i" \
+            "Cannot reference non-static field \"i\""
+        gdb_test "print B<int>::A<int>::i" "Cannot reference non-static field \"i\""
+    }
     gdb_test "print A<int>::i" "Cannot reference non-static field \"i\""
     gdb_test "print D::C::A<int>::i" "Cannot reference non-static field \"i\""
     gdb_test "print C::A<int>::i" "Cannot reference non-static field \"i\""
@@ -61,18 +70,27 @@  gdb_continue_to_breakpoint "continue to D::f"
 with_test_prefix "at D::f (valid expressions)" {
     gdb_test "print i" "= 4"
     gdb_test "print D::i" "= 4"
-    gdb_test "print D::B<int>::i" "= 2"
-    gdb_test "print B<int>::i" "= 2"
-    gdb_test "print D::Bint::i" \
-	"No type \"Bint\" within class or namespace \"D\"."
-    gdb_test "print Bint::i" "= 2"
+    if {[test_compiler_info {gcc-*-*}]} {
+        unsupported "gdb/16841"
+    } else {
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::i" "= 2"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print B<int>::i" "= 2"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print Bint::i" "= 2"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::Bint::i" "= 2"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print Bint::i" "= 2"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::A<int>::i" "= 1"
+        gdb_test "print B<int>::A<int>::i" "= 1"
+        gdb_test "print D::Bint::A<int>::i" "= 1"
+        gdb_test "print Bint::A<int>::i" "= 1"
+    }
     gdb_test "print D::C::i" "= 3"
     gdb_test "print C::i" "= 3"
-    gdb_test "print D::B<int>::A<int>::i" "= 1"
-    gdb_test "print B<int>::A<int>::i" "= 1"
-    gdb_test "print D::Bint::A<int>::i" \
-	"No type \"Bint\" within class or namespace \"D\"."
-    gdb_test "print Bint::A<int>::i" "= 1"
     gdb_test "print A<int>::i" "= 1"
     gdb_test "print D::C::A<int>::i" "= 1"
     gdb_test "print C::A<int>::i" "= 1"
@@ -86,29 +104,40 @@  with_test_prefix "at D::f (valid expressions)" {
 
 # Test some invalid expressions
 with_test_prefix "at D::f (invalid expressions)" {
-    gdb_test "print D::B<int>::c" "There is no field named c"
-    gdb_test "print D::B<int>::A<int>::c" "There is no field named c"
-    gdb_test "print D::Bint::c" \
-	"No type \"Bint\" within class or namespace \"D\"."
-
-    gdb_test "print D::Bint::A<int>::c" \
-	"No type \"Bint\" within class or namespace \"D\"."
-    gdb_test "print D::C::A<int>::c" "There is no field named c"
-    gdb_test "print B<int>::c" "There is no field named c"
-    gdb_test "print B<int>::A<int>::c" "There is no field named c"
-    gdb_test "print Bint::c" "There is no field named c"
-    gdb_test "print Bint::A<int>::c" "There is no field named c"
+    if {[test_compiler_info {gcc-*-*}]} {
+        unsupported "gdb/16841"
+    } else {
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::c" "There is no field named c"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::A<int>::c" "There is no field named c"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::Bint::c" \
+        "No type \"Bint\" within class or namespace \"D\"."
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::Bint::A<int>::c" \
+        "No type \"Bint\" within class or namespace \"D\"."
+        gdb_test "print B<int>::c" "There is no field named c"
+        gdb_test "print B<int>::A<int>::c" "There is no field named c"
+        gdb_test "print Bint::c" "There is no field named c"
+        gdb_test "print Bint::A<int>::c" "There is no field named c"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::x" "There is no field named x"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::B<int>::A<int>::x" "There is no field named x"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::Bint::x" \
+        "No type \"Bint\" within class or namespace \"D\"."
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print D::Bint::A<int>::x" \
+        "No type \"Bint\" within class or namespace \"D\"."
+        gdb_test "print B<int>::x" "There is no field named x"
+        gdb_test "print B<int>::A<int>::x" "There is no field named x"
+        gdb_test "print Bint::x" "There is no field named x"
+        gdb_test "print Bint::A<int>::x" "There is no field named x"
+    }
     gdb_test "print C::A<int>::c" "There is no field named c"
-    gdb_test "print D::B<int>::x" "There is no field named x"
-    gdb_test "print D::B<int>::A<int>::x" "There is no field named x"
-    gdb_test "print D::Bint::x" \
-	"No type \"Bint\" within class or namespace \"D\"."
-    gdb_test "print D::Bint::A<int>::x" \
-	"No type \"Bint\" within class or namespace \"D\"."
-    gdb_test "print B<int>::x" "There is no field named x"
-    gdb_test "print B<int>::A<int>::x" "There is no field named x"
-    gdb_test "print Bint::x" "There is no field named x"
-    gdb_test "print Bint::A<int>::x" "There is no field named x"
+    gdb_test "print D::C::A<int>::c" "There is no field named c"
     gdb_test "print D::C::x" "There is no field named x"
     gdb_test "print C::x" "There is no field named x"
     gdb_test "print D::C::A<int>::x" "There is no field named x"
@@ -117,8 +146,14 @@  with_test_prefix "at D::f (invalid expressions)" {
 
 # Test some ambiguous names
 with_test_prefix "at D::f (ambiguous names)" {
-    gdb_test "print B<int>::common" " = 200"
-    gdb_test "print Bint::common" " = 200"
+    if {[test_compiler_info {gcc-*-*}]} {
+        unsupported "gdb/16841"
+    } else {
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print B<int>::common" " = 200"
+        setup_kfail gdb/16841 *-*-*
+        gdb_test "print Bint::common" " = 200"
+    }
     gdb_test "print C::common" " = 300"
     gdb_test "print am.i" " = 1000"
     gdb_test "print am.A<int>::i" \