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

David Blaikie
2014-04-25T05:36:01Z

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