[PATCH 0/4] Get rid of some more warnings given by clang

[PATCH 0/4] Get rid of some more warnings given by clang

[Simon Marchi]

This is another small series that gets rid of some warnings given by clang.

Simon Marchi (4):
  environ-selftests: Ignore -Wself-move warning
  x86-dregs: Print debug registers one per line
  dtrace-probe: Put semicolon after while on its own line
  main: Don't add int to string

 gdb/dtrace-probe.c                |  3 ++-
 gdb/main.c                        |  2 +-
 gdb/nat/x86-dregs.c               | 16 ++++++----------
 gdb/unittests/environ-selftests.c |  9 +++++++++
 4 files changed, 18 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Simon Marchi]

clang gives this warning:

/home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: error: explicitly moving variable of type 'gdb_environ' to itself [-Werror,-Wself-move]
  env = std::move (env);
  ~~~ ^            ~~~

In this case, ignoring the warning locally is clearly the thing to do,
since it warns exactly about the behavior we want to test.  We also
don't want to disable this globally, because we would want the compiler
if we wrote that in real code.

I filed a bug in GCC's bugzilla to suggest to add this warning:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159

gdb/ChangeLog:

	* unittests/environ-selftests.c (run_tests): Ignore -Wself-move
	warning.
---
 gdb/unittests/environ-selftests.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
index ecc3955..6989c5e 100644
--- a/gdb/unittests/environ-selftests.c
+++ b/gdb/unittests/environ-selftests.c
@@ -136,7 +136,16 @@ run_tests ()
   env.clear ();
   env.set ("A", "1");
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wself-move"
+#endif /* __clang__ */
   env = std::move (env);
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif /* __clang__ */
+
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
   SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
   SELF_CHECK (env.envp ()[1] == NULL);
-- 
2.7.4

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Simon Marchi wrote:

> clang gives this warning:
>
> /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: error: explicitly moving variable of type 'gdb_environ' to itself [-Werror,-Wself-move]
>   env = std::move (env);
>   ~~~ ^            ~~~
>
> In this case, ignoring the warning locally is clearly the thing to do,
> since it warns exactly about the behavior we want to test.  We also
> don't want to disable this globally, because we would want the compiler

"we would want the code compiler to warn"

> if we wrote that in real code.
>
> I filed a bug in GCC's bugzilla to suggest to add this warning:
>
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159

Thanks!

> gdb/ChangeLog:
>
> 	* unittests/environ-selftests.c (run_tests): Ignore -Wself-move
> 	warning.
> ---
>  gdb/unittests/environ-selftests.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
> index ecc3955..6989c5e 100644
> --- a/gdb/unittests/environ-selftests.c
> +++ b/gdb/unittests/environ-selftests.c
> @@ -136,7 +136,16 @@ run_tests ()
>    env.clear ();
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +
> +#ifdef __clang__
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wself-move"
> +#endif /* __clang__ */
>    env = std::move (env);
> +#ifdef __clang__
> +#pragma clang diagnostic pop
> +#endif /* __clang__ */

Wow.  I know we've discussed this before, but this is ugly :-/.  Anyway,
this file is just a unittest, so I'm totally fine with this.  Do you
think it's worth putting a comment on top, just to explicitly say what
this is doing?

Otherwise, LGTM.

> +
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>    SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);
> -- 
> 2.7.4

Thanks,

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

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Simon Marchi]

On 2017-06-21 22:29, Sergio Durigan Junior wrote:
> On Wednesday, June 21 2017, Simon Marchi wrote:
> 
>> clang gives this warning:
>> 
>> /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: 
>> error: explicitly moving variable of type 'gdb_environ' to itself 
>> [-Werror,-Wself-move]
>>   env = std::move (env);
>>   ~~~ ^            ~~~
>> 
>> In this case, ignoring the warning locally is clearly the thing to do,
>> since it warns exactly about the behavior we want to test.  We also
>> don't want to disable this globally, because we would want the 
>> compiler
> 
> "we would want the code compiler to warn"

Oops thanks.

>> if we wrote that in real code.
>> 
>> I filed a bug in GCC's bugzilla to suggest to add this warning:
>> 
>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159
> 
> Thanks!
> 
>> gdb/ChangeLog:
>> 
>> 	* unittests/environ-selftests.c (run_tests): Ignore -Wself-move
>> 	warning.
>> ---
>>  gdb/unittests/environ-selftests.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/gdb/unittests/environ-selftests.c 
>> b/gdb/unittests/environ-selftests.c
>> index ecc3955..6989c5e 100644
>> --- a/gdb/unittests/environ-selftests.c
>> +++ b/gdb/unittests/environ-selftests.c
>> @@ -136,7 +136,16 @@ run_tests ()
>>    env.clear ();
>>    env.set ("A", "1");
>>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>> +
>> +#ifdef __clang__
>> +#pragma clang diagnostic push
>> +#pragma clang diagnostic ignored "-Wself-move"
>> +#endif /* __clang__ */
>>    env = std::move (env);
>> +#ifdef __clang__
>> +#pragma clang diagnostic pop
>> +#endif /* __clang__ */
> 
> Wow.  I know we've discussed this before, but this is ugly :-/.  
> Anyway,
> this file is just a unittest, so I'm totally fine with this.

