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

Doug Evans
2014-04-23T21:50:37Z

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.