[PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Mark Wielaard]

Just like on s390x with g++ 11.2.1, ppc64le with g++ 11.3.1 produces a
spurious warning for stringop-overread in debuginfod_is_enabled
for url_view. Also suppress it on powerpc64.

 gdb/ChangeLog:

    * debuginfod-support.c (debuginfod_is_enabled): Use
      DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD on powerpc64.
---

Found with the new fedora-ppc64le builder:
https://builder.sourceware.org/buildbot/#/builders/gdb-fedora-ppc64le

Alternatively we could just suppress this warning for any architecture.

 gdb/debuginfod-support.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index f2a31ea1952..6dc08fc29b6 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -193,15 +193,15 @@ debuginfod_is_enabled ()
 	  if (off == gdb::string_view::npos)
 	    break;
 	  url_view = url_view.substr (off);
-#if defined (__s390x__)
-	  /* g++ 11.2.1 on s390x seems convinced url_view might be of
-	     SIZE_MAX length.  And so complains because the length of
-	     an array can only be PTRDIFF_MAX.  */
+#if defined (__s390x__) || defined (__powerpc64__)
+	  /* g++ 11.2.1 on s390x and g++ 11.3.1 on ppc64le seem convinced
+	     url_view might be of SIZE_MAX length.  And so complains
+	     because the length of an array can only be PTRDIFF_MAX.  */
 	  DIAGNOSTIC_PUSH
 	  DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD
 #endif
 	  off = url_view.find_first_of (' ');
-#if defined (__s390x__)
+#if defined (__s390x__) || defined (__powerpc64__)
 	  DIAGNOSTIC_POP
 #endif
 	  gdb_printf
-- 
2.30.2

Re: [PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Simon Marchi]

On 2022-05-11 18:52, Mark Wielaard wrote:
> Just like on s390x with g++ 11.2.1, ppc64le with g++ 11.3.1 produces a
> spurious warning for stringop-overread in debuginfod_is_enabled
> for url_view. Also suppress it on powerpc64.
> 
>  gdb/ChangeLog:
> 
>     * debuginfod-support.c (debuginfod_is_enabled): Use
>       DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD on powerpc64.

LGTM.

> ---
> 
> Found with the new fedora-ppc64le builder:
> https://builder.sourceware.org/buildbot/#/builders/gdb-fedora-ppc64le
> 
> Alternatively we could just suppress this warning for any architecture.

Let's make a deal, if you find a 3rd architecture where this is the case
we do that, alright? ;)

Simon

Re: [PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

Hi Simon,

On Wed, 2022-05-11 at 20:49 -0400, Simon Marchi wrote:
> On 2022-05-11 18:52, Mark Wielaard wrote:
> > Alternatively we could just suppress this warning for any
> > architecture.
> 
> Let's make a deal, if you find a 3rd architecture where this is the
> case we do that, alright? ;)

Got another on hppa:
https://sourceware.org/bugzilla/show_bug.cgi?id=29198

So, ok to push the attached?

Thanks,

Mark

