[v2,3/4] gdb/unittests: PR28413, suppress warnings generated by Gnulib

  Gnulib generates a warning if the system version of certain functions
are used (to redirect the developer to use Gnulib version).  It caused a
compiler error when...

-   Compiled with Clang
-   -Werror is specified (by default)
-   C++ standard used by Clang is before C++17 (by default as of 15.0.0)
    when this unit test is activated.

This issue is raised as PR28413.

However, previous proposal to fix this issue (a "fix" to Gnulib):
<https://lists.gnu.org/archive/html/bug-gnulib/2021-10/msg00003.html>
was rejected because it ruins the intent of Gnulib warnings.

So, we need a Binutils/GDB-side solution.

This commit tries to deal with this issue on the GDB side.  We have
"include/diagnostics.h" to disable certain warnings only when necessary.

This commit suppresses the Gnulib warnings by adding
DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS before including "defs.h".

gdb/ChangeLog:

	pr 28413
	* unittests/string_view-selftests.c: Suppress Gnulib-generated
	warnings when "defs.h" is included.
---
 gdb/unittests/string_view-selftests.c | 5 +++++
 1 file changed, 5 insertions(+)
  

Hi Tsukasa,

Thanks for doing this.  I tested this patch on macOS and found one nit.
Please see below.

On Thu, Sep 22 2022 at 08:25:46 AM +0000, Tsukasa OI wrote:

> Gnulib generates a warning if the system version of certain functions
> are used (to redirect the developer to use Gnulib version).  It caused a
> compiler error when...
>
> -   Compiled with Clang
> -   -Werror is specified (by default)
> -   C++ standard used by Clang is before C++17 (by default as of 15.0.0)
>     when this unit test is activated.
>
> This issue is raised as PR28413.
>
> However, previous proposal to fix this issue (a "fix" to Gnulib):
> <https://lists.gnu.org/archive/html/bug-gnulib/2021-10/msg00003.html>
> was rejected because it ruins the intent of Gnulib warnings.
>
> So, we need a Binutils/GDB-side solution.
>
> This commit tries to deal with this issue on the GDB side.  We have
> "include/diagnostics.h" to disable certain warnings only when necessary.
>
> This commit suppresses the Gnulib warnings by adding
> DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS before including "defs.h".
>
> gdb/ChangeLog:
>
> 	pr 28413
> 	* unittests/string_view-selftests.c: Suppress Gnulib-generated
> 	warnings when "defs.h" is included.
> ---
>  gdb/unittests/string_view-selftests.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/gdb/unittests/string_view-selftests.c b/gdb/unittests/string_view-selftests.c
> index 2d7261d18d3..c1f7799d94c 100644
> --- a/gdb/unittests/string_view-selftests.c
> +++ b/gdb/unittests/string_view-selftests.c
> @@ -23,7 +23,12 @@
>  
>  #define GNULIB_NAMESPACE gnulib
>  
> +#include "diagnostics.h"
> +
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
>  #include "defs.h"
> +DIAGNOSTIC_POP
>  #include "gdbsupport/selftest.h"
>  #include "gdbsupport/gdb_string_view.h"

With your patch applied on macOS 15.10.7, it still says,

==========================================================================================
  CXX    unittests/string_view-selftests.o
In file included from unittests/string_view-selftests.c:39:
/Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/fstream:579:17: error: The symbol
      ::fdopen refers to the system function. Use gnulib::fdopen instead.
      [-Werror,-Wuser-defined-warnings]
      __file_ = fdopen(__fd, __mdstr);
                ^
