From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15675 invoked by alias); 15 Apr 2014 03:24:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 15662 invoked by uid 89); 15 Apr 2014 03:24:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qg0-f47.google.com Received: from mail-qg0-f47.google.com (HELO mail-qg0-f47.google.com) (209.85.192.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 15 Apr 2014 03:24:37 +0000 Received: by mail-qg0-f47.google.com with SMTP id i50so1692304qgf.6 for ; Mon, 14 Apr 2014 20:24:35 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.224.114.209 with SMTP id f17mr24373491qaq.40.1397532275236; Mon, 14 Apr 2014 20:24:35 -0700 (PDT) Received: by 10.140.30.74 with HTTP; Mon, 14 Apr 2014 20:24:35 -0700 (PDT) In-Reply-To: References: Date: Tue, 15 Apr 2014 03:24:00 -0000 Message-ID: Subject: Re: [patch] Fix unused static symbols so they're not dropped by clang From: David Blaikie To: Doug Evans Cc: Andrew Pinski , gdb-patches , Eric Christopher Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg00287.txt.bz2 On Mon, Apr 14, 2014 at 3:56 PM, Doug Evans wrote: > On Sun, Apr 13, 2014 at 12:11 AM, David Blaikie wrote: >> On Fri, Apr 11, 2014 at 9:00 PM, Andrew Pinski wrote: >>> On Fri, Apr 11, 2014 at 1:17 PM, David Blaikie wrote: >>>> On Fri, Apr 11, 2014 at 11:23 AM, Doug Evans wrote: >>>>> On Thu, Apr 10, 2014 at 11:51 PM, David Blaikie 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")