[patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[David Blaikie]

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

Clang has an optimization that causes a the debug info to only include
the declaration of a type if the type is referenced but never used in
a context that requires a definition (eg: pointers are handed around
but never deferenced).

This patch introduces a variable to one test file to cause clang to
emit the full definition of the type as well as fixing up a related
typo in the test message of the associated expect file.

Like the difference between GCC and Clang in the emission of unused
static entities, I think this case is also a matter of degrees - both
GCC and Clang implement other similar optimizations* to the one
outlined here and the GDB test suite has managed to pass without
disabling those optimizations in GCC and I hope it's suitable to do
the same for Clang.

Though admittedly I don't have much of the context of the history of
the testsuite, its priorities/preferences when it comes to
distinguishing testing compiler behavior versus debugger behavior,
etc.

* the one I know of involves dynamic types: both GCC and Clang only
emit the debug info definition of such a type in any translation unit
that emits the key function. This means in many contexts where a full
definition is provided in the source only a declaration is provided in
the debug info.

[-- Attachment #2: clang_full_type.diff --]
[-- Type: text/plain, Size: 2354 bytes --]

commit 1128f6fb45483d45668d09e0696f4a590334e0c4
Author: David Blaikie <dblaikie@gmail.com>
Date:   Sat Apr 12 23:27:19 2014 -0700

    Cause clang to emit the definition of a type used only by pointer
    
    gdb/testsuite/
    	* gdb.stabs/gdb11479.c: introduce a variable to cause clang to
    	emit the full definition of type required by the test
    	* gdb.stabs/gdb11479.exp: correct a typo in a test message

diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
index 730c116..07ba18e 100644
--- gdb/testsuite/ChangeLog
+++ gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2014-04-12  David Blaikie  <dblaikie@gmail.com>
+
+        * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
+	emit the full definition of type required by the test
+        * gdb.stabs/gdb11479.exp: correct a typo in a test message
+
 2014-04-12  Siva Chandra Reddy  <sivachandra@google.com>
 	    Doug Evans  <xdje42@gmail.com>
 
diff --git gdb/testsuite/gdb.stabs/gdb11479.c gdb/testsuite/gdb.stabs/gdb11479.c
index eb7fcf9..f70930f 100644
--- gdb/testsuite/gdb.stabs/gdb11479.c
+++ gdb/testsuite/gdb.stabs/gdb11479.c
@@ -55,7 +55,7 @@ struct dummy {
 enum dummy_enum {
   enum1,
   enum2
-};
+} tag_dummy_enum;
 
 void *
 hack (const struct dummy *t, const enum dummy_enum *e)
diff --git gdb/testsuite/gdb.stabs/gdb11479.exp gdb/testsuite/gdb.stabs/gdb11479.exp
index b9ed238..a2782ac 100644
--- gdb/testsuite/gdb.stabs/gdb11479.exp
+++ gdb/testsuite/gdb.stabs/gdb11479.exp
@@ -31,13 +31,13 @@ proc do_test {version} {
     gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" \
 	"Inspect t in test2 $version"
     # Check that the enum type length has been set to a non-zero value
-    gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (e) in test2 $version"
+    gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (*e) in test2 $version"
     gdb_test "continue" "Breakpoint .* test .*" \
 	"Stop at first breakpoint $version"
     gdb_test "print *t" ".*\{x = 5, y = 25, b = 2.5\}.*" \
 	"Inspect t in test $version"
     # Check that the enum type length has been set to a non-zero value
-    gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (e) in test $version"
+    gdb_test "print sizeof (*e)" "= \[1-9\]*" "sizeof (*e) in test $version"
 }
 
 if { [prepare_for_testing $testfile.exp $testfile $testfile.c {debug additional_flags=-gstabs}] == 0 } {

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[David Blaikie]

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

Oh, and there was one other instance of the same issue (I believe
these are the only two cases)

This test case (gdb.cp/pr10728) seems like it could probably be
simplified a great deal, down to:

struct foo;
foo *f, *g;
int main() {
} // break and print f - g

if I'm not mistaken... - but I've made a relatively small change for
now. If preferred, I can make the more substantial simplification
suggested. Or otherwise appease clang (by introducing a use of the
original 'x' type definition in some other way - dereferencing a
pointer, etc).

- David

On Sat, Apr 12, 2014 at 11:43 PM, David Blaikie <dblaikie@gmail.com> wrote:
> Clang has an optimization that causes a the debug info to only include
> the declaration of a type if the type is referenced but never used in
> a context that requires a definition (eg: pointers are handed around
> but never deferenced).
>
> This patch introduces a variable to one test file to cause clang to
> emit the full definition of the type as well as fixing up a related
> typo in the test message of the associated expect file.
>
> Like the difference between GCC and Clang in the emission of unused
> static entities, I think this case is also a matter of degrees - both
> GCC and Clang implement other similar optimizations* to the one
> outlined here and the GDB test suite has managed to pass without
> disabling those optimizations in GCC and I hope it's suitable to do
> the same for Clang.
>
> Though admittedly I don't have much of the context of the history of
> the testsuite, its priorities/preferences when it comes to
> distinguishing testing compiler behavior versus debugger behavior,
> etc.
>
> * the one I know of involves dynamic types: both GCC and Clang only
> emit the debug info definition of such a type in any translation unit
> that emits the key function. This means in many contexts where a full
> definition is provided in the source only a declaration is provided in
> the debug info.

[-- Attachment #2: pr10728-full-type.diff --]
[-- Type: text/plain, Size: 1812 bytes --]

commit 24a8810a97155710d3c53b401eb8729bc4f80c00
Author: David Blaikie <dblaikie@gmail.com>
Date:   Sun Apr 13 00:48:45 2014 -0700

    Return by value to coax Clang into emitting the full definition of a test type.
    
    gdb/testsuite/
    	* gdb.cp/pr10728-x.cc: Return by value instead of pointer to coax
    	Clang into emitting the definition of the type.
    	* gdb.cp/pr10728-x.h: Ditto.
    	* gdb.cp/pr10728-y.cc: Ditto.

diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
index 730c116..1d15721 100644
--- gdb/testsuite/ChangeLog
+++ gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2014-04-12  David Blaikie  <dblaikie@gmail.com>
+
+	* gdb.cp/pr10728-x.cc: Return by value instead of pointer to coax
+	Clang into emitting the definition of the type.
+	* gdb.cp/pr10728-x.h: Ditto.
+	* gdb.cp/pr10728-y.cc: Ditto.
+
 2014-04-12  Siva Chandra Reddy  <sivachandra@google.com>
 	    Doug Evans  <xdje42@gmail.com>
 
diff --git gdb/testsuite/gdb.cp/pr10728-x.cc gdb/testsuite/gdb.cp/pr10728-x.cc
index 7623c0b..d6b7666 100644
--- gdb/testsuite/gdb.cp/pr10728-x.cc
+++ gdb/testsuite/gdb.cp/pr10728-x.cc
@@ -2,6 +2,6 @@
 
 int main()
 {
-  X* x = y();
+  X x = y();
   return 0;		// marker 1
 }
diff --git gdb/testsuite/gdb.cp/pr10728-x.h gdb/testsuite/gdb.cp/pr10728-x.h
index 63737d9..0ba58bb 100644
--- gdb/testsuite/gdb.cp/pr10728-x.h
+++ gdb/testsuite/gdb.cp/pr10728-x.h
@@ -5,5 +5,5 @@ struct X
   Y* y2;
 };
 
-X* y();
+X y();
 
diff --git gdb/testsuite/gdb.cp/pr10728-y.cc gdb/testsuite/gdb.cp/pr10728-y.cc
index 84b222d..d8a932a 100644
--- gdb/testsuite/gdb.cp/pr10728-y.cc
+++ gdb/testsuite/gdb.cp/pr10728-y.cc
@@ -1,11 +1,11 @@
 #include "pr10728-x.h"
 struct Y{};
 
-X* y()
+X y()
 {
-  static X xx;
+  X xx;
   static Y yy;
   xx.y1 = &yy;
   xx.y2 = xx.y1+1;
-  return &xx;
+  return xx;
 }

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Joel Brobecker]

On Sat, Apr 12, 2014 at 11:43:40PM -0700, David Blaikie wrote:
> Clang has an optimization that causes a the debug info to only include
> the declaration of a type if the type is referenced but never used in
> a context that requires a definition (eg: pointers are handed around
> but never deferenced).
> 
> This patch introduces a variable to one test file to cause clang to
> emit the full definition of the type as well as fixing up a related
> typo in the test message of the associated expect file.
> 
> Like the difference between GCC and Clang in the emission of unused
> static entities, I think this case is also a matter of degrees - both
> GCC and Clang implement other similar optimizations* to the one
> outlined here and the GDB test suite has managed to pass without
> disabling those optimizations in GCC and I hope it's suitable to do
> the same for Clang.
> 
> Though admittedly I don't have much of the context of the history of
> the testsuite, its priorities/preferences when it comes to
> distinguishing testing compiler behavior versus debugger behavior,
> etc.
> 
> * the one I know of involves dynamic types: both GCC and Clang only
> emit the debug info definition of such a type in any translation unit
> that emits the key function. This means in many contexts where a full
> definition is provided in the source only a declaration is provided in
> the debug info.

> commit 1128f6fb45483d45668d09e0696f4a590334e0c4
> Author: David Blaikie <dblaikie@gmail.com>
> Date:   Sat Apr 12 23:27:19 2014 -0700
> 
>     Cause clang to emit the definition of a type used only by pointer
>     
>     gdb/testsuite/
>     	* gdb.stabs/gdb11479.c: introduce a variable to cause clang to
>     	emit the full definition of type required by the test
>     	* gdb.stabs/gdb11479.exp: correct a typo in a test message

The gdb11479.exp is obvious and can be pushed. The change to gdb11479.c,
on the other hand, changes the test, and I don't think we want that,
because I disagree with the outcome of Clang's optimization here.
If Clang doesn't want to fix the problem, best to just xfail the test
with Clang, and write a new one that provides the type definition as
Clang wants it.

Thanks,
-- 
Joel

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Eric Christopher]

On Mon, Apr 14, 2014 at 6:10 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> On Sat, Apr 12, 2014 at 11:43:40PM -0700, David Blaikie wrote:
>> Clang has an optimization that causes a the debug info to only include
>> the declaration of a type if the type is referenced but never used in
>> a context that requires a definition (eg: pointers are handed around
>> but never deferenced).
>>
>> This patch introduces a variable to one test file to cause clang to
>> emit the full definition of the type as well as fixing up a related
>> typo in the test message of the associated expect file.
>>
>> Like the difference between GCC and Clang in the emission of unused
>> static entities, I think this case is also a matter of degrees - both
>> GCC and Clang implement other similar optimizations* to the one
>> outlined here and the GDB test suite has managed to pass without
>> disabling those optimizations in GCC and I hope it's suitable to do
>> the same for Clang.
>>
>> Though admittedly I don't have much of the context of the history of
>> the testsuite, its priorities/preferences when it comes to
>> distinguishing testing compiler behavior versus debugger behavior,
>> etc.
>>
>> * the one I know of involves dynamic types: both GCC and Clang only
>> emit the debug info definition of such a type in any translation unit
>> that emits the key function. This means in many contexts where a full
>> definition is provided in the source only a declaration is provided in
>> the debug info.
>
>> commit 1128f6fb45483d45668d09e0696f4a590334e0c4
>> Author: David Blaikie <dblaikie@gmail.com>
>> Date:   Sat Apr 12 23:27:19 2014 -0700
>>
>>     Cause clang to emit the definition of a type used only by pointer
>>
>>     gdb/testsuite/
>>       * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
>>       emit the full definition of type required by the test
>>       * gdb.stabs/gdb11479.exp: correct a typo in a test message
>
> The gdb11479.exp is obvious and can be pushed. The change to gdb11479.c,
> on the other hand, changes the test, and I don't think we want that,
> because I disagree with the outcome of Clang's optimization here.
> If Clang doesn't want to fix the problem, best to just xfail the test
> with Clang, and write a new one that provides the type definition as
> Clang wants it.
>

Could you describe your objection here? In particular, as it relates
to Dave's analysis of a similar gcc optimization. (Also a few people
have been interested in implementing this same behavior in gcc).

