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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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
>