Yeah, I didn't expect to have to put the #ifdefs for __clang__ though.  
Without them, gcc emits a warning [-Wunknown-pragma].  We always have 
the option to turn -Wunknown-pragma off globally, what do you prefer?

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Simon Marchi wrote:

>>> gdb/ChangeLog:
>>>
>>> 	* unittests/environ-selftests.c (run_tests): Ignore -Wself-move
>>> 	warning.
>>> ---
>>>  gdb/unittests/environ-selftests.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/gdb/unittests/environ-selftests.c
>>> b/gdb/unittests/environ-selftests.c
>>> index ecc3955..6989c5e 100644
>>> --- a/gdb/unittests/environ-selftests.c
>>> +++ b/gdb/unittests/environ-selftests.c
>>> @@ -136,7 +136,16 @@ run_tests ()
>>>    env.clear ();
>>>    env.set ("A", "1");
>>>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>>> +
>>> +#ifdef __clang__
>>> +#pragma clang diagnostic push
>>> +#pragma clang diagnostic ignored "-Wself-move"
>>> +#endif /* __clang__ */
>>>    env = std::move (env);
>>> +#ifdef __clang__
>>> +#pragma clang diagnostic pop
>>> +#endif /* __clang__ */
>>
>> Wow.  I know we've discussed this before, but this is ugly :-/.
>> Anyway,
>> this file is just a unittest, so I'm totally fine with this.
>
> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though.
> Without them, gcc emits a warning [-Wunknown-pragma].  We always have
> the option to turn -Wunknown-pragma off globally, what do you prefer?

I'd prefer to leave the ifdef.  It's just a small part in the
"ugliness".

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

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Pedro Alves]

On 06/21/2017 10:05 PM, Simon Marchi wrote:

> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. 
> Without them, gcc emits a warning [-Wunknown-pragma].  We always have
> the option to turn -Wunknown-pragma off globally, what do you prefer?
> 

Don't both GCC and Clang understand "#pragma GCC diagnostic" instead?

Or better even, wrap it in some macros (and use _Pragma):

 #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push")
 #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop")
 #define DIAGNOSTIC_IGNORE(option) \
   _Pragma (STRINGIFY (GCC diagnostic ignored option))

Alternatively, you could replace the std::move with a cast
to rvalue ref, which is just what std::move really is:

 -env = std::move (env);
 +env = static_cast<gdb_environ &&> (env);

Thanks,
Pedro Alves

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Pedro Alves wrote:

> On 06/21/2017 10:05 PM, Simon Marchi wrote:
>
>> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. 
>> Without them, gcc emits a warning [-Wunknown-pragma].  We always have
>> the option to turn -Wunknown-pragma off globally, what do you prefer?
>> 
>
> Don't both GCC and Clang understand "#pragma GCC diagnostic" instead?
>
> Or better even, wrap it in some macros (and use _Pragma):
>
>  #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push")
>  #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop")
>  #define DIAGNOSTIC_IGNORE(option) \
>    _Pragma (STRINGIFY (GCC diagnostic ignored option))
>
>
> Alternatively, you could replace the std::move with a cast
> to rvalue ref, which is just what std::move really is:
>
>  -env = std::move (env);
>  +env = static_cast<gdb_environ &&> (env);

I don't like this cast TBH.  std::move is more readable.

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

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Simon Marchi]

On 2017-06-21 23:28, Pedro Alves wrote:
> On 06/21/2017 10:05 PM, Simon Marchi wrote:
> 
>> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though.
>> Without them, gcc emits a warning [-Wunknown-pragma].  We always have
>> the option to turn -Wunknown-pragma off globally, what do you prefer?
>> 
> 
> Don't both GCC and Clang understand "#pragma GCC diagnostic" instead?

Yes, but then it's GCC that complains that it doesn't know the warning:

/home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:141:32: 
error: unknown option after ‘#pragma GCC diagnostic’ kind 
[-Werror=pragmas]
  #pragma GCC diagnostic ignored "-Wself-move"
                                 ^

> Or better even, wrap it in some macros (and use _Pragma):
> 
>  #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push")
>  #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop")
>  #define DIAGNOSTIC_IGNORE(option) \
>    _Pragma (STRINGIFY (GCC diagnostic ignored option))

Oh that's interesting.  The gcc doc said that _Pragma was added exactly 
for this purpose (to be usable in macros).  I'll try that.

> Alternatively, you could replace the std::move with a cast
> to rvalue ref, which is just what std::move really is:
> 
>  -env = std::move (env);
>  +env = static_cast<gdb_environ &&> (env);

I guess that with a comment explaining why we use that it would be ok, 
but that would not be my first choice either.

Thanks,

Simon

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Pedro Alves]

On 06/22/2017 08:44 AM, Simon Marchi wrote:
> On 2017-06-21 23:28, Pedro Alves wrote:
>> On 06/21/2017 10:05 PM, Simon Marchi wrote:
>>
>>> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though.
>>> Without them, gcc emits a warning [-Wunknown-pragma].  We always have
>>> the option to turn -Wunknown-pragma off globally, what do you prefer?
>>>
>>
>> Don't both GCC and Clang understand "#pragma GCC diagnostic" instead?
> 
> Yes, but then it's GCC that complains that it doesn't know the warning:
> 
> /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:141:32:
> error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]
>  #pragma GCC diagnostic ignored "-Wself-move"
>                                 ^
> 