-eric

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Joel Brobecker]

> > The gdb11479.exp is obvious and can be pushed. The change to gdb11479.c,
> > on the other hand, changes the test, and I don't think we want that,
> > because I disagree with the outcome of Clang's optimization here.
> > If Clang doesn't want to fix the problem, best to just xfail the test
> > with Clang, and write a new one that provides the type definition as
> > Clang wants it.
> 
> Could you describe your objection here? In particular, as it relates
> to Dave's analysis of a similar gcc optimization. (Also a few people
> have been interested in implementing this same behavior in gcc).

The fact is, at the moment, that GCC generates the necessary debugging
information, without the need to create a typedef. If it stops working
thanks to a GCC "optimization", I would like to know about it (personally).
In this particular case, the type is used locally, albeit as a pointer,
and it would seem unfriendly to me that the user not be able to either
print the pointed object's size, or any of the enum's element, just
because it is only used via a pointer. I also seems strange to me
that an unused typedef is enough to trigger full debug info generation,
while a variable indirectly referencing a type is not.

That's why I suggested that we keep the current testcase as is,
with an xfail with clang, and create a new one if we want to, that
tests the behavior with the proposed work around.

That's only my opinion, however; it would be fine for the change
to go in if other maintainers disagree with me and approve the change.

-- 
Joel

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[David Blaikie]

