Message ID | CAENS6Es8jHe=xp1Ca0WiZ2dg2JNqJ5JYLP89CCBGWWqPQ20rrQ@mail.gmail.com |
---|---|
State | Committed |
Headers |
Return-Path: <x14314964@homiemail-mx23.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 393A8360078 for <siddhesh@wilcox.dreamhost.com>; Thu, 10 Apr 2014 23:51:59 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14314964) id A20F86277B298; Thu, 10 Apr 2014 23:51:58 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 5C9E86277B232 for <gdb@patchwork.siddhesh.in>; Thu, 10 Apr 2014 23:51:58 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:date:message-id:subject:from:to :content-type; q=dns; s=default; b=V+lpskE1sCO5QPXTwwoqQTyN3bpC8 c997z/MnfAjc5xRcLVN5nV99J6EwfHu3R6AWKpYOOt4KB1dlQMgYI+KiJ32AP5xt g+41svTIztURCwYfHDXzoKUigOqfCA3QclH0T5qYwXXZ1V4aGnIysqdRdu70HlRl pWkOj7CgxQfL6o= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:date:message-id:subject:from:to :content-type; s=default; bh=Qq+QTLKDhdLf2rQKkMi7PmMTw5c=; b=BjD EMcHM2ultwTYoPJvEKy2IHvxJwhvMh/SJJpRIfqJSYGulhoTUeihqd3fBA9ByH4X bpMdIF2nn3iwiG835tbPwgVqkcLgxPG3PhG+TLqLLUEFF7COxBvQVEUTvMjT26Qt +y9VQvlbL31ZzGE96gdCgms1L+/4aUBhslvNfipE= Received: (qmail 7490 invoked by alias); 11 Apr 2014 06:51:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-gdb=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 7477 invoked by uid 89); 11 Apr 2014 06:51:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f52.google.com Received: from mail-qa0-f52.google.com (HELO mail-qa0-f52.google.com) (209.85.216.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 11 Apr 2014 06:51:52 +0000 Received: by mail-qa0-f52.google.com with SMTP id s7so2956090qap.11 for <gdb-patches@sourceware.org>; Thu, 10 Apr 2014 23:51:50 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.224.5.136 with SMTP id 8mr26707255qav.42.1397199110003; Thu, 10 Apr 2014 23:51:50 -0700 (PDT) Received: by 10.140.30.74 with HTTP; Thu, 10 Apr 2014 23:51:49 -0700 (PDT) Date: Thu, 10 Apr 2014 23:51:49 -0700 Message-ID: <CAENS6Es8jHe=xp1Ca0WiZ2dg2JNqJ5JYLP89CCBGWWqPQ20rrQ@mail.gmail.com> Subject: [patch] Fix unused static symbols so they're not dropped by clang From: David Blaikie <dblaikie@gmail.com> To: gdb-patches@sourceware.org, Eric Christopher <echristo@gmail.com>, Doug Evans <dje@google.com> Content-Type: multipart/mixed; boundary=001a11c2c59aa1f0eb04f6bec463 X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in |
Commit Message
David Blaikie
April 11, 2014, 6:51 a.m. UTC
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. Thanks, - David commit d72b042cbd4f0af6d2e4ac6901445677b93f77ee Author: David Blaikie <dblaikie@gmail.com> Date: Thu Apr 10 23:45:28 2014 -0700 Ensure unreferenced static symbols aren't omitted by clang (either marking them __attribute__((used)) or making them non-static) gdb/testsuite/ * gdb.base/catch-syscall.c: Make unreferenced statics non-static to ensure clang would not discard them. * gdb.base/gdbvars.c: Ditto. * gdb.base/memattr.c: Ditto. * gdb.base/whatis.c: Ditto. * gdb.python/py-prettyprint.c: Ditto. * gdb.trace/actions.c: Ditto. * gdb.cp/ptype-cv-cp.cc: Mark unused global const int as used to ensure clang would not discard it.
Comments
> On 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. I think clang should change here rather than the testsuite of gdb. Unused static functions make sense to be kept around at -O0 for debugging reasons. Thanks, Andrew > > 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. > > Thanks, > - David > <unused.diff>
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?
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, 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. 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 ...).
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 ...).
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). I'm not sure where we'd build in passing that flag from the GCC test suite. I'd rather not have "clang passes the test suite only if you pass this carefully chosen set of flags".
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. Thanks, Andrew Pinski > I'm not sure > where we'd build in passing that flag from the GCC test suite. I'd > rather not have "clang passes the test suite only if you pass this > carefully chosen set of flags".
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. - David
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.
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")
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.
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
diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog index 12ed4f9..623831a 100644 --- gdb/testsuite/ChangeLog +++ gdb/testsuite/ChangeLog @@ -1,3 +1,15 @@ +2014-04-10 David Blaikie <dblaikie@gmail.com> + + * gdb.base/catch-syscall.c: Make unreferenced statics non-static to + ensure clang would not discard them. + * gdb.base/gdbvars.c: Ditto. + * gdb.base/memattr.c: Ditto. + * gdb.base/whatis.c: Ditto. + * gdb.python/py-prettyprint.c: Ditto. + * gdb.trace/actions.c: Ditto. + * gdb.cp/ptype-cv-cp.cc: Mark unused global const int as used to + ensure clang would not discard it. + 2014-04-10 Pedro Alves <palves@redhat.com> * gdb.base/cond-eval-mode.c: New file. diff --git gdb/testsuite/gdb.base/catch-syscall.c gdb/testsuite/gdb.base/catch-syscall.c index aa5727a..ea33b93 100644 --- gdb/testsuite/gdb.base/catch-syscall.c +++ gdb/testsuite/gdb.base/catch-syscall.c @@ -14,16 +14,16 @@ /* These are the syscalls numbers used by the test. */ -static int close_syscall = SYS_close; -static int chroot_syscall = SYS_chroot; +int close_syscall = SYS_close; +int chroot_syscall = SYS_chroot; /* GDB had a bug where it couldn't catch syscall number 0 (PR 16297). In most GNU/Linux architectures, syscall number 0 is restart_syscall, which can't be called from userspace. However, the "read" syscall is zero on x86_64. */ -static int read_syscall = SYS_read; -static int pipe_syscall = SYS_pipe; -static int write_syscall = SYS_write; -static int exit_group_syscall = SYS_exit_group; +int read_syscall = SYS_read; +int pipe_syscall = SYS_pipe; +int write_syscall = SYS_write; +int exit_group_syscall = SYS_exit_group; int main (void) diff --git gdb/testsuite/gdb.base/gdbvars.c gdb/testsuite/gdb.base/gdbvars.c index 352a76b..46fa84b 100644 --- gdb/testsuite/gdb.base/gdbvars.c +++ gdb/testsuite/gdb.base/gdbvars.c @@ -4,12 +4,12 @@ typedef void *ptr; ptr p = &p; -static void +void foo_void (void) { } -static int +int foo_int (void) { return 0; diff --git gdb/testsuite/gdb.base/memattr.c gdb/testsuite/gdb.base/memattr.c index 74b2d61..62868b6 100644 --- gdb/testsuite/gdb.base/memattr.c +++ gdb/testsuite/gdb.base/memattr.c @@ -16,11 +16,11 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. */ #define MEMSIZE 64 -static int mem1[MEMSIZE] = {111, 222, 333, 444, 555}; -static int mem2[MEMSIZE]; -static int mem3[MEMSIZE]; -static int mem4[MEMSIZE]; -static int mem5[MEMSIZE]; +int mem1[MEMSIZE] = {111, 222, 333, 444, 555}; +int mem2[MEMSIZE]; +int mem3[MEMSIZE]; +int mem4[MEMSIZE]; +int mem5[MEMSIZE]; int main() { diff --git gdb/testsuite/gdb.base/whatis.c gdb/testsuite/gdb.base/whatis.c index a1a3188..bcda5fd 100644 --- gdb/testsuite/gdb.base/whatis.c +++ gdb/testsuite/gdb.base/whatis.c @@ -88,14 +88,14 @@ double v_double_array[2]; a special case kludge in GDB (Unix system include files like to define caddr_t), but for a variety of types. */ typedef char *char_addr; -static char_addr a_char_addr; +char_addr a_char_addr; typedef unsigned short *ushort_addr; -static ushort_addr a_ushort_addr; +ushort_addr a_ushort_addr; typedef signed long *slong_addr; -static slong_addr a_slong_addr; +slong_addr a_slong_addr; #ifndef NO_LONG_LONG typedef signed long long *slong_long_addr; -static slong_long_addr a_slong_long_addr; +slong_long_addr a_slong_long_addr; #endif char *v_char_pointer; diff --git gdb/testsuite/gdb.cp/ptype-cv-cp.cc gdb/testsuite/gdb.cp/ptype-cv-cp.cc index 6546f68..add4021 100644 --- gdb/testsuite/gdb.cp/ptype-cv-cp.cc +++ gdb/testsuite/gdb.cp/ptype-cv-cp.cc @@ -22,7 +22,7 @@ typedef volatile const_my_int volatile_const_my_int; typedef const volatile_my_int const_volatile_my_int; my_int v_my_int (0); -const_my_int v_const_my_int (1); +__attribute__((used)) const_my_int v_const_my_int (1); volatile_my_int v_volatile_my_int (2); const_volatile_my_int v_const_volatile_my_int (3); volatile_const_my_int v_volatile_const_my_int (4); diff --git gdb/testsuite/gdb.python/py-prettyprint.c gdb/testsuite/gdb.python/py-prettyprint.c index 0fd05f5..817bf33 100644 --- gdb/testsuite/gdb.python/py-prettyprint.c +++ gdb/testsuite/gdb.python/py-prettyprint.c @@ -230,7 +230,7 @@ struct nullstr struct string_repr string_1 = { { "one" } }; struct string_repr string_2 = { { "two" } }; -static int +int eval_func (int p1, int p2, int p3, int p4, int p5, int p6, int p7, int p8) { return p1; diff --git gdb/testsuite/gdb.trace/actions.c gdb/testsuite/gdb.trace/actions.c index 04c69f2..497d04d 100644 --- gdb/testsuite/gdb.trace/actions.c +++ gdb/testsuite/gdb.trace/actions.c @@ -116,7 +116,7 @@ unsigned long gdb_c_test( unsigned long *parm ) return ( (unsigned long) 0 ); } -static void gdb_asm_test (void) +void gdb_asm_test (void) { }