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

[Pedro Alves <palves@redhat.com>]

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