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

[David Blaikie <dblaikie at gmail dot com>]

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