Message ID | 20180521121557.16535-1-hjl.tools@gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 60138 invoked by alias); 21 May 2018 12:16:17 -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-##L=##H@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 60050 invoked by uid 89); 21 May 2018 12:16:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=flex, 2017-2018, 20172018 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mga01.intel.com Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 21 May 2018 12:16:02 +0000 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 May 2018 05:15:57 -0700 X-ExtLoop1: 1 Received: from gnu-cfl-1.sc.intel.com ([172.25.70.237]) by orsmga006.jf.intel.com with ESMTP; 21 May 2018 05:15:57 -0700 From: "H.J. Lu" <hjl.tools@gmail.com> To: binutils@sourceware.org Cc: gdb-patches@sourceware.org Subject: [PATCH 1/3] Move gdb/common/diagnostics.h to include/diagnostics.h Date: Mon, 21 May 2018 05:15:55 -0700 Message-Id: <20180521121557.16535-1-hjl.tools@gmail.com> X-IsSubscribed: yes |
Commit Message
H.J. Lu
May 21, 2018, 12:15 p.m. UTC
Move gdb/common/diagnostics.h to include/diagnostics.h so that it can be used in binutils. gdb/ * ada-lex.l: Include "diagnostics.h" instead of "common/diagnostics.h". * unittests/environ-selftests.c: Likewise. * common/diagnostics.h: Moved to ../include. include/ * diagnostics.h: Moved from ../gdb/common/diagnostics.h. --- gdb/ada-lex.l | 2 +- gdb/unittests/environ-selftests.c | 2 +- {gdb/common => include}/diagnostics.h | 10 +++------- 3 files changed, 5 insertions(+), 9 deletions(-) rename {gdb/common => include}/diagnostics.h (92%)
Comments
On 2018-05-21 08:15, H.J. Lu wrote: > Move gdb/common/diagnostics.h to include/diagnostics.h so that it can > be used in binutils. > > gdb/ > > * ada-lex.l: Include "diagnostics.h" instead of > "common/diagnostics.h". > * unittests/environ-selftests.c: Likewise. > * common/diagnostics.h: Moved to ../include. > > include/ > > * diagnostics.h: Moved from ../gdb/common/diagnostics.h. This series looks OK from the GDB side. Thanks, Simon
Hi H.J. > gdb/ > > * ada-lex.l: Include "diagnostics.h" instead of > "common/diagnostics.h". > * unittests/environ-selftests.c: Likewise. > * common/diagnostics.h: Moved to ../include. > > include/ > > * diagnostics.h: Moved from ../gdb/common/diagnostics.h. Approved from the binutils side as well. Please commit. Cheers Nick
On 21 May 2018, at 13:15, H dot J dot Lu <hjl dot tools at gmail dot com> wrote: > Move gdb/common/diagnostics.h to include/diagnostics.h so that it can > be used in binutils. This patch broke building gdb on MacOS with clang (i.e., after ./configure with no options): CXX gdb.o In file included from ../../../binutils-gdb/gdb/gdb.c:19: In file included from ../../../binutils-gdb/gdb/defs.h:531: In file included from ../../../binutils-gdb/gdb/gdbarch.h:39: In file included from ../../../binutils-gdb/gdb/frame.h:72: In file included from ../../../binutils-gdb/gdb/language.h:26: ../../../binutils-gdb/gdb/symtab.h:1361:1: error: _Pragma takes a parenthesized string literal DEF_VEC_P (symtab_ptr); > --- a/gdb/common/diagnostics.h > +++ b/include/diagnostics.h > [snip] > @@ -15,10 +13,8 @@ > You should have received a copy of the GNU General Public License > along with this program. If not, see <http://www.gnu.org/licenses/>. */ > > -#ifndef COMMON_DIAGNOSTICS_H > -#define COMMON_DIAGNOSTICS_H > - > -#include "common/preprocessor.h" Putting this #include back fixes the build. Apparently in this configuration, include/diagnostics.h doesn't otherwise have a definition of STRINGIFY whereas on Linux or other platforms it does, via some coincidence of different host-related includes or something. Cheers, John
On Mon, Jun 4, 2018 at 3:51 AM, John Marshall <John.W.Marshall@glasgow.ac.uk> wrote: > On 21 May 2018, at 13:15, H dot J dot Lu <hjl dot tools at gmail dot com> wrote: >> Move gdb/common/diagnostics.h to include/diagnostics.h so that it can >> be used in binutils. > > This patch broke building gdb on MacOS with clang (i.e., after ./configure with no options): > > CXX gdb.o > In file included from ../../../binutils-gdb/gdb/gdb.c:19: > In file included from ../../../binutils-gdb/gdb/defs.h:531: > In file included from ../../../binutils-gdb/gdb/gdbarch.h:39: > In file included from ../../../binutils-gdb/gdb/frame.h:72: > In file included from ../../../binutils-gdb/gdb/language.h:26: > ../../../binutils-gdb/gdb/symtab.h:1361:1: error: _Pragma takes a parenthesized string literal > DEF_VEC_P (symtab_ptr); > >> --- a/gdb/common/diagnostics.h >> +++ b/include/diagnostics.h >> [snip] >> @@ -15,10 +13,8 @@ >> You should have received a copy of the GNU General Public License >> along with this program. If not, see <http://www.gnu.org/licenses/>. */ >> >> -#ifndef COMMON_DIAGNOSTICS_H >> -#define COMMON_DIAGNOSTICS_H >> - >> -#include "common/preprocessor.h" > > Putting this #include back fixes the build. Apparently in this configuration, include/diagnostics.h doesn't otherwise have a definition of STRINGIFY whereas on Linux or other platforms it does, via some coincidence of different host-related includes or something. Please add #include "common/preprocessor.h" to gdb.c.
On 4 Jun 2018, at 14:12, H.J. Lu <hjl.tools@gmail.com> wrote: > Please add > > #include "common/preprocessor.h" > > to gdb.c. The problem occurs in other source files too. (And I'm not a GDB maintainer, so I won't be adding anything to anything :-)) > Move gdb/common/diagnostics.h to include/diagnostics.h so that it can > be used in binutils. If the intention is to use include/diagnostics.h in binutils outwith GDB, then gdb/common/preprocessor.h will need to follow diagnostics.h to include/. (Or at least a definition of STRINGIFY will need to be in include/ -- but preprocessor.h seems as generic as diagnostics.h in any case, so it seems appropriate to just move it all.) John
On 06/04/2018 02:12 PM, H.J. Lu wrote: > On Mon, Jun 4, 2018 at 3:51 AM, John Marshall > <John.W.Marshall@glasgow.ac.uk> wrote: >> On 21 May 2018, at 13:15, H dot J dot Lu <hjl dot tools at gmail dot com> wrote: >>> Move gdb/common/diagnostics.h to include/diagnostics.h so that it can >>> be used in binutils. >> >> This patch broke building gdb on MacOS with clang (i.e., after ./configure with no options): >> >> CXX gdb.o >> In file included from ../../../binutils-gdb/gdb/gdb.c:19: >> In file included from ../../../binutils-gdb/gdb/defs.h:531: >> In file included from ../../../binutils-gdb/gdb/gdbarch.h:39: >> In file included from ../../../binutils-gdb/gdb/frame.h:72: >> In file included from ../../../binutils-gdb/gdb/language.h:26: >> ../../../binutils-gdb/gdb/symtab.h:1361:1: error: _Pragma takes a parenthesized string literal >> DEF_VEC_P (symtab_ptr); I think clang is printing a bogus location here. symtab.h includes vec.h, which includes diagnostics.h and does: /* clang has a bug that makes it warn (-Wunused-function) about unused functions that are the result of the DEF_VEC_* macro expansion. See: https://bugs.llvm.org/show_bug.cgi?id=22712 We specifically ignore this warning for the vec functions when the compiler is clang. */ #ifdef __clang__ # define DIAGNOSTIC_IGNORE_UNUSED_VEC_FUNCTION \ DIAGNOSTIC_IGNORE_UNUSED_FUNCTION #else # define DIAGNOSTIC_IGNORE_UNUSED_VEC_FUNCTION #endif and that's most certainly what is tripping on the _Pragma+STRINGIFY. >> >>> --- a/gdb/common/diagnostics.h >>> +++ b/include/diagnostics.h >>> [snip] >>> @@ -15,10 +13,8 @@ >>> You should have received a copy of the GNU General Public License >>> along with this program. If not, see <http://www.gnu.org/licenses/>. */ >>> >>> -#ifndef COMMON_DIAGNOSTICS_H >>> -#define COMMON_DIAGNOSTICS_H >>> - >>> -#include "common/preprocessor.h" >> >> Putting this #include back fixes the build. Apparently in this configuration, include/diagnostics.h doesn't otherwise have a definition of STRINGIFY whereas on Linux or other platforms it does, via some coincidence of different host-related includes or something. > > Please add > > #include "common/preprocessor.h" > > to gdb.c. > I don't think so, see above. Where does binutils get the definition of STRINGIFY from? Your other patch does: > --- a/include/diagnostics.h > +++ b/include/diagnostics.h > @@ -19,8 +19,13 @@ > #ifdef __GNUC__ > # define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") > # define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") > + > +/* Stringification. */ > +# define DIAGNOSTIC_STRINGIFY_1(x) #x > +# define DIAGNOSTIC_STRINGIFY(x) DIAGNOSTIC_STRINGIFY_1 (x) > + > # define DIAGNOSTIC_IGNORE(option) \ > - _Pragma (STRINGIFY (GCC diagnostic ignored option)) > + _Pragma (DIAGNOSTIC_STRINGIFY (GCC diagnostic ignored option)) > #else So I'm surprised by your suggestion. That DIAGNOSTIC_STRINGIFY bit should be split out of that other patch and pushed in separately, IMO. Alternatively, preprocessor.h should be shared too. Thanks, Pedro Alves
On Mon, Jun 4, 2018 at 6:35 AM, Pedro Alves <palves@redhat.com> wrote: > On 06/04/2018 02:12 PM, H.J. Lu wrote: >> On Mon, Jun 4, 2018 at 3:51 AM, John Marshall >> <John.W.Marshall@glasgow.ac.uk> wrote: >>> On 21 May 2018, at 13:15, H dot J dot Lu <hjl dot tools at gmail dot com> wrote: >>>> Move gdb/common/diagnostics.h to include/diagnostics.h so that it can >>>> be used in binutils. >>> >>> This patch broke building gdb on MacOS with clang (i.e., after ./configure with no options): >>> >>> CXX gdb.o >>> In file included from ../../../binutils-gdb/gdb/gdb.c:19: >>> In file included from ../../../binutils-gdb/gdb/defs.h:531: >>> In file included from ../../../binutils-gdb/gdb/gdbarch.h:39: >>> In file included from ../../../binutils-gdb/gdb/frame.h:72: >>> In file included from ../../../binutils-gdb/gdb/language.h:26: >>> ../../../binutils-gdb/gdb/symtab.h:1361:1: error: _Pragma takes a parenthesized string literal >>> DEF_VEC_P (symtab_ptr); > > I think clang is printing a bogus location here. symtab.h includes > vec.h, which includes diagnostics.h and does: > > /* clang has a bug that makes it warn (-Wunused-function) about unused functions > that are the result of the DEF_VEC_* macro expansion. See: > > https://bugs.llvm.org/show_bug.cgi?id=22712 > > We specifically ignore this warning for the vec functions when the compiler > is clang. */ > #ifdef __clang__ > # define DIAGNOSTIC_IGNORE_UNUSED_VEC_FUNCTION \ > DIAGNOSTIC_IGNORE_UNUSED_FUNCTION > #else > # define DIAGNOSTIC_IGNORE_UNUSED_VEC_FUNCTION > #endif > > and that's most certainly what is tripping on the _Pragma+STRINGIFY. > >>> >>>> --- a/gdb/common/diagnostics.h >>>> +++ b/include/diagnostics.h >>>> [snip] >>>> @@ -15,10 +13,8 @@ >>>> You should have received a copy of the GNU General Public License >>>> along with this program. If not, see <http://www.gnu.org/licenses/>. */ >>>> >>>> -#ifndef COMMON_DIAGNOSTICS_H >>>> -#define COMMON_DIAGNOSTICS_H >>>> - >>>> -#include "common/preprocessor.h" >>> >>> Putting this #include back fixes the build. Apparently in this configuration, include/diagnostics.h doesn't otherwise have a definition of STRINGIFY whereas on Linux or other platforms it does, via some coincidence of different host-related includes or something. >> >> Please add >> >> #include "common/preprocessor.h" >> >> to gdb.c. >> > > I don't think so, see above. > > Where does binutils get the definition of STRINGIFY from? > > Your other patch does: > >> --- a/include/diagnostics.h >> +++ b/include/diagnostics.h >> @@ -19,8 +19,13 @@ >> #ifdef __GNUC__ >> # define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") >> # define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") >> + >> +/* Stringification. */ >> +# define DIAGNOSTIC_STRINGIFY_1(x) #x >> +# define DIAGNOSTIC_STRINGIFY(x) DIAGNOSTIC_STRINGIFY_1 (x) >> + >> # define DIAGNOSTIC_IGNORE(option) \ >> - _Pragma (STRINGIFY (GCC diagnostic ignored option)) >> + _Pragma (DIAGNOSTIC_STRINGIFY (GCC diagnostic ignored option)) >> #else > > So I'm surprised by your suggestion. That DIAGNOSTIC_STRINGIFY > bit should be split out of that other patch and pushed in > separately, IMO. Alternatively, preprocessor.h should be shared too. > I pushed my second patch and will submit a follow up patch to address your concern on GCC version.
diff --git a/gdb/ada-lex.l b/gdb/ada-lex.l index c83a619833..621ebb2a95 100644 --- a/gdb/ada-lex.l +++ b/gdb/ada-lex.l @@ -41,7 +41,7 @@ POSEXP (e"+"?{NUM10}) %{ -#include "common/diagnostics.h" +#include "diagnostics.h" /* Some old versions of flex generate code that uses the "register" keyword, which clang warns about. This was observed for example with flex 2.5.35, diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c index a66e8c7a48..31b7ebf9c1 100644 --- a/gdb/unittests/environ-selftests.c +++ b/gdb/unittests/environ-selftests.c @@ -20,7 +20,7 @@ #include "defs.h" #include "selftest.h" #include "common/environ.h" -#include "common/diagnostics.h" +#include "diagnostics.h" static const char gdb_selftest_env_var[] = "GDB_SELFTEST_ENVIRON"; diff --git a/gdb/common/diagnostics.h b/include/diagnostics.h similarity index 92% rename from gdb/common/diagnostics.h rename to include/diagnostics.h index e631f506de..0725664177 100644 --- a/gdb/common/diagnostics.h +++ b/include/diagnostics.h @@ -1,7 +1,5 @@ /* Copyright (C) 2017-2018 Free Software Foundation, Inc. - This file is part of GDB. - This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation; either version 3 of the License, or @@ -15,10 +13,8 @@ You should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>. */ -#ifndef COMMON_DIAGNOSTICS_H -#define COMMON_DIAGNOSTICS_H - -#include "common/preprocessor.h" +#ifndef DIAGNOSTICS_H +#define DIAGNOSTICS_H #ifdef __GNUC__ # define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") @@ -61,4 +57,4 @@ #endif -#endif /* COMMON_DIAGNOSTICS_H */ +#endif /* DIAGNOSTICS_H */