On Mon, Apr 14, 2014 at 11:16 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> > The gdb11479.exp is obvious and can be pushed. The change to gdb11479.c,
>> > on the other hand, changes the test, and I don't think we want that,
>> > because I disagree with the outcome of Clang's optimization here.
>> > If Clang doesn't want to fix the problem, best to just xfail the test
>> > with Clang, and write a new one that provides the type definition as
>> > Clang wants it.
>>
>> Could you describe your objection here? In particular, as it relates
>> to Dave's analysis of a similar gcc optimization. (Also a few people
>> have been interested in implementing this same behavior in gcc).
>
> The fact is, at the moment, that GCC generates the necessary debugging
> information, without the need to create a typedef.

Minor (but important) correction: this isn't introducing a typedef,
it's introducing a variable.

> If it stops working
> thanks to a GCC "optimization", I would like to know about it (personally).

This comes into a discussion about whether the GDB test suite is a
test suite for GCC (or Clang) or for GDB. I'm by no means in a
position to make a claim as to which of these things it is, though
it's clear that GCC and Clang use this suite to validate their
behavior relative to GDB.

But it'd probably be nice to separate out those things a little - this
test case isn't intended to test that GCC behavior. I believe/assume
this test case could still test the GDB behavior that it was committed
to validate without tickling the difference between GCC and Clang.

