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

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

[Simon Marchi]

It is currently possible to build with clang by jumping through a few hoops and
compiling without -Werror, but it is not pretty.  There is a _ton_ of warnings.
clang often gives some good and relevant warnings (e.g. [1]), so it would be
useful to get the number to a reasonnable level to be able to see those that
are actually relevant.  I started to work on the lowest hanging fruits and the
changes that should not be too controversial.

[1] https://sourceware.org/ml/gdb-patches/2017-06/msg00252.html

Simon Marchi (5):
  gdb: Pass -x c++ to the compiler
  gdb: Use -Werror when checking for (un)supported warning flags
  gdb: Add -Wno-mismatched-tags
  linux-low: Remove usage of "register" keyword
  Add ATTRIBUTE_PRINTF to trace_start_error

 gdb/Makefile.in           |  2 +-
 gdb/configure             |  7 ++++---
 gdb/gdbserver/Makefile.in |  2 +-
 gdb/gdbserver/configure   | 13 +++++++------
 gdb/gdbserver/linux-low.c | 16 ++++++++--------
 gdb/nat/fork-inferior.h   |  2 +-
 gdb/warning.m4            |  7 ++++---
 7 files changed, 26 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH 4/5] linux-low: Remove usage of "register" keyword

[Simon Marchi]

AFAIK, the register keyword is not relevant today, and clang complains
about it:

/home/emaisin/src/binutils-gdb/gdb/gdbserver/linux-low.c:5873:3: error: 'register' storage class specifier is deprecated and incompatible with C++1z
      [-Werror,-Wdeprecated-register]
  register PTRACE_XFER_TYPE *buffer;
  ^~~~~~~~~

I think we can safely remove it.

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_read_memory, linux_write_memory): Remove
	usage of "register" keyword.