Ah, but that's a different kind of complaint.  Since both compilers
understand "#pragma GCC" but only one understands "#pragma clang",
then it seems clearly better to me to write the infrastructure
macros in terms of the former.  It'll make it a bit easier to
reuse the macros for other warnings.

>> Or better even, wrap it in some macros (and use _Pragma):
>>
>>  #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push")
>>  #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop")
>>  #define DIAGNOSTIC_IGNORE(option) \
>>    _Pragma (STRINGIFY (GCC diagnostic ignored option))
> 
> Oh that's interesting.  The gcc doc said that _Pragma was added exactly
> for this purpose (to be usable in macros).  I'll try that.

Right.

Thanks,
Pedro Alves

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Simon Marchi]

On 2017-06-21 22:29, Sergio Durigan Junior wrote:
>> +#ifdef __clang__
>> +#pragma clang diagnostic push
>> +#pragma clang diagnostic ignored "-Wself-move"
>> +#endif /* __clang__ */
>>    env = std::move (env);
>> +#ifdef __clang__
>> +#pragma clang diagnostic pop
>> +#endif /* __clang__ */

> Do you
> think it's worth putting a comment on top, just to explicitly say what
> this is doing?

Forgot to reply to this.  It seemed self-explanatory enough to me that 
ignoring the "self-move" warning just above a blatant self move was 
probably to silence a warning pointing out the self move :).

Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Simon Marchi wrote:

> On 2017-06-21 22:29, Sergio Durigan Junior wrote:
>>> +#ifdef __clang__
>>> +#pragma clang diagnostic push
>>> +#pragma clang diagnostic ignored "-Wself-move"
>>> +#endif /* __clang__ */
>>>    env = std::move (env);
>>> +#ifdef __clang__
>>> +#pragma clang diagnostic pop
>>> +#endif /* __clang__ */
>
>> Do you
>> think it's worth putting a comment on top, just to explicitly say what
>> this is doing?
>
> Forgot to reply to this.  It seemed self-explanatory enough to me that
> ignoring the "self-move" warning just above a blatant self move was
> probably to silence a warning pointing out the self move :).

Yeah, you're right.  I just thought a comment in English would make it
*even* clearer ;-).  But that's just a personal preference, I don't
mind.

Cheers,

-- 
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 v2] environ-selftests: Ignore -Wself-move warning

[Simon Marchi]

New in v2: add wrapper macros in common/diagnostics.h, use them in
environ-selftests.c.

clang gives this warning:

/home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: error: explicitly moving variable of type 'gdb_environ' to itself [-Werror,-Wself-move]
  env = std::move (env);
  ~~~ ^            ~~~

In this case, ignoring the warning locally is clearly the thing to do,
since it warns exactly about the behavior we want to test.  We also
don't want to disable this globally, because we would want the compiler
to warn if we wrote that in real code.

I added the file common/diagnostics.h, in which we can put macros
used to control compiler diagnostics.  This makes the resulting code
less cluttered:

  #ifdef __clang__
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wself-move"
  #endif /* __clang__ */
    env = std::move (env);
  #ifdef __clang__
  #pragma clang diagnostic pop
  #endif /* __clang__ */

vs

  DIAG_PUSH_IGNORE_SELF_MOVE
  env = std::move (env);
  DIAG_POP_IGNORE_SELF_MOVE

I also added a comment in the end, as per Sergio's suggestion.  It's
clear to see that the warning it turned off without it, but I think it's
good to have it to justify why it is.

I filed a bug in GCC's bugzilla to suggest to add this warning:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159

gdb/ChangeLog:

	* unittests/environ-selftests.c (run_tests): Ignore -Wself-move
	warning.
	* common/diagnostics.h: New file.
---
 gdb/common/diagnostics.h          | 35 +++++++++++++++++++++++++++++++++++
 gdb/unittests/environ-selftests.c |  7 +++++++
 2 files changed, 42 insertions(+)
 create mode 100644 gdb/common/diagnostics.h

diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h
new file mode 100644
index 0000000..2bf462c
--- /dev/null
+++ b/gdb/common/diagnostics.h
@@ -0,0 +1,35 @@
+/* Copyright (C) 2017 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
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   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
+
+#if defined (__clang__)
+
+#define DIAG_PUSH_IGNORE_SELF_MOVE \
+  _Pragma("clang diagnostic push") \
+  _Pragma("clang diagnostic ignored \"-Wself-move\"")
+#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop")
+
+#else
+
+#define DIAG_PUSH_IGNORE_SELF_MOVE
+#define DIAG_POP_IGNORE_SELF_MOVE
+
+#endif
+
+#endif /* COMMON_DIAGNOSTICS_H */
diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
index ecc3955..fae9bd0 100644
--- a/gdb/unittests/environ-selftests.c
+++ b/gdb/unittests/environ-selftests.c
@@ -20,6 +20,7 @@
 #include "defs.h"
 #include "selftest.h"
 #include "common/environ.h"