./../gnulib/import/stdio.h:826:1: note: from 'diagnose_if' attribute on 'fdopen':
_GL_CXXALIASWARN (fdopen);
^~~~~~~~~~~~~~~~~~~~~~~~~
./../gnulib/import/stdio.h:461:4: note: expanded from macro '_GL_CXXALIASWARN'
   _GL_CXXALIASWARN_1 (func, GNULIB_NAMESPACE)
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./../gnulib/import/stdio.h:463:4: note: expanded from macro '_GL_CXXALIASWARN_1'
   _GL_CXXALIASWARN_2 (func, namespace)
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./../gnulib/import/stdio.h:468:5: note: expanded from macro '_GL_CXXALIASWARN_2'
    _GL_WARN_ON_USE (func, \
    ^~~~~~~~~~~~~~~~~~~~~~~~
./../gnulib/import/stdio.h:632:19: note: expanded from macro '_GL_WARN_ON_USE'
  __attribute__ ((__diagnose_if__ (1, message, "warning")))
                  ^                ~
1 error generated.
make[2]: *** [unittests/string_view-selftests.o] Error 1
==========================================================================================

I think this can be solved by just adding
DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS before including <fstream>
according to your method.

WDYT?
==========================================================================================
diff --git a/gdb/unittests/string_view-selftests.c b/gdb/unittests/string_view-selftests.c
index 2d7261d18d3..b779b9b765a 100644
--- a/gdb/unittests/string_view-selftests.c
+++ b/gdb/unittests/string_view-selftests.c
@@ -23,7 +23,12 @@

 #define GNULIB_NAMESPACE gnulib

+#include "diagnostics.h"
+
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
 #include "defs.h"
+DIAGNOSTIC_POP
 #include "gdbsupport/selftest.h"
 #include "gdbsupport/gdb_string_view.h"

@@ -31,7 +36,10 @@
    included test files are wrapped in a namespace.  */
 #include <string>
 #include <sstream>
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
 #include <fstream>
+DIAGNOSTIC_POP
 #include <iostream>

 /* libstdc++'s testsuite uses VERIFY.  */
==========================================================================================

Other than that, looks good to me.

Reviewed-by: Enze Li <enze.li@hotmail.com>

Thanks,
Enze
  

On 2022/10/18 22:44, Enze Li wrote:
> Hi Tsukasa,
> 
> Thanks for doing this.  I tested this patch on macOS and found one nit.
> Please see below.
> 
> On Thu, Sep 22 2022 at 08:25:46 AM +0000, Tsukasa OI wrote:
> 
>> Gnulib generates a warning if the system version of certain functions
>> are used (to redirect the developer to use Gnulib version).  It caused a
>> compiler error when...
>>
>> -   Compiled with Clang
>> -   -Werror is specified (by default)
>> -   C++ standard used by Clang is before C++17 (by default as of 15.0.0)
>>     when this unit test is activated.
>>
>> This issue is raised as PR28413.
>>
>> However, previous proposal to fix this issue (a "fix" to Gnulib):
>> <https://lists.gnu.org/archive/html/bug-gnulib/2021-10/msg00003.html>
>> was rejected because it ruins the intent of Gnulib warnings.
>>
>> So, we need a Binutils/GDB-side solution.
>>
>> This commit tries to deal with this issue on the GDB side.  We have
>> "include/diagnostics.h" to disable certain warnings only when necessary.
>>
>> This commit suppresses the Gnulib warnings by adding
>> DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS before including "defs.h".
>>
>> gdb/ChangeLog:
>>
>> 	pr 28413
>> 	* unittests/string_view-selftests.c: Suppress Gnulib-generated
>> 	warnings when "defs.h" is included.
>> ---
>>  gdb/unittests/string_view-selftests.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/gdb/unittests/string_view-selftests.c b/gdb/unittests/string_view-selftests.c
>> index 2d7261d18d3..c1f7799d94c 100644
>> --- a/gdb/unittests/string_view-selftests.c
>> +++ b/gdb/unittests/string_view-selftests.c
>> @@ -23,7 +23,12 @@
>>  
>>  #define GNULIB_NAMESPACE gnulib
>>  
>> +#include "diagnostics.h"
>> +
>> +DIAGNOSTIC_PUSH
>> +DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
>>  #include "defs.h"
>> +DIAGNOSTIC_POP
>>  #include "gdbsupport/selftest.h"
>>  #include "gdbsupport/gdb_string_view.h"
> 
> With your patch applied on macOS 15.10.7, it still says,
> 
> ==========================================================================================
>   CXX    unittests/string_view-selftests.o
> In file included from unittests/string_view-selftests.c:39:
> /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/fstream:579:17: error: The symbol
>       ::fdopen refers to the system function. Use gnulib::fdopen instead.
>       [-Werror,-Wuser-defined-warnings]
>       __file_ = fdopen(__fd, __mdstr);
>                 ^
> ./../gnulib/import/stdio.h:826:1: note: from 'diagnose_if' attribute on 'fdopen':
> _GL_CXXALIASWARN (fdopen);
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> ./../gnulib/import/stdio.h:461:4: note: expanded from macro '_GL_CXXALIASWARN'
>    _GL_CXXALIASWARN_1 (func, GNULIB_NAMESPACE)
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./../gnulib/import/stdio.h:463:4: note: expanded from macro '_GL_CXXALIASWARN_1'
>    _GL_CXXALIASWARN_2 (func, namespace)
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./../gnulib/import/stdio.h:468:5: note: expanded from macro '_GL_CXXALIASWARN_2'
>     _GL_WARN_ON_USE (func, \
>     ^~~~~~~~~~~~~~~~~~~~~~~~
> ./../gnulib/import/stdio.h:632:19: note: expanded from macro '_GL_WARN_ON_USE'
>   __attribute__ ((__diagnose_if__ (1, message, "warning")))
>                   ^                ~
> 1 error generated.
> make[2]: *** [unittests/string_view-selftests.o] Error 1
> ==========================================================================================
> 
> I think this can be solved by just adding
> DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS before including <fstream>
> according to your method.
> 
> WDYT?

Thanks for testing this on real Mac (since only Mac machine I own is
very old so that support is dropped).  Thinking of possible differences
in the standard C++ library (as you tested), it'd be better to surround
entire #include block with DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
(rather than just "defs.h" and <fstream>).

I will submit PATCH v3 consisting only with this patch (because 2 of 4
patches [1-2/4] are merged already and Simon Marchi made a change better
than my PATCH v2 4/4).

Regards,
Tsukasa

> ==========================================================================================
> diff --git a/gdb/unittests/string_view-selftests.c b/gdb/unittests/string_view-selftests.c
> index 2d7261d18d3..b779b9b765a 100644
> --- a/gdb/unittests/string_view-selftests.c
> +++ b/gdb/unittests/string_view-selftests.c
> @@ -23,7 +23,12 @@
> 
>  #define GNULIB_NAMESPACE gnulib
> 
> +#include "diagnostics.h"
> +
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
>  #include "defs.h"
> +DIAGNOSTIC_POP
>  #include "gdbsupport/selftest.h"
>  #include "gdbsupport/gdb_string_view.h"
> 
> @@ -31,7 +36,10 @@
>     included test files are wrapped in a namespace.  */
>  #include <string>
>  #include <sstream>
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_IGNORE_USER_DEFINED_WARNINGS
>  #include <fstream>
> +DIAGNOSTIC_POP
>  #include <iostream>
> 
>  /* libstdc++'s testsuite uses VERIFY.  */
> ==========================================================================================
> 
> Other than that, looks good to me.
> 
> Reviewed-by: Enze Li <enze.li@hotmail.com>
> 
> Thanks,
> Enze
>