I've tried to be consistent to this principle with the patches I'm
providing - if there's a difference in Clang and GCC behavior that's
not necessary for a test case to test GDB and the test can be adjusted
to be neutral to that difference, I've tried to change the tests in
that direction.

In cases where a GCC or Clang output makes it impossible to test a GDB
feature (eg: "compiler X produces this quirky output that GDB should
handle nicely") I've either XFAILed the test if the difference is a
bug in the other compiler or adjusted the test to mark as UNSUPPORTED
when run under the other compiler ("hey, we can't produce that quirky
output on this compiler so in this configuration we can't test that
GDB can cope with it")

It's a bit fuzzy - I've XFAILed some thing that are bugs in one
compiler or the other even when the test case might be able to be made
agnostic to the difference because it took less time than refactoring
the test, and hey "it's a bug anyway".

> In this particular case, the type is used locally, albeit as a pointer,
> and it would seem unfriendly to me that the user not be able to either
> print the pointed object's size, or any of the enum's element, just
> because it is only used via a pointer.

GCC has similar optimizations that might surprise you:

struct foo {
  virtual ~foo();
};

foo f; // GCC and Clang only emits a declaration of 'foo' here

If you make that dtor non-virtual, you'll get a definition. The
optimizations here are founded on the assumption that the whole
program will be built with debug info (in the virtual example above,
we know the type has a key function that must be defined /somewhere/
so just emit the full definition of the type when we emit the
definition of that key function - in the case of Clang's optimization
the assumption is that /some/ translation unit in the program will
dereference the pointer, and at that point we'll emit the definition
of the type). These assumptions are worth a substantial amount (easily
10s of percent of unlinked debug info size - and only less in the
linked case if you use type units) and have clear value to many users.

>  I also seems strange to me
> that an unused typedef is enough to trigger full debug info generation,
> while a variable indirectly referencing a type is not.

(as mentioned, it's actually a global variable of the type, rather
than a typedef)

The way Clang works is that any action that would require the full
definition of the type that was already being emitted, causes the full
definition to be emitted, if that makes sense (I can provide
examples/more words - it is a bit hard to articulate clearly).

> That's why I suggested that we keep the current testcase as is,
> with an xfail with clang, and create a new one if we want to, that
> tests the behavior with the proposed work around.
>
> That's only my opinion, however; it would be fine for the change
> to go in if other maintainers disagree with me and approve the change.

Fair enough - I think the conversation's useful in any case, to help
understand how these different priorities coexist within the project.

- David

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Joel Brobecker]

> Minor (but important) correction: this isn't introducing a typedef,
> it's introducing a variable.

OK - sorry for the confusion - I went too fast and it changes
the perspective a little.

> This comes into a discussion about whether the GDB test suite is a
> test suite for GCC (or Clang) or for GDB. I'm by no means in a
> position to make a claim as to which of these things it is, though
> it's clear that GCC and Clang use this suite to validate their
> behavior relative to GDB.

I don't remember if this was ever discussed, but it should be
compiler-neutral, IMO, even if we do have a practical bias towards
GCC. The intent is to test that, with the compiler you have, GDB
is able to debug your code as expected.

> But it'd probably be nice to separate out those things a little

We try to do it, sometimes. That's why we have gdb.dwarf2.
Ideally, for every testcase, we'd create one using assembly
sources to control exactly the code/debug-info produce, and
one where we compile some source which, at some point, resulted
in the assembly we used in our first testcase. That way, we test
both the handling of that particular output, as well as the
user-level handling of whatever gets generated by your compiler.
But it is a lot of work, and sadly, usually only one of the two
options is elected when new testcases are added. Just a practical
matter.

> I've tried to be consistent to this principle with the patches I'm
> providing - if there's a difference in Clang and GCC behavior that's
> not necessary for a test case to test GDB and the test can be adjusted
> to be neutral to that difference, I've tried to change the tests in
> that direction.

My position, in this situation, is that your change is actually
not completely neutral: Even if this was not the initial intention
when the test was written, as it is now, it allows us to verify
that the compiler produces the full debugging information for
an enum type even in the case where the type is only referenced
through a pointer. By adding a global variable, we lose that part,
potentially allowing GCC to regress (from a GDB user's perspective).

If the type was opaque, I would have had no objection. But in this
case, I try to put myself in the shoes of a user debugging that code,
and it would seem reasonable to be able to dereference "e".

-- 
Joel

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Doug Evans]

David Blaikie writes:
 > commit 24a8810a97155710d3c53b401eb8729bc4f80c00
 > Author: David Blaikie <dblaikie@gmail.com>
 > Date:   Sun Apr 13 00:48:45 2014 -0700
 > 
 >     Return by value to coax Clang into emitting the full definition of a test type.
 >     
 >     gdb/testsuite/
 >     	* gdb.cp/pr10728-x.cc: Return by value instead of pointer to coax
 >     	Clang into emitting the definition of the type.
 >     	* gdb.cp/pr10728-x.h: Ditto.
 >     	* gdb.cp/pr10728-y.cc: Ditto.

LGTM with one nit.

ChangeLog conventions require one to document the function
in which the change went.
There's plenty of existing boilerplate to copy from.

Thanks!

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Pedro Alves]