[-- Attachment #2: 0001-gdb-Always-suppress-stringop-overread-warning-in-deb.patch --]
[-- Type: text/x-patch, Size: 1767 bytes --]

From 0a1bd604d3037272d89c7c3e9ddc02e513e1027a Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Tue, 31 May 2022 11:00:06 +0200
Subject: [PATCH] gdb: Always suppress stringop-overread warning in
 debuginfod-support.c

Just like on s390x with g++ 11.2.1 and ppc64le with g++ 11.3.1 g++ 11
on hppa produces a spurious warning for stringop-overread in
debuginfod_is_enabled for url_view. Just always suppress it on all
arches.

https://sourceware.org/bugzilla/show_bug.cgi?id=29198

gdb/ChangeLog:

	* debuginfod-support.c (debuginfod_is_enabled): Always use
	DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD.
---
 gdb/debuginfod-support.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 6dc08fc29b6..bd355b67352 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -193,17 +193,14 @@ debuginfod_is_enabled ()
 	  if (off == gdb::string_view::npos)
 	    break;
 	  url_view = url_view.substr (off);
-#if defined (__s390x__) || defined (__powerpc64__)
-	  /* g++ 11.2.1 on s390x and g++ 11.3.1 on ppc64le seem convinced
-	     url_view might be of SIZE_MAX length.  And so complains
-	     because the length of an array can only be PTRDIFF_MAX.  */
+	  /* g++ 11.2.1 on s390x, g++q 11.3.1 on ppc64le and g++ 11 on
+	     hppa seem convinced url_view might be of SIZE_MAX length.
+	     And so complains because the length of an array can only
+	     be PTRDIFF_MAX.  */
 	  DIAGNOSTIC_PUSH
 	  DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD
-#endif
 	  off = url_view.find_first_of (' ');
-#if defined (__s390x__) || defined (__powerpc64__)
 	  DIAGNOSTIC_POP
-#endif
 	  gdb_printf
 	    (_("  <%ps>\n"),
 	     styled_string (file_name_style.style (),
-- 
2.18.4

Re: [PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Pedro Alves]

On 2022-05-31 10:05, Mark Wielaard wrote:
> +	  /* g++ 11.2.1 on s390x, g++q 11.3.1 on ppc64le and g++ 11 on
> +	     hppa seem convinced url_view might be of SIZE_MAX length.
> +	     And so complains because the length of an array can only
> +	     be PTRDIFF_MAX.  */

I wonder whether sticking in an assert so that the compiler knows the
size of array, would make the warning go away:

diff --git c/gdb/debuginfod-support.c w/gdb/debuginfod-support.c
index 6dc08fc29b6..d6fab39eac8 100644
--- c/gdb/debuginfod-support.c
+++ w/gdb/debuginfod-support.c
@@ -187,6 +187,8 @@ debuginfod_is_enabled ()
                    "from the following URLs:\n"));
 
       gdb::string_view url_view (urls);
+      gdb_assert (url_view.size () < PTRDIFF_MAX);
+
       while (true)
        {
          size_t off = url_view.find_first_not_of (' ');

Re: [PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Mark Wielaard]

Hi Pedro,

On Tue, 2022-05-31 at 10:46 +0100, Pedro Alves wrote:
> On 2022-05-31 10:05, Mark Wielaard wrote:
> > +	  /* g++ 11.2.1 on s390x, g++q 11.3.1 on ppc64le and g++ 11 on
> > +	     hppa seem convinced url_view might be of SIZE_MAX length.
> > +	     And so complains because the length of an array can only
> > +	     be PTRDIFF_MAX.  */
> 
> I wonder whether sticking in an assert so that the compiler knows the
> size of array, would make the warning go away:

If only we had the try buildbots running already, then we could more
easily try (because one of them, fedora-s390x, does show this issue).
I'll try (pun intended) to get that up by next week.

> diff --git c/gdb/debuginfod-support.c w/gdb/debuginfod-support.c
> index 6dc08fc29b6..d6fab39eac8 100644
> --- c/gdb/debuginfod-support.c
> +++ w/gdb/debuginfod-support.c
> @@ -187,6 +187,8 @@ debuginfod_is_enabled ()
>                     "from the following URLs:\n"));
>  
>        gdb::string_view url_view (urls);
> +      gdb_assert (url_view.size () < PTRDIFF_MAX);
> +
>        while (true)
>         {
>           size_t off = url_view.find_first_not_of (' ');

I tried by hand, but that does result in the same warning (with the
DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD removed):

  CXX    debuginfod-support.o
In file included from /usr/include/c++/11/string:40,
                 from ../../binutils-gdb/gdb/../gdbsupport/ptid.h:36,
                 from ../../binutils-gdb/gdb/../gdbsupport/common-defs.h:198,
                 from ../../binutils-gdb/gdb/defs.h:28,
                 from ../../binutils-gdb/gdb/debuginfod-support.c:19:
In static member function ‘static constexpr const char_type* std::char_traits<char>::find(const char_type*, std::size_t, const char_type&)’,
    inlined from ‘constexpr std::basic_string_view<_CharT, _Traits>::size_type std::basic_string_view<_CharT, _Traits>::find(_CharT, std::basic_string_view<_CharT, _Traits>::size_type) const [with _CharT = char; _Traits = std::char_traits<char>]’ at /usr/include/c++/11/bits/string_view.tcc:87:41,
    inlined from ‘constexpr std::basic_string_view<_CharT, _Traits>::size_type std::basic_string_view<_CharT, _Traits>::find_first_of(_CharT, std::basic_string_view<_CharT, _Traits>::size_type) const [with _CharT = char; _Traits = std::char_traits<char>]’ at /usr/include/c++/11/string_view:431:26,
    inlined from ‘bool debuginfod_is_enabled()’ at ../../binutils-gdb/gdb/debuginfod-support.c:197:33:
/usr/include/c++/11/bits/char_traits.h:413:62: error: ‘void* __builtin_memchr(const void*, int, long unsigned int)’ specified bound 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread]
  413 |         return static_cast<const char_type*>(__builtin_memchr(__s, __a, __n));
      |                                              ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[1]: *** [Makefile:1895: debuginfod-support.o] Error 1

Re: [PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Mark Wielaard]

Hi,

