diff mbox

[1/3] Move gdb/common/diagnostics.h to include/diagnostics.h

Message ID 20180521121557.16535-1-hjl.tools@gmail.com
State New, archived
Headers

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

Simon Marchi May 21, 2018, 1:19 p.m. UTC | #1 
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
Nick Clifton June 1, 2018, 7:49 a.m. UTC | #2 
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
John Marshall June 4, 2018, 10:51 a.m. UTC | #3 
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
H.J. Lu June 4, 2018, 1:12 p.m. UTC | #4 
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.
John Marshall June 4, 2018, 1:24 p.m. UTC | #5 
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
Pedro Alves June 4, 2018, 1:35 p.m. UTC | #6 
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
H.J. Lu June 4, 2018, 1:37 p.m. UTC | #7 
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 mbox

Patch

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 */