Fix unused static symbols so they're not dropped by clang

  Several tests used file-static functions and variables that were not
referenced by the code. Even at -O0, clang omits these entities at the
frontend so the tests fail.

Since it doesn't look like these tests needed this functionality for
what they were testing, I've modified the variables/functions to
either be non-static, or marked them with __attribute__((used)).

If it's preferred that I use the attribute more pervasively, rather
than just making the entities non-static, I can provide a patch for
that (or some other preferred solution). There's certainly precedent
for both (non-static entities and __attribute__((used)) in the
testsuite already and much more of the former than the latter).

I have commit-after-review access, so just looking for sign-off here.

Thanks,
- David
commit d72b042cbd4f0af6d2e4ac6901445677b93f77ee
Author: David Blaikie <dblaikie@gmail.com>
Date:   Thu Apr 10 23:45:28 2014 -0700

    Ensure unreferenced static symbols aren't omitted by clang (either marking them __attribute__((used)) or making them non-static)
    
    gdb/testsuite/
           * gdb.base/catch-syscall.c: Make unreferenced statics non-static to
           ensure clang would not discard them.
           * gdb.base/gdbvars.c: Ditto.
           * gdb.base/memattr.c: Ditto.
           * gdb.base/whatis.c: Ditto.
           * gdb.python/py-prettyprint.c: Ditto.
           * gdb.trace/actions.c: Ditto.
           * gdb.cp/ptype-cv-cp.cc: Mark unused global const int as used to
           ensure clang would not discard it.
  

> On Apr 10, 2014, at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
> 
> Several tests used file-static functions and variables that were not
> referenced by the code. Even at -O0, clang omits these entities at the
> frontend so the tests fail.

I think clang should change here rather than the testsuite of gdb. Unused static functions make sense to be kept around at -O0 for debugging reasons. 

Thanks,
Andrew


> 
> Since it doesn't look like these tests needed this functionality for
> what they were testing, I've modified the variables/functions to
> either be non-static, or marked them with __attribute__((used)).
> 
> If it's preferred that I use the attribute more pervasively, rather
> than just making the entities non-static, I can provide a patch for
> that (or some other preferred solution). There's certainly precedent
> for both (non-static entities and __attribute__((used)) in the
> testsuite already and much more of the former than the latter).
> 
> I have commit-after-review access, so just looking for sign-off here.
> 
> Thanks,
> - David
> <unused.diff>
  

On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
> Several tests used file-static functions and variables that were not
> referenced by the code. Even at -O0, clang omits these entities at the
> frontend so the tests fail.
>
> Since it doesn't look like these tests needed this functionality for
> what they were testing, I've modified the variables/functions to
> either be non-static, or marked them with __attribute__((used)).
>
> If it's preferred that I use the attribute more pervasively, rather
> than just making the entities non-static, I can provide a patch for
> that (or some other preferred solution). There's certainly precedent
> for both (non-static entities and __attribute__((used)) in the
> testsuite already and much more of the former than the latter).
>
> I have commit-after-review access, so just looking for sign-off here.

Yikes.

This is becoming more and more painful (not your fault of course!).
I can imagine this being a never ending source of regressions.

Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
  

On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>> Several tests used file-static functions and variables that were not
>> referenced by the code. Even at -O0, clang omits these entities at the
>> frontend so the tests fail.
>>
>> Since it doesn't look like these tests needed this functionality for
>> what they were testing, I've modified the variables/functions to
>> either be non-static, or marked them with __attribute__((used)).
>>
>> If it's preferred that I use the attribute more pervasively, rather
>> than just making the entities non-static, I can provide a patch for
>> that (or some other preferred solution). There's certainly precedent
>> for both (non-static entities and __attribute__((used)) in the
>> testsuite already and much more of the former than the latter).
>>
>> I have commit-after-review access, so just looking for sign-off here.
>
> Yikes.
>
> This is becoming more and more painful (not your fault of course!).
> I can imagine this being a never ending source of regressions.
>
> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?

Failing that,