---
 gdb/gdbserver/linux-low.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 7fbf744..c8e8d08 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5870,11 +5870,11 @@ static int
 linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 {
   int pid = lwpid_of (current_thread);
-  register PTRACE_XFER_TYPE *buffer;
-  register CORE_ADDR addr;
-  register int count;
+  PTRACE_XFER_TYPE *buffer;
+  CORE_ADDR addr;
+  int count;
   char filename[64];
-  register int i;
+  int i;
   int ret;
   int fd;
 
@@ -5958,16 +5958,16 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 static int
 linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
 {
-  register int i;
+  int i;
   /* Round starting address down to longword boundary.  */
-  register CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
+  CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
   /* Round ending address up; get number of longwords that makes.  */
-  register int count
+  int count
     = (((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
     / sizeof (PTRACE_XFER_TYPE);
 
   /* Allocate buffer of that many longwords.  */
-  register PTRACE_XFER_TYPE *buffer = XALLOCAVEC (PTRACE_XFER_TYPE, count);
+  PTRACE_XFER_TYPE *buffer = XALLOCAVEC (PTRACE_XFER_TYPE, count);
 
   int pid = lwpid_of (current_thread);
 
-- 
2.7.4

[PATCH 1/5] gdb: Pass -x c++ to the compiler

[Simon Marchi]

Because we are compiling .c files containing C++ code, clang++ complains
with:

  clang: error: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated

If renaming all the source files to .cpp is out of the question, an
alternative is to pass "-x c++" to convince the compiler that we are
really compiling C++.  It works fine with GCC too.

gdb/ChangeLog:

	* Makefile.in (COMPILE.pre): Add "-x c++".

gdb/gdbserver/ChangeLog:

	* Makefile.in (COMPILE.pre): Add "-x c++".
---
 gdb/Makefile.in           | 2 +-
 gdb/gdbserver/Makefile.in | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5e5fcaa..153a5dd 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -114,7 +114,7 @@ depcomp = $(SHELL) $(srcdir)/../depcomp
 
 # Note that these are overridden by GNU make-specific code below if
 # GNU make is used.  The overrides implement dependency tracking.
-COMPILE.pre = $(CXX) $(CXX_DIALECT)
+COMPILE.pre = $(CXX) -x c++ $(CXX_DIALECT)
 COMPILE.post = -c -o $@
 COMPILE = $(COMPILE.pre) $(INTERNAL_CFLAGS) $(COMPILE.post)
 POSTCOMPILE = @true
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 834425d..4a031e1 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -65,7 +65,7 @@ depcomp = $(SHELL) $(srcdir)/../depcomp
 
 # Note that these are overridden by GNU make-specific code below if
 # GNU make is used.  The overrides implement dependency tracking.
-COMPILE.pre = $(CXX) $(CXX_DIALECT)
+COMPILE.pre = $(CXX) -x c++ $(CXX_DIALECT)
 COMPILE.post = -c -o $@
 COMPILE = $(COMPILE.pre) $(INTERNAL_CFLAGS) $(COMPILE.post)
 POSTCOMPILE = @true
-- 
2.7.4

[PATCH 5/5] Add ATTRIBUTE_PRINTF to trace_start_error

[Simon Marchi]

clang complains that the fmt passed to vwarning in trace_start_error is
not a literal.  This looks like a fair warning, which can be removed by
adding ATTRIBUTE_PRINTF to the declaration of trace_start_error.

gdb/ChangeLog:

	* nat/fork-inferior.h (trace_start_error): Add ATTRIBUTE_PRINTF.
---
 gdb/nat/fork-inferior.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index 10e3832..d369cff 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -95,7 +95,7 @@ extern void gdb_flush_out_err ();
    (i.e., when the "traceme_fun" callback is called on fork_inferior)
    and bail out.  This function does not return.  */
 extern void trace_start_error (const char *fmt, ...)
-  ATTRIBUTE_NORETURN;
+  ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 
 /* Like "trace_start_error", but the error message is constructed by
    combining STRING with the system error message for errno.  This
-- 
2.7.4

Re: [PATCH 5/5] Add ATTRIBUTE_PRINTF to trace_start_error

[Sergio Durigan Junior]

On Saturday, June 10 2017, Simon Marchi wrote:

> clang complains that the fmt passed to vwarning in trace_start_error is
> not a literal.  This looks like a fair warning, which can be removed by
> adding ATTRIBUTE_PRINTF to the declaration of trace_start_error.
>
> gdb/ChangeLog:
>
> 	* nat/fork-inferior.h (trace_start_error): Add ATTRIBUTE_PRINTF.
> ---
>  gdb/nat/fork-inferior.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
> index 10e3832..d369cff 100644
> --- a/gdb/nat/fork-inferior.h
> +++ b/gdb/nat/fork-inferior.h
> @@ -95,7 +95,7 @@ extern void gdb_flush_out_err ();
>     (i.e., when the "traceme_fun" callback is called on fork_inferior)
>     and bail out.  This function does not return.  */
>  extern void trace_start_error (const char *fmt, ...)
> -  ATTRIBUTE_NORETURN;
> +  ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>  
>  /* Like "trace_start_error", but the error message is constructed by
>     combining STRING with the system error message for errno.  This
> -- 
> 2.7.4

Thanks, this looks good to me.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH 3/5] gdb: Add -Wno-mismatched-tags

[Simon Marchi]

clang complains that for some types, we use both the class and struct
keywords in different places.  It's not really a problem, so I think we
can safely turn this warning off.

gdb/ChangeLog:

	* configure: Re-generate.
	* warning.m4 (build_warnings): Add -Wno-mismatched-tags.

gdb/gdbserver/ChangeLog:

	* configure: Re-generate.
---
 gdb/configure           | 3 ++-
 gdb/gdbserver/configure | 3 ++-
 gdb/warning.m4          | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/configure b/gdb/configure
index df2747a..755e3dc 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -15151,7 +15151,8 @@ build_warnings="-Wall -Wpointer-arith \
 -Wno-unused -Wunused-value -Wunused-function \
 -Wno-switch -Wno-char-subscripts \
 -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
--Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized"
+-Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
+-Wno-mismatched-tags"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 2550c7b..35aeabc 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -7148,7 +7148,8 @@ build_warnings="-Wall -Wpointer-arith \
 -Wno-unused -Wunused-value -Wunused-function \
 -Wno-switch -Wno-char-subscripts \
 -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
--Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized"
+-Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
+-Wno-mismatched-tags"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.
diff --git a/gdb/warning.m4 b/gdb/warning.m4
index 00e0f38..0b6aaab 100644
--- a/gdb/warning.m4
+++ b/gdb/warning.m4
@@ -40,7 +40,8 @@ build_warnings="-Wall -Wpointer-arith \
 -Wno-unused -Wunused-value -Wunused-function \
 -Wno-switch -Wno-char-subscripts \
 -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable \
--Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized"
+-Wno-sign-compare -Wno-narrowing -Wno-error=maybe-uninitialized \
+-Wno-mismatched-tags"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.
-- 
2.7.4

[PATCH 2/5] gdb: Use -Werror when checking for (un)supported warning flags

[Simon Marchi]

In warning.m4, we pass all the warning flags one by one to the compiler
to test if they are supported by this particular compiler.  If the
compiler exits with an error, we conclude that this warning flag is not
supported and exclude it.  This allows us to use warning flags without
having to worry about which versions of which compilers support each
flag.

clang, by default, only emits a warning if an unknown flag is passed:

  warning: unknown warning option '-Wfoo' [-Wunknown-warning-option]

The result is that we think that all the warning flags we use are
supported by clang (they are not), and the compilation fails later when
building with -Werror, since the aforementioned warning becomes an
error.  The fix is to also pass -Werror when probing for supported
flags, then we'll correctly get an error when using an unknown warning,
and we'll exclude it:

  error: unknown warning option '-Wfoo' [-Werror,-Wunknown-warning-option]

I am not sure why there is a change in a random comment in
gdbserver/configure, but I suppose it's a leftfover from a previous
patch, so I included it.

gdb/ChangeLog:

	* configure: Re-generate.
	* warning.m4: Pass -Werror to compiler when checking for
	supported warning flags.

gdb/gdbserver/ChangeLog:

	* configure: Re-generate.
---
 gdb/configure           |  4 ++--
 gdb/gdbserver/configure | 10 +++++-----
 gdb/warning.m4          |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/configure b/gdb/configure
index 1f15d5d3..df2747a 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -15222,9 +15222,9 @@ $as_echo_n "checking compiler warning flags... " >&6; }
 	*)
 	    # Check whether GCC accepts it.
 	    saved_CFLAGS="$CFLAGS"
-	    CFLAGS="$CFLAGS $wtest"
+	    CFLAGS="$CFLAGS -Werror $wtest"
 	    saved_CXXFLAGS="$CXXFLAGS"
-	    CXXFLAGS="$CXXFLAGS $wtest"
+	    CXXFLAGS="$CXXFLAGS -Werror $wtest"
 	    cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index b314c41..2550c7b 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -7219,9 +7219,9 @@ $as_echo_n "checking compiler warning flags... " >&6; }
 	*)
 	    # Check whether GCC accepts it.
 	    saved_CFLAGS="$CFLAGS"
-	    CFLAGS="$CFLAGS $wtest"
+	    CFLAGS="$CFLAGS -Werror $wtest"
 	    saved_CXXFLAGS="$CXXFLAGS"
-	    CXXFLAGS="$CXXFLAGS $wtest"
+	    CXXFLAGS="$CXXFLAGS -Werror $wtest"
 	    cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 
@@ -7498,9 +7498,9 @@ _ACEOF
 fi
 
 
-# See if <sys/user.h> supports the %fs_base and %gs_base amd64 segment
-# registers.  Older amd64 Linux's don't have the fs_base and gs_base
-# members of `struct user_regs_struct'.
+# See if <sys/user.h> supports the %fs_base and %gs_bas amd64 segment registers.
+# Older amd64 Linux's don't have the fs_base and gs_base members of
+# `struct user_regs_struct'.
 ac_fn_c_check_member "$LINENO" "struct user_regs_struct" "fs_base" "ac_cv_member_struct_user_regs_struct_fs_base" "#include <sys/user.h>
 "
 if test "x$ac_cv_member_struct_user_regs_struct_fs_base" = x""yes; then :
diff --git a/gdb/warning.m4 b/gdb/warning.m4
index 98e7453..00e0f38 100644
--- a/gdb/warning.m4
+++ b/gdb/warning.m4
@@ -103,9 +103,9 @@ then
 	*)
 	    # Check whether GCC accepts it.
 	    saved_CFLAGS="$CFLAGS"
-	    CFLAGS="$CFLAGS $wtest"
+	    CFLAGS="$CFLAGS -Werror $wtest"
 	    saved_CXXFLAGS="$CXXFLAGS"
-	    CXXFLAGS="$CXXFLAGS $wtest"
+	    CXXFLAGS="$CXXFLAGS -Werror $wtest"
 	    AC_TRY_COMPILE([],[],WARN_CFLAGS="${WARN_CFLAGS} $w",)
 	    CFLAGS="$saved_CFLAGS"
 	    CXXFLAGS="$saved_CXXFLAGS"
-- 
2.7.4

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

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Sat, 10 Jun 2017 21:58:04 +0200
> 
> It is currently possible to build with clang by jumping through a few hoops and
> compiling without -Werror, but it is not pretty.  There is a _ton_ of warnings.
> clang often gives some good and relevant warnings (e.g. [1]), so it would be
> useful to get the number to a reasonnable level to be able to see those that
> are actually relevant.  I started to work on the lowest hanging fruits and the
> changes that should not be too controversial.
> 
> [1] https://sourceware.org/ml/gdb-patches/2017-06/msg00252.html
> 
> Simon Marchi (5):
>   gdb: Pass -x c++ to the compiler
>   gdb: Use -Werror when checking for (un)supported warning flags
>   gdb: Add -Wno-mismatched-tags
>   linux-low: Remove usage of "register" keyword
>   Add ATTRIBUTE_PRINTF to trace_start_error

Question to Joel and Pedro: Do we really want to go to these length to
accommodate clang?  What's our policy?

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

[Yao Qi]

Eli Zaretskii <eliz@gnu.org> writes:

> Question to Joel and Pedro: Do we really want to go to these length to
> accommodate clang?  What's our policy?

I'd like to see GDB can be built by clang.  What is more, I'd like to
get rid of any unnecessary GNU-specific or GCC assumptions from GDB
tests.  It is a good thing to build and test GDB with different compilers.

-- 
Yao (齐尧)

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

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: Simon Marchi <simon.marchi@ericsson.com>,  gdb-patches@sourceware.org
> Date: Mon, 12 Jun 2017 08:56:51 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Question to Joel and Pedro: Do we really want to go to these length to
> > accommodate clang?  What's our policy?
> 
> I'd like to see GDB can be built by clang.  What is more, I'd like to
> get rid of any unnecessary GNU-specific or GCC assumptions from GDB
> tests.  It is a good thing to build and test GDB with different compilers.

That's not the issue: AFAIU, GDB already builds with clang.

The issue is how much effort would we want to invest for that, and
what are we willing to give up when using GCC to be able to use other
compilers.  For example, the proposed patch adds an explicit "-x c++"
switch to _all_ compilations, and also tweaks the warning switches.
I'm not sure we want GCC builds to be affected so that clang builds
could be cleaner.  But maybe we have a policy about this which deems
these issues acceptable?

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

[Simon Marchi]

On 2017-06-12 16:35, Eli Zaretskii wrote:
> That's not the issue: AFAIU, GDB already builds with clang.
> 
> The issue is how much effort would we want to invest for that, and
> what are we willing to give up when using GCC to be able to use other
> compilers.  For example, the proposed patch adds an explicit "-x c++"
> switch to _all_ compilations, and also tweaks the warning switches.
> I'm not sure we want GCC builds to be affected so that clang builds
> could be cleaner.  But maybe we have a policy about this which deems
> these issues acceptable?

Hi Eli,

I included in this patchset the changes that I think improve the 
situation with Clang, but are neutral to GCC, so I don't think these 
should pose any problem.  Here is what I have to say about each patch:

  - 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.

  - 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.

  - 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.

  - linux-low: Remove usage of "register" keyword: That's a good legacy 
code cleanup in any case, IMHO.

  - Add ATTRIBUTE_PRINTF to trace_start_error: It's actually a legit 
warning, I'm surprised GCC itself doesn't warn about that.

But I think it's a good thing to discuss how far we're willing to go to 
make GDB build cleanly with Clang, because some other issues are not so 
easily resolved.  Some warnings are a bit silly and don't provide much 
value (e.g. [1] or -Wmismatched-tags), so it may not make sense to go 
too far out of our way to make it happy.

I think it's also good to have this discussion because of how relevant 
Clang is nowadays.  A big number of software developers are on OS 
X/macOS, on which Clang is the default compiler (shipped with Xcode).  
Making the source more Clang-friendly removes a barrier to them 
contributing to GDB.

Simon

[1] https://bugs.llvm.org//show_bug.cgi?id=22712#c1

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

[Andrew Pinski]

On Mon, Jun 12, 2017 at 8:54 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2017-06-12 16:35, Eli Zaretskii wrote:
>>
>> That's not the issue: AFAIU, GDB already builds with clang.
>>
>> The issue is how much effort would we want to invest for that, and
>> what are we willing to give up when using GCC to be able to use other
>> compilers.  For example, the proposed patch adds an explicit "-x c++"
>> switch to _all_ compilations, and also tweaks the warning switches.
>> I'm not sure we want GCC builds to be affected so that clang builds
>> could be cleaner.  But maybe we have a policy about this which deems
>> these issues acceptable?
>
>
> Hi Eli,
>
> I included in this patchset the changes that I think improve the situation
> with Clang, but are neutral to GCC, so I don't think these should pose any
> problem.  Here is what I have to say about each patch:
>
>  - 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++?

>
>  - 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.

>
>  - 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.

>
>  - 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?

Thanks,
Andrew

>
> But I think it's a good thing to discuss how far we're willing to go to make
> GDB build cleanly with Clang, because some other issues are not so easily
> resolved.  Some warnings are a bit silly and don't provide much value (e.g.
> [1] or -Wmismatched-tags), so it may not make sense to go too far out of our
> way to make it happy.
>
> I think it's also good to have this discussion because of how relevant Clang
> is nowadays.  A big number of software developers are on OS X/macOS, on
> which Clang is the default compiler (shipped with Xcode).  Making the source
> more Clang-friendly removes a barrier to them contributing to GDB.
>
> Simon
>
> [1] https://bugs.llvm.org//show_bug.cgi?id=22712#c1

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

[Pedro Alves]

On 06/12/2017 05:23 PM, Andrew Pinski wrote:
> On Mon, Jun 12, 2017 at 8:54 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:

>>  - 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.

IIRC, the reason this warning exists is because Microsoft's compilers
mangle "struct" and "class" differently, so for projects that
want to be portable to that compiler, it's a helpful warning.
(Whether that should ever be part of -Wall is a separate matter...)

I don't think we'd want to bend backwards to support MSVC
though.  It's so non-conforming that it's scary.  Disabling
that warning is the right thing to do, IMO.

Thanks,
Pedro Alves

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

[Andrew Pinski]

On Mon, Jun 12, 2017 at 9:35 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/12/2017 05:23 PM, Andrew Pinski wrote:
>> On Mon, Jun 12, 2017 at 8:54 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
>>>  - 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.
>
> IIRC, the reason this warning exists is because Microsoft's compilers
> mangle "struct" and "class" differently, so for projects that
> want to be portable to that compiler, it's a helpful warning.
> (Whether that should ever be part of -Wall is a separate matter...)
>
> I don't think we'd want to bend backwards to support MSVC
> though.  It's so non-conforming that it's scary.  Disabling
> that warning is the right thing to do, IMO.

Why not have clang disable this warning by default instead?
I am sorry but people who write C++ should understand that they are the same.

Thanks,
Andrew

>
> Thanks,
> Pedro Alves
>

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

[Pedro Alves]

On 06/12/2017 05:37 PM, Andrew Pinski wrote:
> On Mon, Jun 12, 2017 at 9:35 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/12/2017 05:23 PM, Andrew Pinski wrote:
>>> On Mon, Jun 12, 2017 at 8:54 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>
>>>>  - 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.
>>
>> IIRC, the reason this warning exists is because Microsoft's compilers
>> mangle "struct" and "class" differently, so for projects that
>> want to be portable to that compiler, it's a helpful warning.
>> (Whether that should ever be part of -Wall is a separate matter...)
>>
>> I don't think we'd want to bend backwards to support MSVC
>> though.  It's so non-conforming that it's scary.  Disabling
>> that warning is the right thing to do, IMO.
> 
> Why not have clang disable this warning by default instead?

You'll have to ask clang developers.

> I am sorry but people who write C++ should understand that they are the same.

We know the standard says so, and I know that that's true on the ABIs
we care about.  But in practice, what compilers (a project
cares about) matters more than what the standard says.  After all,
if the standard says the language does X, but no gcc behaves that
way, we wouldn't ever want code to depend on X, would we?

Thanks,
Pedro Alves

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

[Pedro Alves]

On 06/12/2017 05:45 PM, Pedro Alves wrote:
> On 06/12/2017 05:37 PM, Andrew Pinski wrote:
>> On Mon, Jun 12, 2017 at 9:35 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 06/12/2017 05:23 PM, Andrew Pinski wrote:
>>>> On Mon, Jun 12, 2017 at 8:54 AM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>>
>>>>>  - 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.
>>>
>>> IIRC, the reason this warning exists is because Microsoft's compilers
>>> mangle "struct" and "class" differently, so for projects that
>>> want to be portable to that compiler, it's a helpful warning.
>>> (Whether that should ever be part of -Wall is a separate matter...)
>>>
>>> I don't think we'd want to bend backwards to support MSVC
>>> though.  It's so non-conforming that it's scary.  Disabling
>>> that warning is the right thing to do, IMO.
>>
>> Why not have clang disable this warning by default instead?
> 
> You'll have to ask clang developers.
> 
>> I am sorry but people who write C++ should understand that they are the same.
> 
> We know the standard says so, and I know that that's true on the ABIs
> we care about.  But in practice, what compilers (a project
> cares about) matters more than what the standard says.  After all,
> if the standard says the language does X, but no gcc behaves that
> way, we wouldn't ever want code to depend on X, would we?

For reference, so no one thinks I'm making this up:  :-)

  https://bugzilla.mozilla.org/show_bug.cgi?id=1026535

Thanks,
Pedro Alves

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

[Simon Marchi]

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).

>>  - 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’


>>  - 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.

>> 
>>  - 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.

Simon

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

[Andrew Pinski]

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

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

[Simon Marchi]

On 2017-06-12 18:55, Andrew Pinski wrote:
> On Mon, Jun 12, 2017 at 9:44 AM, Simon Marchi <simon.marchi@polymtl.ca> 
> wrote:
>> 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.

I don't know.

Simon

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

[Eli Zaretskii]

> Date: Mon, 12 Jun 2017 17:54:42 +0200
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Yao Qi <qiyaoltc@gmail.com>, simon.marchi@ericsson.com,
>  gdb-patches@sourceware.org
> 
> I included in this patchset the changes that I think improve the 
> situation with Clang, but are neutral to GCC, so I don't think these 
> should pose any problem.

Yes, I know.

> But I think it's a good thing to discuss how far we're willing to go to 
> make GDB build cleanly with Clang, because some other issues are not so 
> easily resolved.

That's precisely the reason why I raised this: it would be good to
have a clear policy on this, to avoid the need for unnecessary work
and unnecessary disputes.  (I actually hoped we already did have such
a policy, but if not, I think we should try to come up with it.)

Thanks.

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

[Yao Qi]

Eli Zaretskii <eliz@gnu.org> writes:

> That's precisely the reason why I raised this: it would be good to
> have a clear policy on this, to avoid the need for unnecessary work
> and unnecessary disputes.  (I actually hoped we already did have such
> a policy, but if not, I think we should try to come up with it.)

In general, it is good to keep GDB built by different popular compilers,
so people are easy to build GDB and different warnings from different
compilers will catch more bugs in GDB.  On the other hand, GCC is still
the primary compiler to build GDB, and support of other compilers in
building GDB should not undermine the case that GDB is built by GCC.
For example, it is not acceptable to build GDB with compiler X but break
the build with GCC.  We still must fix the GDB build failure with GCC, as
what we did in the past, and we welcome the contributions to fix the GDB
build with other compilers.

-- 
Yao (齐尧)

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

[Simon Marchi]

On 2017-06-13 11:14 AM, Yao Qi wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> That's precisely the reason why I raised this: it would be good to
>> have a clear policy on this, to avoid the need for unnecessary work
>> and unnecessary disputes.  (I actually hoped we already did have such
>> a policy, but if not, I think we should try to come up with it.)
> 
> In general, it is good to keep GDB built by different popular compilers,
> so people are easy to build GDB and different warnings from different
> compilers will catch more bugs in GDB.  On the other hand, GCC is still
> the primary compiler to build GDB, and support of other compilers in
> building GDB should not undermine the case that GDB is built by GCC.
> For example, it is not acceptable to build GDB with compiler X but break
> the build with GCC.  We still must fix the GDB build failure with GCC, as
> what we did in the past, and we welcome the contributions to fix the GDB
> build with other compilers.
> 

I think that makes sense.

If somebody is willing to do the work and that it doesn't degrade the code quality,
we should have no problem accepting it.  So if it's a "side-step" that allows both
compilers to be happy, that's ok.  As a patch submitter, if you use primarily GCC,
you are not required to test your patches with Clang, but if you use primarily Clang,
you must test your patch with GCC (a version that's easily accessible for you).

Does that sound like a good rule?

I have a concrete example that is currently in the pipeline.  I hit this warning/error:

/home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: variable 'i' is incremented both in the loop header and in the loop body [-Werror,-Wfor-loop-analysis]
      i++;
      ^
/home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: incremented here
  ALL_DEBUG_ADDRESS_REGISTERS (i)
                               ^

which would require changing this code:

  ALL_DEBUG_ADDRESS_REGISTERS (i)
    {
      ...
      i++;
    }

to either the expansion of the macro:

  for (int i = DR_FIRSTADDR; i <= DR_LASTADDR; i += 2)
    {
      ...
    }

or to a new macro that would take into account the increment:

  ITER_DEBUG_ADDRESS_REGISTERS (i, i += 2)  // other users would use i++
    {
      ...
    }

or something else.

I don't think it makes the code worst.  One could even argue that the current code
breaks the "abstraction" of the macro anyway, so it's not ideal.  But overall, I think
that eliminating the error like this is better than adding -Wno-for-loop-analysis, because
if I wrote code like this and it was actually a mistake, I would like the compiler to tell
me.

Simon

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

[Pedro Alves]

On 06/13/2017 11:23 AM, Simon Marchi wrote:

> I have a concrete example that is currently in the pipeline.  I hit this warning/error:
> 
> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: variable 'i' is incremented both in the loop header and in the loop body [-Werror,-Wfor-loop-analysis]
>       i++;
>       ^
> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: incremented here
>   ALL_DEBUG_ADDRESS_REGISTERS (i)
>                                ^
> 
> which would require changing this code:
> 
>   ALL_DEBUG_ADDRESS_REGISTERS (i)
>     {
>       ...
>       i++;
>     }
> 
> to either the expansion of the macro:
> 
>   for (int i = DR_FIRSTADDR; i <= DR_LASTADDR; i += 2)
>     {
>       ...
>     }

This, IMO.  I think it's just that one place?

> 
> or to a new macro that would take into account the increment:
> 
>   ITER_DEBUG_ADDRESS_REGISTERS (i, i += 2)  // other users would use i++
>     {
>       ...
>     }
> 

That'd obfuscate too much for no real benefit, IMO.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2017-06-13 13:06, Pedro Alves wrote:
> On 06/13/2017 11:23 AM, Simon Marchi wrote:
> 
>> I have a concrete example that is currently in the pipeline.  I hit 
>> this warning/error:
>> 
>> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: 
>> variable 'i' is incremented both in the loop header and in the loop 
>> body [-Werror,-Wfor-loop-analysis]
>>       i++;
>>       ^
>> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: 
>> incremented here
>>   ALL_DEBUG_ADDRESS_REGISTERS (i)
>>                                ^
>> 
>> which would require changing this code:
>> 
>>   ALL_DEBUG_ADDRESS_REGISTERS (i)
>>     {
>>       ...
>>       i++;
>>     }
>> 
>> to either the expansion of the macro:
>> 
>>   for (int i = DR_FIRSTADDR; i <= DR_LASTADDR; i += 2)
>>     {
>>       ...
>>     }
> 
> This, IMO.  I think it's just that one place?

It's the only instance of that particular problem, yes.

>> or to a new macro that would take into account the increment:
>> 
>>   ITER_DEBUG_ADDRESS_REGISTERS (i, i += 2)  // other users would use 
>> i++
>>     {
>>       ...
>>     }
>> 
> 
> That'd obfuscate too much for no real benefit, IMO.

Agreed.

Simon

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

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Tue, 13 Jun 2017 12:23:27 +0200
> 
> If somebody is willing to do the work and that it doesn't degrade the code quality,
> we should have no problem accepting it.  So if it's a "side-step" that allows both
> compilers to be happy, that's ok.  As a patch submitter, if you use primarily GCC,
> you are not required to test your patches with Clang, but if you use primarily Clang,
> you must test your patch with GCC (a version that's easily accessible for you).
> 
> Does that sound like a good rule?

Let's not forget that code submitted by someone who is willing to do
the work stays in GDB even when that someone is no longer willing to
support that compiler.  So there are additional factors to consider
when making this decision.

> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: variable 'i' is incremented both in the loop header and in the loop body [-Werror,-Wfor-loop-analysis]
>       i++;
>       ^
> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: incremented here
>   ALL_DEBUG_ADDRESS_REGISTERS (i)
>                                ^

Why is incrementing a loop variable in the body an error?  It's
perfectly valid code.

> I think
> that eliminating the error like this is better than adding -Wno-for-loop-analysis, because
> if I wrote code like this and it was actually a mistake, I would like the compiler to tell
> me.

Does clang have the equivalent of "#pragma push"?  If it does, we
could disable this warning only for clang and only for that code
snippet.

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

[Simon Marchi]

On 2017-06-13 16:38, Eli Zaretskii wrote:
> Let's not forget that code submitted by someone who is willing to do
> the work stays in GDB even when that someone is no longer willing to
> support that compiler.  So there are additional factors to consider
> when making this decision.

Indeed.  As Pedro said, we shouldn't do the same kind of efforts for 
$OBSCURE_COMPILER than to support a widely used one (like Clang).

> 
>> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: 
>> variable 'i' is incremented both in the loop header and in the loop 
>> body [-Werror,-Wfor-loop-analysis]
>>       i++;
>>       ^
>> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: 
>> incremented here
>>   ALL_DEBUG_ADDRESS_REGISTERS (i)
>>                                ^
> 
> Why is incrementing a loop variable in the body an error?  It's
> perfectly valid code.

It's an error only because we use -Werror for development, otherwise it 
would be a warning.  Warnings are meant to point out valid code that may 
not be very good, or a common mistake, or a probable source of bugs.

>> I think
>> that eliminating the error like this is better than adding 
>> -Wno-for-loop-analysis, because
>> if I wrote code like this and it was actually a mistake, I would like 
>> the compiler to tell
>> me.
> 
> Does clang have the equivalent of "#pragma push"?  If it does, we
> could disable this warning only for clang and only for that code
> snippet.

That's indeed a solution, but I'd keep that for the cases where we can't 
find an elegant solution that pleases both GCC and Clang.

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

[Eli Zaretskii]

> Date: Tue, 13 Jun 2017 19:07:29 +0200
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Simon Marchi <simon.marchi@ericsson.com>, qiyaoltc@gmail.com,
>  gdb-patches@sourceware.org
> 
> > Does clang have the equivalent of "#pragma push"?  If it does, we
> > could disable this warning only for clang and only for that code
> > snippet.
> 
> That's indeed a solution, but I'd keep that for the cases where we can't 
> find an elegant solution that pleases both GCC and Clang.

See, I don't consider the proposed solution to be elegant, because it
tweaks a perfectly valid code to placate a stupid compiler warning.
Someone at some point might rightfully ask why didn't we use
ALL_DEBUG_ADDRESS_REGISTERS instead, and might even reinstate the code
you are about to change.  So I prefer to have a seemingly "ugly"
workaround, which nonetheless points out exactly which warning of what
compiler caused it.  That way, at some future point, when clang
hopefully gets its act together, we could revisit the issue and see
that the workaround is no longer needed.

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

[Simon Marchi]

On 2017-06-13 21:22, Eli Zaretskii wrote:
> See, I don't consider the proposed solution to be elegant, because it
> tweaks a perfectly valid code to placate a stupid compiler warning.

It replaces perfectly valid code with some other perfectly valid code.  
That the compiler warning is stupid is your opinion.  I think it's 
useful (even though it may trigger false positives sometimes).

> Someone at some point might rightfully ask why didn't we use
> ALL_DEBUG_ADDRESS_REGISTERS instead, and might even reinstate the code
> you are about to change. So I prefer to have a seemingly "ugly"
> workaround, which nonetheless points out exactly which warning of what
> compiler caused it.  That way, at some future point, when clang
> hopefully gets its act together, we could revisit the issue and see
> that the workaround is no longer needed.

We can always add comments like

   /* Don't use ALL_DEBUG_ADDRESS_REGISTERS here to silence Clang's 
-Wfor-loop-analysis warning.  */

like we have right now for GCC warnings:

breakpoint.c:14747:      /* Initialize it just to avoid a GCC false 
warning.  */

Even though I have a slight preference for not silencing warnings when 
possible, it's really not a strong opinion, I would also be fine with 
the #pragma if that's what people prefer.

Simon

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

[Eli Zaretskii]

> Date: Tue, 13 Jun 2017 22:17:15 +0200
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: qiyaoltc@gmail.com, gdb-patches@sourceware.org
> 
> On 2017-06-13 21:22, Eli Zaretskii wrote:
> > See, I don't consider the proposed solution to be elegant, because it
> > tweaks a perfectly valid code to placate a stupid compiler warning.
> 
> It replaces perfectly valid code with some other perfectly valid code.  

Which is not used anywhere else, right?

> We can always add comments like
> 
>    /* Don't use ALL_DEBUG_ADDRESS_REGISTERS here to silence Clang's 
> -Wfor-loop-analysis warning.  */

That's the least we should do, IMO.

Thanks.

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

[Pedro Alves]

On 06/14/2017 03:29 AM, Eli Zaretskii wrote:
>> Date: Tue, 13 Jun 2017 22:17:15 +0200
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Cc: qiyaoltc@gmail.com, gdb-patches@sourceware.org
>>
>> On 2017-06-13 21:22, Eli Zaretskii wrote:
>>> See, I don't consider the proposed solution to be elegant, because it
>>> tweaks a perfectly valid code to placate a stupid compiler warning.
>>
>> It replaces perfectly valid code with some other perfectly valid code.  
> 
> Which is not used anywhere else, right?
> 
>> We can always add comments like
>>
>>    /* Don't use ALL_DEBUG_ADDRESS_REGISTERS here to silence Clang's 
>> -Wfor-loop-analysis warning.  */
> 
> That's the least we should do, IMO.

I'd vote for saying instead:

   /* Don't use ALL_DEBUG_ADDRESS_REGISTERS here because
      we want to skip over two registers at a time.  */

Or better even, just don't skip two registers at a time?
The code is manually printing two columns on each iteration.
How about the patchlet below instead?  I'd call it a clean up on
its own right.

Note this output is only shown with "maint set show-debug-regs on".
So with this, if someone cared for saving vertical space of the
debug output (I don't), this could be easily tweaked to adjust the number
of debug regs per line printed depending on screen width, for example.
Doing that with the current scheme of printing more than
one register with a single format string wouldn't work that well.

From 2cfc65a311798c519ae393f474302f634aa2595d Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 14 Jun 2017 11:25:19 +0100
Subject: [PATCH] dregs

---
 gdb/nat/x86-dregs.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index 8c8adfa..7fd2d56 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -198,15 +198,14 @@ x86_show_dr (struct x86_debug_reg_state *state,
 		phex (state->dr_status_mirror, 8));
   ALL_DEBUG_ADDRESS_REGISTERS (i)
     {
-      debug_printf ("\
-\tDR%d: addr=0x%s, ref.count=%d  DR%d: addr=0x%s, ref.count=%d\n",
+      debug_printf ("\tDR%d: addr=0x%s, ref.count=%d",
 		    i, phex (state->dr_mirror[i],
 			     x86_get_debug_register_length ()),
-		    state->dr_ref_count[i],
-		    i + 1, phex (state->dr_mirror[i + 1],
-				 x86_get_debug_register_length ()),
-		    state->dr_ref_count[i + 1]);
-      i++;
+		    state->dr_ref_count[i]);
+
+      /* Print two debug registers per line.  */
+      if ((i - DR_FIRSTADDR) % 2)
+	debug_printf ("\n");
     }
 }
 
-- 
2.5.5

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

[John Baldwin]

On 6/14/17 6:45 AM, Pedro Alves wrote:
> Or better even, just don't skip two registers at a time?
> The code is manually printing two columns on each iteration.
> How about the patchlet below instead?  I'd call it a clean up on
> its own right.

I prefer this fix and agree.  Other places that try to print registers
in columns (mips-tdep.c) iterate one register at a time and make the
newline conditional, so this pattern is consistent with that.

-- 
John Baldwin

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

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> If somebody is willing to do the work and that it doesn't degrade the code quality,
> we should have no problem accepting it.  So if it's a "side-step" that allows both
> compilers to be happy, that's ok.  As a patch submitter, if you use primarily GCC,
> you are not required to test your patches with Clang, but if you use primarily Clang,
> you must test your patch with GCC (a version that's easily accessible for you).
>
> Does that sound like a good rule?

Yes, it is equivalent to "it is not acceptable to build GDB with
compiler X but break  the build with GCC" in my last email.

I add some comments from Simon and Pedro.  Eli, is it good to you?

 In general, it is good to keep GDB built by different popular compilers,
 so people are easy to build GDB and different warnings from different
 compilers will catch more bugs in GDB.  On the other hand, GCC is still
 the primary compiler to build GDB, and support of other compilers in
 building GDB should not undermine the case that GDB is built by GCC nor
 degrade the code quality.   For example, it is not acceptable to build
 GDB with compiler X but break the build with GCC.  We still must fix
 the GDB build failure with GCC, as what we did in the past, and we
 welcome the contributions to fix the GDB build with other compilers.

 Ideally, every bug that other compilers find in the GDB source code
 that GCC didn't warn about should be considered as a GCC bug, and we
 should make sure that it's reported on the GCC tracker.

I also looked for the place to add this policy.  Looks the most relevant
page is https://sourceware.org/gdb/wiki/Internals%20Compiler-Warnings

-- 
Yao (齐尧)

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

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Simon Marchi <simon.marchi@polymtl.ca>,  <gdb-patches@sourceware.org>, palves@redhat.com
> Date: Tue, 13 Jun 2017 16:21:54 +0100
> 
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> > If somebody is willing to do the work and that it doesn't degrade the code quality,
> > we should have no problem accepting it.  So if it's a "side-step" that allows both
> > compilers to be happy, that's ok.  As a patch submitter, if you use primarily GCC,
> > you are not required to test your patches with Clang, but if you use primarily Clang,
> > you must test your patch with GCC (a version that's easily accessible for you).
> >
> > Does that sound like a good rule?
> 
> Yes, it is equivalent to "it is not acceptable to build GDB with
> compiler X but break  the build with GCC" in my last email.
> 
> I add some comments from Simon and Pedro.  Eli, is it good to you?

I believe I answered that some time ago.

Specifically, this text sounds too vague to me to be considered
"policy".  Too much is left to judgment calls.  But if everyone else
is happy, I won't insist on prolonging this discussion.

> I also looked for the place to add this policy.  Looks the most relevant
> page is https://sourceware.org/gdb/wiki/Internals%20Compiler-Warnings

I would suggest to consider a new page, or maybe make this part of
coding standards.

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

[Yao Qi]

Eli Zaretskii <eliz@gnu.org> writes:

> Specifically, this text sounds too vague to me to be considered
> "policy".  Too much is left to judgment calls.  But if everyone else
> is happy, I won't insist on prolonging this discussion.
>

It is vague because the tasks this "policy" about are not clear.  It is
unlike other polices like "-Werror policy" or "GDB's #include file
policy", their tasks are quite clear, so it is possible to give clear
actionable rules.  However, it is still unknown that what are we going
to do to make GDB built with compilers other than GCC.  It was just
raised by Simon's patches several days ago.  We need to write this vague
policy down, and don't discourage people building GDB with different
compilers.

Patches are still reviewed, and some of them may be controversial.  As
we have some concrete bad/good patches, we can have some specific rules
in this policy.

>> I also looked for the place to add this policy.  Looks the most relevant
>> page is https://sourceware.org/gdb/wiki/Internals%20Compiler-Warnings
>
> I would suggest to consider a new page, or maybe make this part of
> coding standards.

OK, let me create a new page, "Build GDB with different compilers" in
chapter "Miscellaneous guidelines" in 
https://sourceware.org/gdb/wiki/Internals  It has few to do with coding
standards.  People should still write the code in the same way,
specified in our coding standards, although GDB can be built by
different compilers.

-- 
Yao (齐尧)

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

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

>  In general, it is good to keep GDB built by different popular compilers,
>  so people are easy to build GDB and different warnings from different
>  compilers will catch more bugs in GDB.  On the other hand, GCC is still
>  the primary compiler to build GDB, and support of other compilers in
>  building GDB should not undermine the case that GDB is built by GCC nor
>  degrade the code quality.   For example, it is not acceptable to build
>  GDB with compiler X but break the build with GCC.  We still must fix
>  the GDB build failure with GCC, as what we did in the past, and we
>  welcome the contributions to fix the GDB build with other compilers.
>
>  Ideally, every bug that other compilers find in the GDB source code
>  that GCC didn't warn about should be considered as a GCC bug, and we
>  should make sure that it's reported on the GCC tracker.

I copied it to
https://sourceware.org/gdb/wiki/Internals%20Build-With-Different-Compilers

-- 
Yao (齐尧)

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

[Pedro Alves]

On 06/13/2017 10:14 AM, Yao Qi wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> That's precisely the reason why I raised this: it would be good to
>> have a clear policy on this, to avoid the need for unnecessary work
>> and unnecessary disputes.  (I actually hoped we already did have such
>> a policy, but if not, I think we should try to come up with it.)
> 
> In general, it is good to keep GDB built by different popular compilers,
> so people are easy to build GDB and different warnings from different
> compilers will catch more bugs in GDB.  On the other hand, GCC is still
> the primary compiler to build GDB, and support of other compilers in
> building GDB should not undermine the case that GDB is built by GCC.
> For example, it is not acceptable to build GDB with compiler X but break
> the build with GCC.  We still must fix the GDB build failure with GCC, as
> what we did in the past, and we welcome the contributions to fix the GDB
> build with other compilers.

I'm fine with that, as long as other compilers don't unreasonably hold
us back, when GCC is an option, and the user base that'd want to actually
use such a compiler is small.  E.g., maintenance effort, and migration
to newer language revisions.  Here I don't specifically mean Clang,
but in general.  E.g., exaggerating to illustrate a point, if someone wanted
to port GDB to OddballPlatform used by to handful of people in the world,
and that requires building with a 10-year-old compiler that nobody's
maintaining, then I think it'd be unreasonable for everyone upstream to have
to keep such a port in mind and consider whether such a port would get broken
by some GDB patch or design change.

Other popular compilers nowadays tend to support "GNU C/C++" and impersonate
as GCC (define __GNUC__, etc.), so porting effort tends to not be that big
in practice, fortunately.

The MSVC compiler issue came up in the -Wno-mismatched-tags discussion,
and I think my statements may have sounded too strongly against MSVC.
If someone wants to make GDB buildable with Visual Studio for some reason,
I'd personally be OK with that, as long as that doesn't involve uglifying
the codebase, or holding back migration to newer language revisions.
Modern versions are getting better at standards compliance and gnulib
supports MSVC (ISTR there's some command line compiler wrapper that makes
it possible to run configury with msvc), so it may actually be doable.
I don't know why would someone want to do it, though.  But until someone
seriously steps forward and commits to maintaining such a port, caring
about "-Wmismatched-tags" warnings is useless, since I suspect that
issue would be the least of the porting effort troubles.

BTW, on the different warnings, IMO, ideally every bug that Clang finds in
the code that GCC didn't warn about should be considered a GCC bug,
and we should make sure that it's reported on the GCC tracker.

Thanks,
Pedro Alves

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

[Joel Brobecker]

FWIW, I have a similar position as Pedro and Yao. Unless it causes
extra maintenance issues, or holds us back from trying to implement
new changes, it's interesting to see how other compilers build GDB.
If anything, it increases our chances of catching portability issues;
and, we're also benefiting from the other compilers' warnings that
GCC currently does not warn about.

-- 
Joel

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

[Simon Marchi]

On 2017-06-10 21:58, Simon Marchi wrote:
> It is currently possible to build with clang by jumping through a few 
> hoops and
> compiling without -Werror, but it is not pretty.  There is a _ton_ of 
> warnings.
> clang often gives some good and relevant warnings (e.g. [1]), so it 
> would be
> useful to get the number to a reasonnable level to be able to see those 
> that
> are actually relevant.  I started to work on the lowest hanging fruits 
> and the
> changes that should not be too controversial.
> 
> [1] https://sourceware.org/ml/gdb-patches/2017-06/msg00252.html
> 
> Simon Marchi (5):
>   gdb: Pass -x c++ to the compiler
>   gdb: Use -Werror when checking for (un)supported warning flags
>   gdb: Add -Wno-mismatched-tags
>   linux-low: Remove usage of "register" keyword
>   Add ATTRIBUTE_PRINTF to trace_start_error
> 
>  gdb/Makefile.in           |  2 +-
>  gdb/configure             |  7 ++++---
>  gdb/gdbserver/Makefile.in |  2 +-
>  gdb/gdbserver/configure   | 13 +++++++------
>  gdb/gdbserver/linux-low.c | 16 ++++++++--------
>  gdb/nat/fork-inferior.h   |  2 +-
>  gdb/warning.m4            |  7 ++++---
>  7 files changed, 26 insertions(+), 23 deletions(-)

This is now pushed in.

Simon