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

  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
  

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,
  

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
  

> > 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.
  

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
  

> 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".
  

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.
  

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.
  

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]
  

> 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.