making the entries non-static without adding a comment to explain why
things are the way they are will leave things in a more fragile state,
and if we're going to add a comment we might just as well use an
attribute throughout I guess.
However using the attribute is, technically, more complicated than
that because we shouldn't unnecessarily break testing with other
compilers.
That suggests putting the attribute in a macro in a header protected
by appropriate #ifdefs.
The testsuite doesn't yet have a single location for such headers
(testsuite/include or some such, though there is already testsuite/lib
(cough) but if it's just for the one header ...).
  

On Fri, Apr 11, 2014 at 12:32 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>> Several tests used file-static functions and variables that were not
>>> referenced by the code. Even at -O0, clang omits these entities at the
>>> frontend so the tests fail.
>>>
>>> Since it doesn't look like these tests needed this functionality for
>>> what they were testing, I've modified the variables/functions to
>>> either be non-static, or marked them with __attribute__((used)).
>>>
>>> If it's preferred that I use the attribute more pervasively, rather
>>> than just making the entities non-static, I can provide a patch for
>>> that (or some other preferred solution). There's certainly precedent
>>> for both (non-static entities and __attribute__((used)) in the
>>> testsuite already and much more of the former than the latter).
>>>
>>> I have commit-after-review access, so just looking for sign-off here.
>>
>> Yikes.
>>
>> This is becoming more and more painful (not your fault of course!).
>> I can imagine this being a never ending source of regressions.
>>
>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
>
> Failing that,
>
> making the entries non-static without adding a comment to explain why
> things are the way they are will leave things in a more fragile state,

If people only ever test with GCC, yes. Though to a degree I'm happy
enough carrying the burden of providing patches to cleanup test cases
people commit that break clang. We're going to do this anyway for
other sources of breakage, I don't /think/ this particular kind of
breakage would be especially more egregious (possibly more common, but
providing a patch every few months doesn't sound like the end of the
world to me)

Though I agree that it's slightly subtle to make them non-static with
no comment.

> and if we're going to add a comment we might just as well use an
> attribute throughout I guess.
> However using the attribute is, technically, more complicated than
> that because we shouldn't unnecessarily break testing with other
> compilers.

While some test cases use #ifdefs, there are several test cases that
already use __attribute__((used)) unconditionally... so I'm not sure
if there's a problem adding more. But perhaps I misunderstand the
priority/need here.

> That suggests putting the attribute in a macro in a header protected
> by appropriate #ifdefs.
> The testsuite doesn't yet have a single location for such headers
> (testsuite/include or some such, though there is already testsuite/lib
> (cough) but if it's just for the one header ...).
  

On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>> Several tests used file-static functions and variables that were not
>> referenced by the code. Even at -O0, clang omits these entities at the
>> frontend so the tests fail.
>>
>> Since it doesn't look like these tests needed this functionality for
>> what they were testing, I've modified the variables/functions to
>> either be non-static, or marked them with __attribute__((used)).
>>
>> If it's preferred that I use the attribute more pervasively, rather
>> than just making the entities non-static, I can provide a patch for
>> that (or some other preferred solution). There's certainly precedent
>> for both (non-static entities and __attribute__((used)) in the
>> testsuite already and much more of the former than the latter).
>>
>> I have commit-after-review access, so just looking for sign-off here.
>
> Yikes.
>
> This is becoming more and more painful (not your fault of course!).
> I can imagine this being a never ending source of regressions.
>
> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?

Sort of. It does have -femit-all-decls, which, though poorly named,
causes clang to produce definitions for unused static entities and
even unused inline functions (which GCC doesn't do). I'm not sure
where we'd build in passing that flag from the GCC test suite. I'd
rather not have "clang passes the test suite only if you pass this
carefully chosen set of flags".
  

On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie <dblaikie@gmail.com> wrote:
> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>> Several tests used file-static functions and variables that were not
>>> referenced by the code. Even at -O0, clang omits these entities at the
>>> frontend so the tests fail.
>>>
>>> Since it doesn't look like these tests needed this functionality for
>>> what they were testing, I've modified the variables/functions to
>>> either be non-static, or marked them with __attribute__((used)).
>>>
>>> If it's preferred that I use the attribute more pervasively, rather
>>> than just making the entities non-static, I can provide a patch for
>>> that (or some other preferred solution). There's certainly precedent
>>> for both (non-static entities and __attribute__((used)) in the
>>> testsuite already and much more of the former than the latter).
>>>
>>> I have commit-after-review access, so just looking for sign-off here.
>>
>> Yikes.
>>
>> This is becoming more and more painful (not your fault of course!).
>> I can imagine this being a never ending source of regressions.
>>
>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
>
> Sort of. It does have -femit-all-decls, which, though poorly named,
> causes clang to produce definitions for unused static entities and
> even unused inline functions (which GCC doesn't do).

By default GCC does not keep unused inline functions but there is an
option for that -fkeep-inline-functions.


Thanks,
Andrew Pinski

> I'm not sure
> where we'd build in passing that flag from the GCC test suite. I'd
> rather not have "clang passes the test suite only if you pass this
> carefully chosen set of flags".
  

On Fri, Apr 11, 2014 at 9:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie <dblaikie@gmail.com> wrote:
>> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
>>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>>> Several tests used file-static functions and variables that were not
>>>> referenced by the code. Even at -O0, clang omits these entities at the
>>>> frontend so the tests fail.
>>>>
>>>> Since it doesn't look like these tests needed this functionality for
>>>> what they were testing, I've modified the variables/functions to
>>>> either be non-static, or marked them with __attribute__((used)).
>>>>
>>>> If it's preferred that I use the attribute more pervasively, rather
>>>> than just making the entities non-static, I can provide a patch for
>>>> that (or some other preferred solution). There's certainly precedent
>>>> for both (non-static entities and __attribute__((used)) in the
>>>> testsuite already and much more of the former than the latter).
>>>>
>>>> I have commit-after-review access, so just looking for sign-off here.
>>>
>>> Yikes.
>>>
>>> This is becoming more and more painful (not your fault of course!).
>>> I can imagine this being a never ending source of regressions.
>>>
>>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
>>
>> Sort of. It does have -femit-all-decls, which, though poorly named,
>> causes clang to produce definitions for unused static entities and
>> even unused inline functions (which GCC doesn't do).
>
> By default GCC does not keep unused inline functions but there is an
> option for that -fkeep-inline-functions.

Ah, good to know.

My point was that the GDB test suite passes without enabling that flag
for GCC and I think that's somewhat akin to having the suite passable
without having to add -femit-all-decls for Clang. I realize, of
course, that most GDB developers won't be running the test suite with
Clang, but I'm happy to contribute patches when this comes up from
time to time. It's certainly not a pervasive habit across the test
suite to keep everything static - just this handful of tests happen to
do it.

But I'm open to whatever you folks think is the best approach - if
that means Clang only passes the suite when passing particular flags,
so be it. Perhaps there'd be a way we could build that knowledge into
the testsuite itself so that GDB developers who want to use Clang
don't have to duplicate those details locally.

- David
  

On Sun, Apr 13, 2014 at 12:11 AM, David Blaikie <dblaikie@gmail.com> wrote:
> On Fri, Apr 11, 2014 at 9:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
>>>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>>>> Several tests used file-static functions and variables that were not
>>>>> referenced by the code. Even at -O0, clang omits these entities at the
>>>>> frontend so the tests fail.
>>>>>
>>>>> Since it doesn't look like these tests needed this functionality for
>>>>> what they were testing, I've modified the variables/functions to
>>>>> either be non-static, or marked them with __attribute__((used)).
>>>>>
>>>>> If it's preferred that I use the attribute more pervasively, rather
>>>>> than just making the entities non-static, I can provide a patch for
>>>>> that (or some other preferred solution). There's certainly precedent
>>>>> for both (non-static entities and __attribute__((used)) in the
>>>>> testsuite already and much more of the former than the latter).
>>>>>
>>>>> I have commit-after-review access, so just looking for sign-off here.
>>>>
>>>> Yikes.
>>>>
>>>> This is becoming more and more painful (not your fault of course!).
>>>> I can imagine this being a never ending source of regressions.
>>>>
>>>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
>>>
>>> Sort of. It does have -femit-all-decls, which, though poorly named,
>>> causes clang to produce definitions for unused static entities and
>>> even unused inline functions (which GCC doesn't do).
>>
>> By default GCC does not keep unused inline functions but there is an
>> option for that -fkeep-inline-functions.
>
> Ah, good to know.
>
> My point was that the GDB test suite passes without enabling that flag
> for GCC and I think that's somewhat akin to having the suite passable
> without having to add -femit-all-decls for Clang. I realize, of
> course, that most GDB developers won't be running the test suite with
> Clang, but I'm happy to contribute patches when this comes up from
> time to time. It's certainly not a pervasive habit across the test
> suite to keep everything static - just this handful of tests happen to
> do it.
>
> But I'm open to whatever you folks think is the best approach - if
> that means Clang only passes the suite when passing particular flags,
> so be it. Perhaps there'd be a way we could build that knowledge into
> the testsuite itself so that GDB developers who want to use Clang
> don't have to duplicate those details locally.

I don't have a strong preference other than trying to keep things maintainable.

Maybe it would be enough to document the issue in the testsuite coding
standards section of the manual.  This is a really subtle portability
issue though ... *something* in the code would be nice.
  

On Mon, Apr 14, 2014 at 3:56 PM, Doug Evans <dje@google.com> wrote:
> On Sun, Apr 13, 2014 at 12:11 AM, David Blaikie <dblaikie@gmail.com> wrote:
>> On Fri, Apr 11, 2014 at 9:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>>> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
>>>>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>>>>> Several tests used file-static functions and variables that were not
>>>>>> referenced by the code. Even at -O0, clang omits these entities at the
>>>>>> frontend so the tests fail.
>>>>>>
>>>>>> Since it doesn't look like these tests needed this functionality for
>>>>>> what they were testing, I've modified the variables/functions to
>>>>>> either be non-static, or marked them with __attribute__((used)).
>>>>>>
>>>>>> If it's preferred that I use the attribute more pervasively, rather
>>>>>> than just making the entities non-static, I can provide a patch for
>>>>>> that (or some other preferred solution). There's certainly precedent
>>>>>> for both (non-static entities and __attribute__((used)) in the
>>>>>> testsuite already and much more of the former than the latter).
>>>>>>
>>>>>> I have commit-after-review access, so just looking for sign-off here.
>>>>>
>>>>> Yikes.
>>>>>
>>>>> This is becoming more and more painful (not your fault of course!).
>>>>> I can imagine this being a never ending source of regressions.
>>>>>
>>>>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
>>>>
>>>> Sort of. It does have -femit-all-decls, which, though poorly named,
>>>> causes clang to produce definitions for unused static entities and
>>>> even unused inline functions (which GCC doesn't do).
>>>
>>> By default GCC does not keep unused inline functions but there is an
>>> option for that -fkeep-inline-functions.
>>
>> Ah, good to know.
>>
>> My point was that the GDB test suite passes without enabling that flag
>> for GCC and I think that's somewhat akin to having the suite passable
>> without having to add -femit-all-decls for Clang. I realize, of
>> course, that most GDB developers won't be running the test suite with
>> Clang, but I'm happy to contribute patches when this comes up from
>> time to time. It's certainly not a pervasive habit across the test
>> suite to keep everything static - just this handful of tests happen to
>> do it.
>>
>> But I'm open to whatever you folks think is the best approach - if
>> that means Clang only passes the suite when passing particular flags,
>> so be it. Perhaps there'd be a way we could build that knowledge into
>> the testsuite itself so that GDB developers who want to use Clang
>> don't have to duplicate those details locally.
>
> I don't have a strong preference other than trying to keep things maintainable.
>
> Maybe it would be enough to document the issue in the testsuite coding
> standards section of the manual.  This is a really subtle portability
> issue though ... *something* in the code would be nice.

Given that there are, I assume, many test cases that use unused
non-static functions, the functions after my patch will look just like
those. It'd be weird to comment some but not all of them.

But my initial plan had been to put __attribute__((used))
everywhere... I could still do that, if preferred, but I assume it'll
be woefully inconsistent/arbitrary with some tests using "static
__attribute__((used))" and others using non-static functions anyway. I
suppose the presence of a smattering of static+attribute cases would
remind people to do this in cases where they want/need to make the
entity static, but I'm not sure how effective this would be.

So:

1) Use non-static entities (patch already provided)
2) Use __attribute__((used)) (macro'd at the start of each file? in a
common header? protected under #ifdefs or not (there seem to be a
variety of attributes and gnu-isms not protected by #ifdefs, and some
that are)?)
3) Require Clang run the test suite with non-default flags.
  -> preferably with some auto-detection in the test suite to add
those flags whenever running with clang

Are there other options to consider? (I suppose comments rather than
attributes (2) would be an alternative - "this thing is non-static so
Clang will preserve it")
  

David Blaikie writes:
 > On Mon, Apr 14, 2014 at 3:56 PM, Doug Evans <dje@google.com> wrote:
 > > On Sun, Apr 13, 2014 at 12:11 AM, David Blaikie <dblaikie@gmail.com> wrote:
 > >> On Fri, Apr 11, 2014 at 9:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
 > >>> On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie <dblaikie@gmail.com> wrote:
 > >>>> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
 > >>>>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
 > >>>>>> Several tests used file-static functions and variables that were not
 > >>>>>> referenced by the code. Even at -O0, clang omits these entities at the
 > >>>>>> frontend so the tests fail.
 > >>>>>>
 > >>>>>> Since it doesn't look like these tests needed this functionality for
 > >>>>>> what they were testing, I've modified the variables/functions to
 > >>>>>> either be non-static, or marked them with __attribute__((used)).
 > >>>>>>
 > >>>>>> If it's preferred that I use the attribute more pervasively, rather
 > >>>>>> than just making the entities non-static, I can provide a patch for
 > >>>>>> that (or some other preferred solution). There's certainly precedent
 > >>>>>> for both (non-static entities and __attribute__((used)) in the
 > >>>>>> testsuite already and much more of the former than the latter).
 > >>>>>>
 > >>>>>> I have commit-after-review access, so just looking for sign-off here.
 > >>>>>
 > >>>>> Yikes.
 > >>>>>
 > >>>>> This is becoming more and more painful (not your fault of course!).
 > >>>>> I can imagine this being a never ending source of regressions.
 > >>>>>
 > >>>>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
 > >>>>
 > >>>> Sort of. It does have -femit-all-decls, which, though poorly named,
 > >>>> causes clang to produce definitions for unused static entities and
 > >>>> even unused inline functions (which GCC doesn't do).
 > >>>
 > >>> By default GCC does not keep unused inline functions but there is an
 > >>> option for that -fkeep-inline-functions.
 > >>
 > >> Ah, good to know.
 > >>
 > >> My point was that the GDB test suite passes without enabling that flag
 > >> for GCC and I think that's somewhat akin to having the suite passable
 > >> without having to add -femit-all-decls for Clang. I realize, of
 > >> course, that most GDB developers won't be running the test suite with
 > >> Clang, but I'm happy to contribute patches when this comes up from
 > >> time to time. It's certainly not a pervasive habit across the test
 > >> suite to keep everything static - just this handful of tests happen to
 > >> do it.
 > >>
 > >> But I'm open to whatever you folks think is the best approach - if
 > >> that means Clang only passes the suite when passing particular flags,
 > >> so be it. Perhaps there'd be a way we could build that knowledge into
 > >> the testsuite itself so that GDB developers who want to use Clang
 > >> don't have to duplicate those details locally.
 > >
 > > I don't have a strong preference other than trying to keep things maintainable.
 > >
 > > Maybe it would be enough to document the issue in the testsuite coding
 > > standards section of the manual.  This is a really subtle portability
 > > issue though ... *something* in the code would be nice.
 > 
 > Given that there are, I assume, many test cases that use unused
 > non-static functions, the functions after my patch will look just like
 > those. It'd be weird to comment some but not all of them.
 > 
 > But my initial plan had been to put __attribute__((used))
 > everywhere... I could still do that, if preferred, but I assume it'll
 > be woefully inconsistent/arbitrary with some tests using "static
 > __attribute__((used))" and others using non-static functions anyway. I
 > suppose the presence of a smattering of static+attribute cases would
 > remind people to do this in cases where they want/need to make the
 > entity static, but I'm not sure how effective this would be.
 > 
 > So:
 > 
 > 1) Use non-static entities (patch already provided)
 > 2) Use __attribute__((used)) (macro'd at the start of each file? in a
 > common header? protected under #ifdefs or not (there seem to be a
 > variety of attributes and gnu-isms not protected by #ifdefs, and some
 > that are)?)
 > 3) Require Clang run the test suite with non-default flags.
 >   -> preferably with some auto-detection in the test suite to add
 > those flags whenever running with clang
 > 
 > Are there other options to consider? (I suppose comments rather than
 > attributes (2) would be an alternative - "this thing is non-static so
 > Clang will preserve it")

Let's go with (1) but add something to the wiki documenting the issue.

https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards

I'll do the latter.
  

On Wed, Apr 23, 2014 at 2:50 PM, Doug Evans <dje@google.com> wrote:
> David Blaikie writes:
>  > On Mon, Apr 14, 2014 at 3:56 PM, Doug Evans <dje@google.com> wrote:
>  > > On Sun, Apr 13, 2014 at 12:11 AM, David Blaikie <dblaikie@gmail.com> wrote:
>  > >> On Fri, Apr 11, 2014 at 9:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>  > >>> On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie <dblaikie@gmail.com> wrote:
>  > >>>> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans <dje@google.com> wrote:
>  > >>>>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie <dblaikie@gmail.com> wrote:
>  > >>>>>> Several tests used file-static functions and variables that were not
>  > >>>>>> referenced by the code. Even at -O0, clang omits these entities at the
>  > >>>>>> frontend so the tests fail.
>  > >>>>>>
>  > >>>>>> Since it doesn't look like these tests needed this functionality for
>  > >>>>>> what they were testing, I've modified the variables/functions to
>  > >>>>>> either be non-static, or marked them with __attribute__((used)).
>  > >>>>>>
>  > >>>>>> If it's preferred that I use the attribute more pervasively, rather
>  > >>>>>> than just making the entities non-static, I can provide a patch for
>  > >>>>>> that (or some other preferred solution). There's certainly precedent
>  > >>>>>> for both (non-static entities and __attribute__((used)) in the
>  > >>>>>> testsuite already and much more of the former than the latter).
>  > >>>>>>
>  > >>>>>> I have commit-after-review access, so just looking for sign-off here.
>  > >>>>>
>  > >>>>> Yikes.
>  > >>>>>
>  > >>>>> This is becoming more and more painful (not your fault of course!).
>  > >>>>> I can imagine this being a never ending source of regressions.
>  > >>>>>
>  > >>>>> Does clang perchance have a -O0-and-yes-I-really-mean-O0 option?
>  > >>>>
>  > >>>> Sort of. It does have -femit-all-decls, which, though poorly named,
>  > >>>> causes clang to produce definitions for unused static entities and
>  > >>>> even unused inline functions (which GCC doesn't do).
>  > >>>
>  > >>> By default GCC does not keep unused inline functions but there is an
>  > >>> option for that -fkeep-inline-functions.
>  > >>
>  > >> Ah, good to know.
>  > >>
>  > >> My point was that the GDB test suite passes without enabling that flag
>  > >> for GCC and I think that's somewhat akin to having the suite passable
>  > >> without having to add -femit-all-decls for Clang. I realize, of
>  > >> course, that most GDB developers won't be running the test suite with
>  > >> Clang, but I'm happy to contribute patches when this comes up from
>  > >> time to time. It's certainly not a pervasive habit across the test
>  > >> suite to keep everything static - just this handful of tests happen to
>  > >> do it.
>  > >>
>  > >> But I'm open to whatever you folks think is the best approach - if
>  > >> that means Clang only passes the suite when passing particular flags,
>  > >> so be it. Perhaps there'd be a way we could build that knowledge into
>  > >> the testsuite itself so that GDB developers who want to use Clang
>  > >> don't have to duplicate those details locally.
>  > >
>  > > I don't have a strong preference other than trying to keep things maintainable.
>  > >
>  > > Maybe it would be enough to document the issue in the testsuite coding
>  > > standards section of the manual.  This is a really subtle portability
>  > > issue though ... *something* in the code would be nice.
>  >
>  > Given that there are, I assume, many test cases that use unused
>  > non-static functions, the functions after my patch will look just like
>  > those. It'd be weird to comment some but not all of them.
>  >
>  > But my initial plan had been to put __attribute__((used))
>  > everywhere... I could still do that, if preferred, but I assume it'll
>  > be woefully inconsistent/arbitrary with some tests using "static
>  > __attribute__((used))" and others using non-static functions anyway. I
>  > suppose the presence of a smattering of static+attribute cases would
>  > remind people to do this in cases where they want/need to make the
>  > entity static, but I'm not sure how effective this would be.
>  >
>  > So:
>  >
>  > 1) Use non-static entities (patch already provided)
>  > 2) Use __attribute__((used)) (macro'd at the start of each file? in a
>  > common header? protected under #ifdefs or not (there seem to be a
>  > variety of attributes and gnu-isms not protected by #ifdefs, and some
>  > that are)?)
>  > 3) Require Clang run the test suite with non-default flags.
>  >   -> preferably with some auto-detection in the test suite to add
>  > those flags whenever running with clang
>  >
>  > Are there other options to consider? (I suppose comments rather than
>  > attributes (2) would be an alternative - "this thing is non-static so
>  > Clang will preserve it")
>
> Let's go with (1) but add something to the wiki documenting the issue.

Works for me.

Committed in 22842ff63e28b86e0cd40a87186757b2525578f4

> https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards
>
> I'll do the latter.

Thanks a bunch - let me know when you do, I'd be happy to review it.

- David