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

[David Blaikie <dblaikie at gmail dot com>]

On Fri, Apr 11, 2014 at 12:32 PM, Doug Evans <dje@google.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?
>
> Failing that,
>
> making the entries non-static without adding a comment to explain why
> things are the way they are will leave things in a more fragile state,

If people only ever test with GCC, yes. Though to a degree I'm happy
enough carrying the burden of providing patches to cleanup test cases
people commit that break clang. We're going to do this anyway for
other sources of breakage, I don't /think/ this particular kind of
breakage would be especially more egregious (possibly more common, but
providing a patch every few months doesn't sound like the end of the
world to me)

Though I agree that it's slightly subtle to make them non-static with
no comment.

> and if we're going to add a comment we might just as well use an
> attribute throughout I guess.
> However using the attribute is, technically, more complicated than
> that because we shouldn't unnecessarily break testing with other
> compilers.

While some test cases use #ifdefs, there are several test cases that
already use __attribute__((used)) unconditionally... so I'm not sure
if there's a problem adding more. But perhaps I misunderstand the
priority/need here.

> That suggests putting the attribute in a macro in a header protected
> by appropriate #ifdefs.
> The testsuite doesn't yet have a single location for such headers
> (testsuite/include or some such, though there is already testsuite/lib
> (cough) but if it's just for the one header ...).