On 04/14/2014 11:47 PM, Joel Brobecker wrote:
> My position, in this situation, is that your change is actually
> not completely neutral: Even if this was not the initial intention
> when the test was written, as it is now, it allows us to verify
> that the compiler produces the full debugging information for
> an enum type even in the case where the type is only referenced
> through a pointer. By adding a global variable, we lose that part,
> potentially allowing GCC to regress (from a GDB user's perspective).
> 
> If the type was opaque, I would have had no objection. But in this
> case, I try to put myself in the shoes of a user debugging that code,
> and it would seem reasonable to be able to dereference "e".

I think it'd be good if we had a test that exercises that expectation
explicitly.

-- 
Pedro Alves

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Doug Evans]

David Blaikie writes:
 > Clang has an optimization that causes a the debug info to only include
 > the declaration of a type if the type is referenced but never used in
 > a context that requires a definition (eg: pointers are handed around
 > but never deferenced).
 > 
 > This patch introduces a variable to one test file to cause clang to
 > emit the full definition of the type as well as fixing up a related
 > typo in the test message of the associated expect file.
 > 
 > Like the difference between GCC and Clang in the emission of unused
 > static entities, I think this case is also a matter of degrees - both
 > GCC and Clang implement other similar optimizations* to the one
 > outlined here and the GDB test suite has managed to pass without
 > disabling those optimizations in GCC and I hope it's suitable to do
 > the same for Clang.
 > 
 > Though admittedly I don't have much of the context of the history of
 > the testsuite, its priorities/preferences when it comes to
 > distinguishing testing compiler behavior versus debugger behavior,
 > etc.
 > 
 > * the one I know of involves dynamic types: both GCC and Clang only
 > emit the debug info definition of such a type in any translation unit
 > that emits the key function. This means in many contexts where a full
 > definition is provided in the source only a declaration is provided in
 > the debug info.
 > commit 1128f6fb45483d45668d09e0696f4a590334e0c4
 > Author: David Blaikie <dblaikie@gmail.com>
 > Date:   Sat Apr 12 23:27:19 2014 -0700
 > 
 >     Cause clang to emit the definition of a type used only by pointer
 >     
 >     gdb/testsuite/
 >     	* gdb.stabs/gdb11479.c: introduce a variable to cause clang to
 >     	emit the full definition of type required by the test
 >     	* gdb.stabs/gdb11479.exp: correct a typo in a test message