+#include "common/diagnostics.h"
 
 namespace selftests {
 namespace gdb_environ_tests {
@@ -136,7 +137,13 @@ run_tests ()
   env.clear ();
   env.set ("A", "1");
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+
+  /* Some compilers warn about moving to self, but that's precisely what we want
+     to test here, so turn this warning off.  */
+  DIAG_PUSH_IGNORE_SELF_MOVE
   env = std::move (env);
+  DIAG_POP_IGNORE_SELF_MOVE
+
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
   SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
   SELF_CHECK (env.envp ()[1] == NULL);
-- 
2.7.4

Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning

[Pedro Alves]

On 06/22/2017 09:31 AM, Simon Marchi wrote:
> +#if defined (__clang__)
> +
> +#define DIAG_PUSH_IGNORE_SELF_MOVE \
> +  _Pragma("clang diagnostic push") \
> +  _Pragma("clang diagnostic ignored \"-Wself-move\"")
> +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop")
> +
> +#else
> +
> +#define DIAG_PUSH_IGNORE_SELF_MOVE
> +#define DIAG_POP_IGNORE_SELF_MOVE
> +
> +#endif


Let me try writing a quick patch that puts a "STRINGIZE" macro
in common/preprocessor.h, so we can write the above like I had
suggested before, which is a bit more generic.  There are several
copies in the tree of such a macro, so that'll be a good thing
on its own anyway, IMO.

I'm not sure whether the single push-ignore macro is a good idea,
since if we follow the pattern going forward, it requires more
boilerplace if we need different warnings around the same code:

 {
  DIAG_PUSH_IGNORE_WARN1
  DIAG_PUSH_IGNORE_WARN2
  DIAG_PUSH_IGNORE_WARN3
  ...
 
  // some code

  DIAG_POP_IGNORE_WARN3
  DIAG_POP_IGNORE_WARN2
  DIAG_POP_IGNORE_WARN1
 }

vs:


 {
  DIAG_PUSH
  DIAG_IGNORE_WARN1
  DIAG_IGNORE_WARN2
  DIAG_IGNORE_WARN3
  ...

  // some code

  DIAG_POP
 }

Though we can always support both styles.

Thanks,
Pedro Alves

Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning

[Pedro Alves]

On 06/22/2017 10:51 AM, Pedro Alves wrote:
> On 06/22/2017 09:31 AM, Simon Marchi wrote:
>> +#if defined (__clang__)
>> +
>> +#define DIAG_PUSH_IGNORE_SELF_MOVE \
>> +  _Pragma("clang diagnostic push") \
>> +  _Pragma("clang diagnostic ignored \"-Wself-move\"")
>> +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop")
>> +
>> +#else
>> +
>> +#define DIAG_PUSH_IGNORE_SELF_MOVE
>> +#define DIAG_POP_IGNORE_SELF_MOVE
>> +
>> +#endif
> 
> 
> Let me try writing a quick patch that puts a "STRINGIZE" macro
> in common/preprocessor.h, so we can write the above like I had
> suggested before, which is a bit more generic.  There are several
> copies in the tree of such a macro, so that'll be a good thing
> on its own anyway, IMO.
> 
> I'm not sure whether the single push-ignore macro is a good idea,
> since if we follow the pattern going forward, it requires more
> boilerplace if we need different warnings around the same code:

This is what I had in mind.  I think it should work with clang,
though I haven't tried it.


Subject: [PATCH] environ-selftests: Ignore -Wself-move warning

---
 gdb/common/diagnostics.h          | 40 +++++++++++++++++++++++++++++++++++++++
 gdb/unittests/environ-selftests.c |  8 ++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 gdb/common/diagnostics.h

diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h
new file mode 100644
index 0000000..5a63bfd
--- /dev/null
+++ b/gdb/common/diagnostics.h
@@ -0,0 +1,40 @@
+/* Copyright (C) 2017 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
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   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"
+
+#ifdef __GNUC__
+# define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push")
+# define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop")
+# define DIAGNOSTIC_IGNORE(option) \
+  _Pragma (STRINGIFY (GCC diagnostic ignored option))
+#else
+# define DIAGNOSTIC_PUSH
+# define DIAGNOSTIC_POP
+# define DIAGNOSTIC_IGNORE(option)
+#endif
+
+#ifdef __clang__
+# define DIAGNOSTIC_IGNORE_SELF_MOVE DIAGNOSTIC_IGNORE ("-Wself-move")
+#else
+# define DIAGNOSTIC_IGNORE_SELF_MOVE
+#endif
+
+#endif /* COMMON_DIAGNOSTICS_H */
diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
index ecc3955..28b16f8 100644
--- a/gdb/unittests/environ-selftests.c
+++ b/gdb/unittests/environ-selftests.c
@@ -20,6 +20,7 @@
 #include "defs.h"
 #include "selftest.h"
 #include "common/environ.h"
+#include "common/diagnostics.h"
 
 namespace selftests {
 namespace gdb_environ_tests {
@@ -136,7 +137,14 @@ run_tests ()
   env.clear ();
   env.set ("A", "1");
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+
+  /* Some compilers warn about moving to self, but that's precisely what we want
+     to test here, so turn this warning off.  */
+  DIAGNOSTIC_PUSH
+  DIAGNOSTIC_IGNORE_SELF_MOVE
   env = std::move (env);
+  DIAGNOSTIC_POP
+
   SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
   SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
   SELF_CHECK (env.envp ()[1] == NULL);
-- 
2.5.5

Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning

[Simon Marchi]

On 2017-06-22 12:52, Pedro Alves wrote:
> On 06/22/2017 10:51 AM, Pedro Alves wrote:
>> On 06/22/2017 09:31 AM, Simon Marchi wrote:
>>> +#if defined (__clang__)
>>> +
>>> +#define DIAG_PUSH_IGNORE_SELF_MOVE \
>>> +  _Pragma("clang diagnostic push") \
>>> +  _Pragma("clang diagnostic ignored \"-Wself-move\"")
>>> +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop")
>>> +
>>> +#else
>>> +
>>> +#define DIAG_PUSH_IGNORE_SELF_MOVE
>>> +#define DIAG_POP_IGNORE_SELF_MOVE
>>> +
>>> +#endif
>> 
>> 
>> Let me try writing a quick patch that puts a "STRINGIZE" macro
>> in common/preprocessor.h, so we can write the above like I had
>> suggested before, which is a bit more generic.  There are several
>> copies in the tree of such a macro, so that'll be a good thing
>> on its own anyway, IMO.
>> 
>> I'm not sure whether the single push-ignore macro is a good idea,
>> since if we follow the pattern going forward, it requires more
>> boilerplace if we need different warnings around the same code:
> 
> This is what I had in mind.  I think it should work with clang,
> though I haven't tried it.
> 
> 
> Subject: [PATCH] environ-selftests: Ignore -Wself-move warning
> 
> ---
>  gdb/common/diagnostics.h          | 40 
> +++++++++++++++++++++++++++++++++++++++
>  gdb/unittests/environ-selftests.c |  8 ++++++++
>  2 files changed, 48 insertions(+)
>  create mode 100644 gdb/common/diagnostics.h
> 
> diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h
> new file mode 100644
> index 0000000..5a63bfd
> --- /dev/null
> +++ b/gdb/common/diagnostics.h
> @@ -0,0 +1,40 @@
> +/* Copyright (C) 2017 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
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   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"
> +
> +#ifdef __GNUC__
> +# define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push")
> +# define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop")
> +# define DIAGNOSTIC_IGNORE(option) \
> +  _Pragma (STRINGIFY (GCC diagnostic ignored option))
> +#else
> +# define DIAGNOSTIC_PUSH
> +# define DIAGNOSTIC_POP
> +# define DIAGNOSTIC_IGNORE(option)
> +#endif
> +
> +#ifdef __clang__
> +# define DIAGNOSTIC_IGNORE_SELF_MOVE DIAGNOSTIC_IGNORE ("-Wself-move")
> +#else
> +# define DIAGNOSTIC_IGNORE_SELF_MOVE
> +#endif
> +
> +#endif /* COMMON_DIAGNOSTICS_H */
> diff --git a/gdb/unittests/environ-selftests.c
> b/gdb/unittests/environ-selftests.c
> index ecc3955..28b16f8 100644
> --- a/gdb/unittests/environ-selftests.c
> +++ b/gdb/unittests/environ-selftests.c
> @@ -20,6 +20,7 @@
>  #include "defs.h"
>  #include "selftest.h"
>  #include "common/environ.h"
> +#include "common/diagnostics.h"
> 
>  namespace selftests {
>  namespace gdb_environ_tests {
> @@ -136,7 +137,14 @@ run_tests ()
>    env.clear ();
>    env.set ("A", "1");
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
> +
> +  /* Some compilers warn about moving to self, but that's precisely
> what we want
> +     to test here, so turn this warning off.  */
> +  DIAGNOSTIC_PUSH
> +  DIAGNOSTIC_IGNORE_SELF_MOVE
>    env = std::move (env);
> +  DIAGNOSTIC_POP
> +
>    SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>    SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
>    SELF_CHECK (env.envp ()[1] == NULL);

I didn't understand how it would work to conditionally define the pragma 
based on the compiler, but now that I see it I understand.  LTGM.

Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning

[Pedro Alves]

On 06/22/2017 11:57 AM, Simon Marchi wrote:

> I didn't understand how it would work to conditionally define the pragma
> based on the compiler, but now that I see it I understand.  LTGM.

Great, now pushed.

Thanks,
Pedro Alves

[PATCH 4/4] main: Don't add int to string

[Simon Marchi]

clang shows this warning:

  /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: error: adding 'int' to a string does not append to the string [-Werror,-Wstring-plus-int]
                char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len);
                                                 ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
  /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: note: use array indexing to silence this warning
                char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len);
                                                                ^
                                                 &              [            ]

It's quite easy to get rid of it by using &foo[len] instead of foo + len.
I think this warning is relevant to keep enabled, because it can be an
easy mistake to do.

This warning is already discussed here in GCC bugzilla:

  https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00729.html

and a patch series for it was submitted very recently.

gdb/ChangeLog:

	* main.c (get_init_files): Replace "SYSTEM_GDBINIT +
	datadir_len" with "&SYSTEM_GDBINIT[datadir_len]".
---
 gdb/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/main.c b/gdb/main.c
index df4b111..9813041 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -224,7 +224,7 @@ get_init_files (const char **system_gdbinit,
 	    {
 	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
 		 to gdb_datadir.  */
-	      char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len);
+	      char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]);
 	      char *p;
 
 	      for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p)
-- 
2.7.4

Re: [PATCH 4/4] main: Don't add int to string

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Simon Marchi wrote:

> clang shows this warning:
>
>   /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: error: adding 'int' to a string does not append to the string [-Werror,-Wstring-plus-int]
>                 char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len);
>                                                  ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
>   /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: note: use array indexing to silence this warning
>                 char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len);
>                                                                 ^
>                                                  &              [            ]
>
> It's quite easy to get rid of it by using &foo[len] instead of foo + len.
> I think this warning is relevant to keep enabled, because it can be an
> easy mistake to do.
>
> This warning is already discussed here in GCC bugzilla:
>
>   https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00729.html
>
> and a patch series for it was submitted very recently.
>
> gdb/ChangeLog:
>
> 	* main.c (get_init_files): Replace "SYSTEM_GDBINIT +
> 	datadir_len" with "&SYSTEM_GDBINIT[datadir_len]".
> ---
>  gdb/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/main.c b/gdb/main.c
> index df4b111..9813041 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -224,7 +224,7 @@ get_init_files (const char **system_gdbinit,
>  	    {
>  	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
>  		 to gdb_datadir.  */
> -	      char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len);
> +	      char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]);
>  	      char *p;
>  
>  	      for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p)
> -- 
> 2.7.4