On Tue, 2022-05-31 at 12:51 +0200, Mark Wielaard wrote:
> On Tue, 2022-05-31 at 10:46 +0100, Pedro Alves wrote:
> > I wonder whether sticking in an assert so that the compiler
> > knows the size of array, would make the warning go away:
> 
> > diff --git c/gdb/debuginfod-support.c w/gdb/debuginfod-support.c
> > index 6dc08fc29b6..d6fab39eac8 100644
> > --- c/gdb/debuginfod-support.c
> > +++ w/gdb/debuginfod-support.c
> > @@ -187,6 +187,8 @@ debuginfod_is_enabled ()
> >                     "from the following URLs:\n"));
> >  
> >        gdb::string_view url_view (urls);
> > +      gdb_assert (url_view.size () < PTRDIFF_MAX);
> > +
> >        while (true)
> >         {
> >           size_t off = url_view.find_first_not_of (' ');
> 
> I tried by hand, but that does result in the same warning (with the
> DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD removed):

So, since that didn't work, OK to push the original patch that simply
suppresses the diagnostic always?

Thanks,

Mark

Re: [PATCH] gdb: Workaround stringop-overread warning in debuginfod-support.c on powerpc64

[Pedro Alves]

On 2022-06-06 12:27, Mark Wielaard wrote:
> Hi,
> 
> On Tue, 2022-05-31 at 12:51 +0200, Mark Wielaard wrote:
>> On Tue, 2022-05-31 at 10:46 +0100, Pedro Alves wrote:
>>> I wonder whether sticking in an assert so that the compiler
>>> knows the size of array, would make the warning go away:
>>
>>> diff --git c/gdb/debuginfod-support.c w/gdb/debuginfod-support.c
>>> index 6dc08fc29b6..d6fab39eac8 100644
>>> --- c/gdb/debuginfod-support.c
>>> +++ w/gdb/debuginfod-support.c
>>> @@ -187,6 +187,8 @@ debuginfod_is_enabled ()
>>>                     "from the following URLs:\n"));
>>>  
>>>        gdb::string_view url_view (urls);
>>> +      gdb_assert (url_view.size () < PTRDIFF_MAX);
>>> +
>>>        while (true)
>>>         {
>>>           size_t off = url_view.find_first_not_of (' ');
>>
>> I tried by hand, but that does result in the same warning (with the
>> DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD removed):
> 
> So, since that didn't work, OK to push the original patch that simply
> suppresses the diagnostic always?

The code currently looks like:

      gdb::string_view url_view (urls);
      while (true)
	{
	  size_t off = url_view.find_first_not_of (' ');
	  if (off == gdb::string_view::npos)
	    break;
	  url_view = url_view.substr (off);
#if defined (__s390x__) || defined (__powerpc64__)
	  /* g++ 11.2.1 on s390x and g++ 11.3.1 on ppc64le seem convinced
	     url_view might be of SIZE_MAX length.  And so complains
	     because the length of an array can only be PTRDIFF_MAX.  */
	  DIAGNOSTIC_PUSH
	  DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD
#endif
	  off = url_view.find_first_of (' ');
#if defined (__s390x__) || defined (__powerpc64__)
	  DIAGNOSTIC_POP
#endif


I suppose that the fact that the warning appears in the find_first_of
call, and not the find_first_not_of call above was already an indication
that the assert at that spot wouldn't work, otherwise, I'd think we'd
be seeing a similar warning in the find_first_not_of call.  I suspect that
for the gdb_assert to work, it would need to be put at the same spot where
the DIAGNOSTIC_PUSH is, after the url_view.substr.  The theory being that
substr returns a new string_view and it's that one that would need the
assert.  Like:

       gdb::string_view url_view (urls);
       while (true)
	 {
	   size_t off = url_view.find_first_not_of (' ');
	   if (off == gdb::string_view::npos)
	     break;
	   url_view = url_view.substr (off);
+          gdb_assert (url_view.size () < PTRDIFF_MAX);
 	   off = url_view.find_first_of (' ');


OTOH, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97185 suggests that
the root issue is with not eliminating builtin calls when sizes are 0, so maybe
the right thing to do would be to assert that the string_view isn't empty, like:

diff --git c/gdb/debuginfod-support.c w/gdb/debuginfod-support.c
index 6dc08fc29b6..9a4301f018c 100644
--- c/gdb/debuginfod-support.c
+++ w/gdb/debuginfod-support.c
@@ -193,17 +193,8 @@ debuginfod_is_enabled ()
          if (off == gdb::string_view::npos)
            break;
          url_view = url_view.substr (off);
-#if defined (__s390x__) || defined (__powerpc64__)
-         /* g++ 11.2.1 on s390x and g++ 11.3.1 on ppc64le seem convinced
-            url_view might be of SIZE_MAX length.  And so complains
-            because the length of an array can only be PTRDIFF_MAX.  */
-         DIAGNOSTIC_PUSH
-         DIAGNOSTIC_IGNORE_STRINGOP_OVERREAD
-#endif
+         gdb_assert (!url_view.empty ());
          off = url_view.find_first_of (' ');
-#if defined (__s390x__) || defined (__powerpc64__)
-         DIAGNOSTIC_POP
-#endif
 

Anyhow, your patch is OK with me.