ChangeLog conventions require one to document more specifically
where the change occurred.  E.g.,

	* gdb.stabs/gdb11479.c (tag_dummy_enum): New global to cause clang to
	emit the full definition of type required by the test.
	* gdb.stabs/gdb11479.exp (do_test): Correct a typo in a test message.

Plus note the capitalization and period.

Plus conventions also say to specify the "why" of things in source
and not the changelog.  I realize we're not going to the trouble
of adding comments to every non-static function to document why it
has to be non-static.  So I don't see a great need to do so here,
and I'd leave the ChangeLog entry as written.
I'm just writing this down in case the topic comes up.

 > 
 > diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
 > index 730c116..07ba18e 100644
 > --- gdb/testsuite/ChangeLog
 > +++ gdb/testsuite/ChangeLog
 > @@ -1,3 +1,9 @@
 > +2014-04-12  David Blaikie  <dblaikie@gmail.com>
 > +
 > +        * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
 > +	emit the full definition of type required by the test
 > +        * gdb.stabs/gdb11479.exp: correct a typo in a test message
 > +

Mix of tabs and spaces.  Just use tabs.

Ok with those changes.

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Doug Evans]

On Wed, Apr 23, 2014 at 5:20 PM, Doug Evans <dje@google.com> wrote:
> David Blaikie writes:
>  > Clang has an optimization that causes a the debug info to only include
>  > the declaration of a type if the type is referenced but never used in
>  > a context that requires a definition (eg: pointers are handed around
>  > but never deferenced).
>  >
>  > This patch introduces a variable to one test file to cause clang to
>  > emit the full definition of the type as well as fixing up a related
>  > typo in the test message of the associated expect file.
>  >
>  > Like the difference between GCC and Clang in the emission of unused
>  > static entities, I think this case is also a matter of degrees - both
>  > GCC and Clang implement other similar optimizations* to the one
>  > outlined here and the GDB test suite has managed to pass without
>  > disabling those optimizations in GCC and I hope it's suitable to do
>  > the same for Clang.
>  >
>  > Though admittedly I don't have much of the context of the history of
>  > the testsuite, its priorities/preferences when it comes to
>  > distinguishing testing compiler behavior versus debugger behavior,
>  > etc.
>  >
>  > * the one I know of involves dynamic types: both GCC and Clang only
>  > emit the debug info definition of such a type in any translation unit
>  > that emits the key function. This means in many contexts where a full
>  > definition is provided in the source only a declaration is provided in
>  > the debug info.
>  > commit 1128f6fb45483d45668d09e0696f4a590334e0c4
>  > Author: David Blaikie <dblaikie@gmail.com>
>  > Date:   Sat Apr 12 23:27:19 2014 -0700
>  >
>  >     Cause clang to emit the definition of a type used only by pointer
>  >
>  >     gdb/testsuite/
>  >      * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
>  >      emit the full definition of type required by the test
>  >      * gdb.stabs/gdb11479.exp: correct a typo in a test message
>
> ChangeLog conventions require one to document more specifically
> where the change occurred.  E.g.,
>
>         * gdb.stabs/gdb11479.c (tag_dummy_enum): New global to cause clang to
>         emit the full definition of type required by the test.
>         * gdb.stabs/gdb11479.exp (do_test): Correct a typo in a test message.
>
> Plus note the capitalization and period.
>
> Plus conventions also say to specify the "why" of things in source
> and not the changelog.  I realize we're not going to the trouble
> of adding comments to every non-static function to document why it
> has to be non-static.  So I don't see a great need to do so here,
> and I'd leave the ChangeLog entry as written.
> I'm just writing this down in case the topic comes up.
>
>  >
>  > diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
>  > index 730c116..07ba18e 100644
>  > --- gdb/testsuite/ChangeLog
>  > +++ gdb/testsuite/ChangeLog
>  > @@ -1,3 +1,9 @@
>  > +2014-04-12  David Blaikie  <dblaikie@gmail.com>
>  > +
>  > +        * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
>  > +    emit the full definition of type required by the test
>  > +        * gdb.stabs/gdb11479.exp: correct a typo in a test message
>  > +
>
> Mix of tabs and spaces.  Just use tabs.
>
> Ok with those changes.

