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

[David Blaikie <dblaikie at gmail dot com>]

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")