LGTM.

Thanks,

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

Re: [PATCH 4/4] main: Don't add int to string

[Simon Marchi]

On 2017-06-21 22:35, Sergio Durigan Junior wrote:
> On Wednesday, June 21 2017, Simon Marchi wrote:
> 
>> clang shows this warning:
>> 
>>   /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: error: adding 
>> 'int' to a string does not append to the string 
>> [-Werror,-Wstring-plus-int]
>>                 char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + 
>> datadir_len);
>>                                                  
>> ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
>>   /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: note: use array 
>> indexing to silence this warning
>>                 char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + 
>> datadir_len);
>>                                                                 ^
>>                                                  &              [      
>>       ]
>> 
>> It's quite easy to get rid of it by using &foo[len] instead of foo + 
>> len.
>> I think this warning is relevant to keep enabled, because it can be an
>> easy mistake to do.
>> 
>> This warning is already discussed here in GCC bugzilla:
>> 
>>   https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00729.html
>> 
>> and a patch series for it was submitted very recently.
>> 
>> gdb/ChangeLog:
>> 
>> 	* main.c (get_init_files): Replace "SYSTEM_GDBINIT +
>> 	datadir_len" with "&SYSTEM_GDBINIT[datadir_len]".
>> ---
>>  gdb/main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gdb/main.c b/gdb/main.c
>> index df4b111..9813041 100644
>> --- a/gdb/main.c
>> +++ b/gdb/main.c
>> @@ -224,7 +224,7 @@ get_init_files (const char **system_gdbinit,
>>  	    {
>>  	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
>>  		 to gdb_datadir.  */
>> -	      char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + 
>> datadir_len);
>> +	      char *tmp_sys_gdbinit = xstrdup 
>> (&SYSTEM_GDBINIT[datadir_len]);
>>  	      char *p;
>> 
>>  	      for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p)
>> --
>> 2.7.4
> 
> LGTM.
> 
> Thanks,