Bleah.  Sorry Joel.  I didn't see your earlier mail.

What do you think of adding a testcase that explicitly tests the
user's expectation?
[per Pedro's suggestion]

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[Joel Brobecker]

> Bleah.  Sorry Joel.  I didn't see your earlier mail.

No problem. As I explained, I don't have a strong opinion
on this, and it was fine if you guys disagreed with me.

> What do you think of adding a testcase that explicitly tests the
> user's expectation?
> [per Pedro's suggestion]

That would work.

-- 
Joel

Re: [patch] [gdb/testsuite] include a use of the definition of a type to cause clang to emit debug info

[David Blaikie]

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

On Wed, Apr 23, 2014 at 5:29 PM, Doug Evans <dje@google.com> wrote:
> On Wed, Apr 23, 2014 at 5:20 PM, Doug Evans <dje@google.com> wrote:
>> David Blaikie writes:
>>  > Clang has an optimization that causes a the debug info to only include
>>  > the declaration of a type if the type is referenced but never used in
>>  > a context that requires a definition (eg: pointers are handed around
>>  > but never deferenced).
>>  >
>>  > This patch introduces a variable to one test file to cause clang to
>>  > emit the full definition of the type as well as fixing up a related
>>  > typo in the test message of the associated expect file.
>>  >
>>  > Like the difference between GCC and Clang in the emission of unused
>>  > static entities, I think this case is also a matter of degrees - both
>>  > GCC and Clang implement other similar optimizations* to the one
>>  > outlined here and the GDB test suite has managed to pass without
>>  > disabling those optimizations in GCC and I hope it's suitable to do
>>  > the same for Clang.
>>  >
>>  > Though admittedly I don't have much of the context of the history of
>>  > the testsuite, its priorities/preferences when it comes to
>>  > distinguishing testing compiler behavior versus debugger behavior,
>>  > etc.
>>  >
>>  > * the one I know of involves dynamic types: both GCC and Clang only
>>  > emit the debug info definition of such a type in any translation unit
>>  > that emits the key function. This means in many contexts where a full
>>  > definition is provided in the source only a declaration is provided in
>>  > the debug info.
>>  > commit 1128f6fb45483d45668d09e0696f4a590334e0c4
>>  > Author: David Blaikie <dblaikie@gmail.com>
>>  > Date:   Sat Apr 12 23:27:19 2014 -0700
>>  >
>>  >     Cause clang to emit the definition of a type used only by pointer
>>  >
>>  >     gdb/testsuite/
>>  >      * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
>>  >      emit the full definition of type required by the test
>>  >      * gdb.stabs/gdb11479.exp: correct a typo in a test message
>>
>> ChangeLog conventions require one to document more specifically
>> where the change occurred.  E.g.,
>>
>>         * gdb.stabs/gdb11479.c (tag_dummy_enum): New global to cause clang to
>>         emit the full definition of type required by the test.
>>         * gdb.stabs/gdb11479.exp (do_test): Correct a typo in a test message.
>>
>> Plus note the capitalization and period.

Hmm, hopefully I got most of those edits right. Sorry if I missed some.

>> Plus conventions also say to specify the "why" of things in source
>> and not the changelog.

Makes sense.

>> I realize we're not going to the trouble
>> of adding comments to every non-static function to document why it
>> has to be non-static.  So I don't see a great need to do so here,
>> and I'd leave the ChangeLog entry as written.
>> I'm just writing this down in case the topic comes up.

Yep - for anything particularly non-obvious, I'll try to keep that in mind.

>>
>>  >
>>  > diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
>>  > index 730c116..07ba18e 100644
>>  > --- gdb/testsuite/ChangeLog
>>  > +++ gdb/testsuite/ChangeLog
>>  > @@ -1,3 +1,9 @@
>>  > +2014-04-12  David Blaikie  <dblaikie@gmail.com>
>>  > +
>>  > +        * gdb.stabs/gdb11479.c: introduce a variable to cause clang to
>>  > +    emit the full definition of type required by the test
>>  > +        * gdb.stabs/gdb11479.exp: correct a typo in a test message
>>  > +
>>
>> Mix of tabs and spaces.  Just use tabs.
>>
>> Ok with those changes.

Committed in 22842ff63e28b86e0cd40a87186757b2525578f4 and
22842ff63e28b86e0cd40a87186757b2525578f4

>
> Bleah.  Sorry Joel.  I didn't see your earlier mail.
>
> What do you think of adding a testcase that explicitly tests the
> user's expectation?

I've attached an example of such a test.

My thoughts are that this isn't so much a matter of user expectations
(I've cited similar behavior in GCC that fires when a type is dynamic
that would be equally surprising to a user, I imagine - yet there's no
GDB test ensuring that the compiler doesn't do that), nor of bugs
(I've provided several patches that workaround GCC bugs and make the
test suite pass when the bug can be worked around to allow the tests
to continue testing the GDB behavior they were intended to). Also this
seems to be a compiler test - a debugger's codepaths can be fully
exercised by providing an example with a declaration and an example
with a definition, regardless of how the source was written. A
compiler test would then ensure that the definition is provided in the
cases where the compiler developers desire it to be emitted, it's
redundant to test the debugger with N different ways the compiler
produces the same debug info.

But I'm still getting a feel for what this testsuite is used for, who
the owners/stakeholders are, what their priorities/goals are, etc.
Just some thoughts.

> [per Pedro's suggestion]

[-- Attachment #2: unlimited.diff --]
[-- Type: text/plain, Size: 3058 bytes --]

commit 36d650aa2b2fa688c049ba73b4a8fc26a0b6742b
Author: David Blaikie <dblaikie@gmail.com>
Date:   Thu Apr 24 22:02:40 2014 -0700

    Provide a test for GCC's default and part of Clang's -fstandalone-debug behavior.
    
    gdb/testsuite/
    	* gdb.base/pointer-to-def.exp: Verify that the compiler produces a
    	definition for a type only used via pointer but with a definition
    	available. Use Clang's -fstandalone-debug when running with clang to
    	force this emission.
    	* gdb.base/pointer-to-def.cc: Provides a defined type and a variable of
    	pointer to that type.

diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
index c028cd5..78c3164 100644
--- gdb/testsuite/ChangeLog
+++ gdb/testsuite/ChangeLog
@@ -1,5 +1,14 @@
 2014-04-24  David Blaikie  <dblaikie@gmail.com>
 
+	* gdb.base/pointer-to-def.exp: Verify that the compiler produces a
+	definition for a type only used via pointer but with a definition
+	available. Use Clang's -fstandalone-debug when running with clang to
+	force this emission.
+	* gdb.base/pointer-to-def.cc: Provides a defined type and a variable of
+	pointer to that type.
+
+2014-04-24  David Blaikie  <dblaikie@gmail.com>
+
 	* gdb.stabs/gdb11479.c (tag_dummy_enum): introduce a variable to cause
 	clang to emit the full definition of type required by the test
 	* gdb.stabs/gdb11479.exp (do_test): correct a typo in a test message
diff --git gdb/testsuite/gdb.cp/pointer-to-def.cc gdb/testsuite/gdb.cp/pointer-to-def.cc
new file mode 100644
index 0000000..f6499b9
--- /dev/null
+++ gdb/testsuite/gdb.cp/pointer-to-def.cc
@@ -0,0 +1,8 @@
+struct foo {
+  int i;
+};
+
+foo *f;
+
+int main() {
+}
diff --git gdb/testsuite/gdb.cp/pointer-to-def.exp gdb/testsuite/gdb.cp/pointer-to-def.exp
new file mode 100644
index 0000000..2c51827
--- /dev/null
+++ gdb/testsuite/gdb.cp/pointer-to-def.exp
@@ -0,0 +1,35 @@
+# Copyright 2010-2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if { [skip_cplus_tests] } { continue }
+
+if { [get_compiler_info] } {
+    return -1
+}
+
+set additional_flags ""
+
+if { [test_compiler_info {clang-*-*}] } {
+  set additional_flags "-fstandalone-debug"
+}
+
+standard_testfile .cc
+
+if { [prepare_for_testing ${testfile}.exp $testfile ${srcfile} "debug additional_flags=$additional_flags"] } {
+    untested pointer-to-def.exp
+    return -1
+}
+
+gdb_test "ptype f" "type = struct foo {\r\n    int i;\r\n} \\*" "definition of foo"