Re: [PATCH 0/5] Remove a few hurdles of compiling with clang

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Jun 12, 2017 at 9:44 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2017-06-12 18:23, Andrew Pinski wrote:
>>
>> On Mon, Jun 12, 2017 at 8:54 AM, Simon Marchi <simon.marchi@polymtl.ca>
>> wrote:
>>>
>>>  - gdb: Pass -x c++ to the compiler: GCC (and even the Intel compiler)
>>> supports this option too, at worst it's a neutral change for compiling
>>> with
>>> GCC.
>>
>>
>> Why is this needed?  Why can't you use clang++ or similar to force
>> compiling as C++?
>
>
> Sorry for being unclear.  I do use clang++ and it complains:
>
> $ clang++ test.c
> clang: warning: treating 'c' input as 'c++' when in C++ mode, this behavior
> is deprecated
>
> So using -x c++ is the only way I found to make it work (short of renaming
> the files).

Why is it deprecated?  I don't see a reason for that.


>
>>>  - gdb: Use -Werror when checking for (un)supported warning flags: it
>>> just
>>> forces the behavior to what's already the default with GCC.  Again, it's
>>> neutral at worst, at best it protects us if GCC ever decides to change
>>> its
>>> default behavior.
>>
>>
>>
>> Yes I think this is ok because gcc also does not warn about
>> unsupported warning flags unless there is an error.
>
>
> Hmm that's not what I observe:
>
> $ gcc test.c
> $ gcc test.c -Wfoo
> gcc: error: unrecognized command line option ‘-Wfoo’

Oh right GCC does error out about the positive case, it is the
negative case -Wno-foo it does not error out for.

>
>
>>>  - gdb: Add -Wno-mismatched-tags: We already have a system that tests
>>> which
>>> warning flags are supported by the current compiler, so this flag will
>>> not
>>> be included in the builds with GCC.  So it's neutral for GCC, and
>>> improves
>>> the situation for Clang with almost no effort.
>>
>>
>> This warning is a bug in clang and really should not be warned about
>> in either -Wall or -Wextra.  I have been complaining about this since
>> clang added this option.
>
>
>
>>>
>>>  - linux-low: Remove usage of "register" keyword: That's a good legacy
>>> code
>>> cleanup in any case, IMHO.
>>
>>
>> Yes and no.  I don't think register was deprecated in C++11.
>
>
> From what I understand, the register keyword has more or less no effect on
> compilers today, so it would be pretty much useless.  So we can remove it
> and we don't really care.

I am saying clang is broken here to some extend.  It is warning about
something which is not deprecated.  The fix is correct just I am
saying clang's warning is not correct.

>
>>>
>>>  - Add ATTRIBUTE_PRINTF to trace_start_error: It's actually a legit
>>> warning,
>>> I'm surprised GCC itself doesn't warn about that.
>>
>>
>> This warning does not make sense.  Can you give some more context?
>
>
> Sure, the actual error is:
>
> /home/emaisin/src/binutils-gdb/gdb/gdbserver/../nat/fork-inferior.c:582:13:
> error: format string is not a string literal [-Werror,-Wformat-nonliteral]
>   vwarning (fmt, ap);
>             ^~~
> 1 error generated.
>
> That code is at:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/nat/fork-inferior.c;h=0913409e6dd01e78019e85068f2e34e8e744ec5b;hb=HEAD#l575
>
> The vwarning function is itself marked with ATTRIBUTE_PRINTF:
>
>  31 extern void vwarning (const char *fmt, va_list args)
>  32      ATTRIBUTE_PRINTF (1, 0);
>
> So it makes sense (I think) to complain that the value passed to fmt is not
> a literal, unless that value is also marked as being a format string.  Then
> it pushes the requirement of passing a literal to the caller.

Oh, the warning is a bit weird and worded incorrectly for this case.
the fix is correct though.

Thanks,
Andrew


>
> Simon