Thanks, pushed.

[PATCH 3/4] dtrace-probe: Put semicolon after while on its own line

[Simon Marchi]

clang shows this warning.

  /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: while loop has empty body [-Werror,-Wempty-body]
            while (*p++ != '\0' && p - strtab < strtab_size);
                                                            ^
  /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put the semicolon on a separate line to silence this warning

Putting the semicolon on its own line is not a big sacrifice to get rid of this
warning.  I think it's also useful to keep this, because it can catch errors
like this:

  while (something);
    {
      ...
    }

although gcc would warn about it in a different way (misleading indentation).

This warning is already discussed here in the GCC bugzilla:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184

gdb/ChangeLog:

	* dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on
	its own line.
---
 gdb/dtrace-probe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 122f8de..9a02694 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile,
 	  arg.type_str = xstrdup (p);
 
 	  /* Use strtab_size as a sentinel.  */
-	  while (*p++ != '\0' && p - strtab < strtab_size);
+	  while (*p++ != '\0' && p - strtab < strtab_size)
+	    ;  /* Silence clang's -Wempty-body warning.  */
 
 	  /* Try to parse a type expression from the type string.  If
 	     this does not work then we set the type to `long
-- 
2.7.4

Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Simon Marchi wrote:

> clang shows this warning.
>
>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: while loop has empty body [-Werror,-Wempty-body]
>             while (*p++ != '\0' && p - strtab < strtab_size);
>                                                             ^
>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put the semicolon on a separate line to silence this warning
>
> Putting the semicolon on its own line is not a big sacrifice to get rid of this
> warning.  I think it's also useful to keep this, because it can catch errors
> like this:
>
>   while (something);
>     {
>       ...
>     }
>
> although gcc would warn about it in a different way (misleading indentation).
>
> This warning is already discussed here in the GCC bugzilla:
>
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184
>
> gdb/ChangeLog:
>
> 	* dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on
> 	its own line.
> ---
>  gdb/dtrace-probe.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index 122f8de..9a02694 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  	  arg.type_str = xstrdup (p);
>  
>  	  /* Use strtab_size as a sentinel.  */
> -	  while (*p++ != '\0' && p - strtab < strtab_size);
> +	  while (*p++ != '\0' && p - strtab < strtab_size)
> +	    ;  /* Silence clang's -Wempty-body warning.  */

