Re: [patch] Fix unused static symbols so they're not dropped by clang

[Doug Evans <dje at google dot com>]

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.