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

["H.J. Lu" <hjl.tools@gmail.com>]

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.


-- 
H.J.