Lately I've been choosing to explicitly put "continue;" when the body
doesn't contain anything, like:

	while (*p++ != '\0' && p - strtab < strtab_size)
	  continue;

I don't know what others think about it, but it would solve this problem
and also be more verbose that we're just iterating without a body.

>  
>  	  /* Try to parse a type expression from the type string.  If
>  	     this does not work then we set the type to `long
> -- 
> 2.7.4

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

Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line

[Simon Marchi]

On 2017-06-21 22:34, Sergio Durigan Junior wrote:
> On Wednesday, June 21 2017, Simon Marchi wrote:
> 
>> clang shows this warning.
>> 
>>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: 
>> while loop has empty body [-Werror,-Wempty-body]
>>             while (*p++ != '\0' && p - strtab < strtab_size);
>>                                                             ^
>>   /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put 
>> the semicolon on a separate line to silence this warning
>> 
>> Putting the semicolon on its own line is not a big sacrifice to get 
>> rid of this
>> warning.  I think it's also useful to keep this, because it can catch 
>> errors
>> like this:
>> 
>>   while (something);
>>     {
>>       ...
>>     }
>> 
>> although gcc would warn about it in a different way (misleading 
>> indentation).
>> 
>> This warning is already discussed here in the GCC bugzilla:
>> 
>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184
>> 
>> gdb/ChangeLog:
>> 
>> 	* dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on
>> 	its own line.
>> ---
>>  gdb/dtrace-probe.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
>> index 122f8de..9a02694 100644
>> --- a/gdb/dtrace-probe.c
>> +++ b/gdb/dtrace-probe.c
>> @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile,
>>  	  arg.type_str = xstrdup (p);
>> 
>>  	  /* Use strtab_size as a sentinel.  */
>> -	  while (*p++ != '\0' && p - strtab < strtab_size);
>> +	  while (*p++ != '\0' && p - strtab < strtab_size)
>> +	    ;  /* Silence clang's -Wempty-body warning.  */
> 
> Lately I've been choosing to explicitly put "continue;" when the body
> doesn't contain anything, like:
> 
> 	while (*p++ != '\0' && p - strtab < strtab_size)
> 	  continue;
> 
> I don't know what others think about it, but it would solve this 
> problem
> and also be more verbose that we're just iterating without a body.

I also looks good, but I don't have a preference.  I'll do that if 
others like it too.

Thanks,

Simon

Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line

[Pedro Alves]

On 06/21/2017 09:15 PM, Simon Marchi wrote:
>  	  /* Use strtab_size as a sentinel.  */
> -	  while (*p++ != '\0' && p - strtab < strtab_size);
> +	  while (*p++ != '\0' && p - strtab < strtab_size)
> +	    ;  /* Silence clang's -Wempty-body warning.  */

I'd must put the ; on its own line (there's probably something
in the coding conventions about this already), and without the
comment.  It's quite common to write for/while loop like that,
see e.g.,:

  $ grep "^[ |\t]*;[ |\t]*$" *.c -C 2

Sure the comment makes sense in the context of the patch,
but when reading the code without considering the patch's context,
it just looks like noise to me.

Thanks,
Pedro Alves

Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line

[Simon Marchi]

On 2017-06-21 23:36, Pedro Alves wrote:
> On 06/21/2017 09:15 PM, Simon Marchi wrote:
>>  	  /* Use strtab_size as a sentinel.  */
>> -	  while (*p++ != '\0' && p - strtab < strtab_size);
>> +	  while (*p++ != '\0' && p - strtab < strtab_size)
>> +	    ;  /* Silence clang's -Wempty-body warning.  */
> 
> I'd must put the ; on its own line (there's probably something
> in the coding conventions about this already), and without the
> comment.  It's quite common to write for/while loop like that,
> see e.g.,:
> 
>   $ grep "^[ |\t]*;[ |\t]*$" *.c -C 2
> 
> Sure the comment makes sense in the context of the patch,
> but when reading the code without considering the patch's context,
> it just looks like noise to me.

Ok, since that's already a common practice in our codebase, I added it 
to our wiki (I didn't find anything related to that in the GNU 
standards):

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#preview

I am pushing without the comment.

Thanks,

Simon

Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line

[Simon Marchi]

On 2017-06-25 12:48, Simon Marchi wrote:
> Ok, since that's already a common practice in our codebase, I added it
> to our wiki (I didn't find anything related to that in the GNU
> standards):
> 
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#preview

Sorry, partly wrong URL:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Loop_with_empty_body

[PATCH 2/4] x86-dregs: Print debug registers one per line

[Simon Marchi]

This get around this warning given by 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)
                               ^

... I decided in the end to simply print the debug registers one per
line.  I don't think it particularly helps readability to have them two
per line anyway.

gdb/ChangeLog:

	* nat/x86-dregs.c (x86_show_dr): Print registers one per line.
---
 gdb/nat/x86-dregs.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index 8c8adfa..58b1179 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state,
 			      here.  */
 			   : "??unknown??"))));
   debug_printf (":\n");
-  debug_printf ("\tCONTROL (DR7): %s          STATUS (DR6): %s\n",
-		phex (state->dr_control_mirror, 8),
-		phex (state->dr_status_mirror, 8));
+
+  debug_printf ("\tCONTROL (DR7): 0x%s\n",phex (state->dr_control_mirror, 8));
+  debug_printf ("\tSTATUS (DR6): 0x%s\n", 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\n",
 		    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]);
     }
 }
 
-- 
2.7.4

Re: [PATCH 2/4] x86-dregs: Print debug registers one per line

[Sergio Durigan Junior]

On Wednesday, June 21 2017, Simon Marchi wrote:

> This get around this warning given by 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)
>                                ^
>
> ... I decided in the end to simply print the debug registers one per
> line.  I don't think it particularly helps readability to have them two
> per line anyway.

Agreed, one per line sounds better to me.

>
> gdb/ChangeLog:
>
> 	* nat/x86-dregs.c (x86_show_dr): Print registers one per line.
> ---
>  gdb/nat/x86-dregs.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
> index 8c8adfa..58b1179 100644
> --- a/gdb/nat/x86-dregs.c
> +++ b/gdb/nat/x86-dregs.c
> @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state,
>  			      here.  */
>  			   : "??unknown??"))));
>    debug_printf (":\n");
> -  debug_printf ("\tCONTROL (DR7): %s          STATUS (DR6): %s\n",
> -		phex (state->dr_control_mirror, 8),
> -		phex (state->dr_status_mirror, 8));
> +
> +  debug_printf ("\tCONTROL (DR7): 0x%s\n",phex (state->dr_control_mirror, 8));
                                           ^^^

Space after comma?

> +  debug_printf ("\tSTATUS (DR6): 0x%s\n", 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\n",
>  		    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]);
>      }
>  }
>  
> -- 
> 2.7.4

LGTM.  Thanks,

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

Re: [PATCH 2/4] x86-dregs: Print debug registers one per line

[Simon Marchi]

On 2017-06-21 22:31, Sergio Durigan Junior wrote:
> On Wednesday, June 21 2017, Simon Marchi wrote:
> 
>> This get around this warning given by 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)
>>                                ^
>> 
>> ... I decided in the end to simply print the debug registers one per
>> line.  I don't think it particularly helps readability to have them 
>> two
>> per line anyway.
> 
> Agreed, one per line sounds better to me.
> 
>> 
>> gdb/ChangeLog:
>> 
>> 	* nat/x86-dregs.c (x86_show_dr): Print registers one per line.
>> ---
>>  gdb/nat/x86-dregs.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>> 
>> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
>> index 8c8adfa..58b1179 100644
>> --- a/gdb/nat/x86-dregs.c
>> +++ b/gdb/nat/x86-dregs.c
>> @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state,
>>  			      here.  */
>>  			   : "??unknown??"))));
>>    debug_printf (":\n");
>> -  debug_printf ("\tCONTROL (DR7): %s          STATUS (DR6): %s\n",
>> -		phex (state->dr_control_mirror, 8),
>> -		phex (state->dr_status_mirror, 8));
>> +
>> +  debug_printf ("\tCONTROL (DR7): 0x%s\n",phex 
>> (state->dr_control_mirror, 8));
>                                            ^^^
> 
> Space after comma?

Yep, thanks.

>> +  debug_printf ("\tSTATUS (DR6): 0x%s\n", 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\n",
>>  		    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]);
>>      }
>>  }
>> 
>> --
>> 2.7.4
> 
> LGTM.  Thanks,

Thanks!

Re: [PATCH 2/4] x86-dregs: Print debug registers one per line

[Simon Marchi]

On 2017-06-21 23:06, Simon Marchi wrote:
> On 2017-06-21 22:31, Sergio Durigan Junior wrote:
>> On Wednesday, June 21 2017, Simon Marchi wrote:
>> 
>>> This get around this warning given by 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)
>>>                                ^
>>> 
>>> ... I decided in the end to simply print the debug registers one per
>>> line.  I don't think it particularly helps readability to have them 
>>> two
>>> per line anyway.
>> 
>> Agreed, one per line sounds better to me.
>> 
>>> 
>>> gdb/ChangeLog:
>>> 
>>> 	* nat/x86-dregs.c (x86_show_dr): Print registers one per line.
>>> ---
>>>  gdb/nat/x86-dregs.c | 16 ++++++----------
>>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
>>> index 8c8adfa..58b1179 100644
>>> --- a/gdb/nat/x86-dregs.c
>>> +++ b/gdb/nat/x86-dregs.c
>>> @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state,
>>>  			      here.  */
>>>  			   : "??unknown??"))));
>>>    debug_printf (":\n");
>>> -  debug_printf ("\tCONTROL (DR7): %s          STATUS (DR6): %s\n",
>>> -		phex (state->dr_control_mirror, 8),
>>> -		phex (state->dr_status_mirror, 8));
>>> +
>>> +  debug_printf ("\tCONTROL (DR7): 0x%s\n",phex 
>>> (state->dr_control_mirror, 8));
>>                                            ^^^
>> 
>> Space after comma?
> 
> Yep, thanks.
> 
>>> +  debug_printf ("\tSTATUS (DR6): 0x%s\n", 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\n",
>>>  		    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]);
>>>      }
>>>  }
>>> 
>>> --
>>> 2.7.4
>> 
>> LGTM.  Thanks,
> 
> Thanks!

This is now pushed.