Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Alan Modra]

On Fri, Mar 19, 2021 at 01:44:38PM +0100, Martin Liška wrote:
> @@ -73,6 +64,13 @@ extern "C" {
>  #define LITMEMCPY(DEST,STR2) memcpy ((DEST), (STR2), sizeof (STR2) - 1)
>  #define LITSTRCPY(DEST,STR2) memcpy ((DEST), (STR2), sizeof (STR2))
>  
> +/* Return 1 if STR string starts with PREFIX.  */
> +
> +static inline int
> +startswith (const char *str, const char *prefix)
> +{
> +  return __builtin_strncmp (str, prefix, __builtin_strlen (prefix)) == 0;
> +}
>  
>  #define BFD_SUPPORTS_PLUGINS @supports_plugins@
>  

In binutils, we haven't yet made a policy that the project requires
gcc, so builtins can't be used without a fallback.  I tried building
with the following but it runs into a compilation failure in gdb.  I
expect your patch would do the same..

In file included from /home/alan/src/binutils-gdb/gdb/defs.h:37,
                 from /home/alan/src/binutils-gdb/gdb/gdb.c:19:
../bfd/bfd.h:85:1: error: ambiguating new declaration of ‘int startswith(const char*, const char*)’
 startswith (const char *str, const char *prefix)
 ^~~~~~~~~~
In file included from /home/alan/src/binutils-gdb/gdb/../gdbsupport/common-defs.h:125,
                 from /home/alan/src/binutils-gdb/gdb/defs.h:28,
                 from /home/alan/src/binutils-gdb/gdb/gdb.c:19:
/home/alan/src/binutils-gdb/gdb/../gdbsupport/common-utils.h:122:1: note: old declaration ‘bool startswith(const char*, const char*)’
 startswith (const char *string, const char *pattern)
 ^~~~~~~~~~

Forcing gdb to remove their startswith is a bit rude.  Should I use
#ifndef __cplusplus or #ifndef gdb_assert around the bfd version?


bfd/
	* bfd-in.h (startswith): New inline.
	(CONST_STRNEQ): Use startswith.
	* bfd-in2.h: Regenerate.
libctf/
	* ctf-impl.h: Include string.h.

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index c9a7673147..4d99c21d3e 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -65,7 +65,6 @@ extern "C" {
    definition of strncmp is provided here.
 
    Note - these macros do NOT work if STR2 is not a constant string.  */
-#define CONST_STRNEQ(STR1,STR2) (strncmp ((STR1), (STR2), sizeof (STR2) - 1) == 0)
   /* strcpy() can have a similar problem, but since we know we are
      copying a constant string, we can use memcpy which will be faster
      since there is no need to check for a NUL byte inside STR.  We
@@ -73,6 +72,15 @@ extern "C" {
 #define LITMEMCPY(DEST,STR2) memcpy ((DEST), (STR2), sizeof (STR2) - 1)
 #define LITSTRCPY(DEST,STR2) memcpy ((DEST), (STR2), sizeof (STR2))
 
+/* Return 1 if STR string starts with PREFIX.  */
+
+static inline int
+startswith (const char *str, const char *prefix)
+{
+  return strncmp (str, prefix, strlen (prefix)) == 0;
+}
+#define CONST_STRNEQ(STR1,STR2) startswith (STR1, STR2)
+
 
 #define BFD_SUPPORTS_PLUGINS @supports_plugins@
 
diff --git a/libctf/ctf-impl.h b/libctf/ctf-impl.h
index ad4af32e7e..342d2ff23e 100644
--- a/libctf/ctf-impl.h
+++ b/libctf/ctf-impl.h
@@ -32,6 +32,7 @@
 #include <stddef.h>
 #include <stdio.h>
 #include <stdint.h>
+#include <string.h>
 #include <limits.h>
 #include <ctype.h>
 #include <elf.h>


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Tom Tromey]

>>>>> "Alan" == Alan Modra via Binutils <binutils@sourceware.org> writes:

Alan> Forcing gdb to remove their startswith is a bit rude.

FWIW I think it would be fine, assuming it compiles, considering that
the functions have identical intended semantics.

Tom

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Alan Modra]

On Sat, Mar 20, 2021 at 12:58:10PM -0600, Tom Tromey wrote:
> >>>>> "Alan" == Alan Modra via Binutils <binutils@sourceware.org> writes:
> 
> Alan> Forcing gdb to remove their startswith is a bit rude.
> 
> FWIW I think it would be fine, assuming it compiles, considering that
> the functions have identical intended semantics.

Yes, the following compiles.  Fortunately all gdb files that include
gdbsupport/common-utils.h also include bfd.h by one means or another.

Committed.

bfd/
	* bfd-in.h (startswith): New inline.
	(CONST_STRNEQ): Use startswith.
	* bfd-in2.h: Regenerate.
gdbsupport/
	* common-utils.h (startswith): Delete version now supplied by bfd.h.
libctf/
	* ctf-impl.h: Include string.h.

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index c9a7673147..453ac48e26 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -65,7 +65,6 @@ extern "C" {
    definition of strncmp is provided here.
 
    Note - these macros do NOT work if STR2 is not a constant string.  */
-#define CONST_STRNEQ(STR1,STR2) (strncmp ((STR1), (STR2), sizeof (STR2) - 1) == 0)
   /* strcpy() can have a similar problem, but since we know we are
      copying a constant string, we can use memcpy which will be faster
      since there is no need to check for a NUL byte inside STR.  We
@@ -564,3 +563,12 @@ struct ecoff_debug_swap;
 struct ecoff_extr;
 struct bfd_link_info;
 struct bfd_link_hash_entry;
+
+/* Return TRUE if the start of STR matches PREFIX, FALSE otherwise.  */
+
+static inline bfd_boolean
+startswith (const char *str, const char *prefix)
+{
+  return strncmp (str, prefix, strlen (prefix)) == 0;
+}
+#define CONST_STRNEQ(STR1,STR2) startswith (STR1, STR2)
diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
index 28c08ee976..1de747f186 100644
--- a/gdbsupport/common-utils.h
+++ b/gdbsupport/common-utils.h
@@ -116,16 +116,8 @@ std::string extract_string_maybe_quoted (const char **arg);
 
 extern const char *safe_strerror (int);
 
-/* Return true if the start of STRING matches PATTERN, false otherwise.  */
-
-static inline bool
-startswith (const char *string, const char *pattern)
-{
-  return strncmp (string, pattern, strlen (pattern)) == 0;
-}
-
-/* Version of startswith that takes string_view arguments.  See comment
-   above.  */
+/* Version of startswith that takes string_view arguments.  Return
+   true if the start of STRING matches PATTERN, false otherwise.  */
 
 static inline bool
 startswith (gdb::string_view string, gdb::string_view pattern)
diff --git a/libctf/ctf-impl.h b/libctf/ctf-impl.h
index ad4af32e7e..342d2ff23e 100644
--- a/libctf/ctf-impl.h
+++ b/libctf/ctf-impl.h
@@ -32,6 +32,7 @@
 #include <stddef.h>
 #include <stdio.h>
 #include <stdint.h>
+#include <string.h>
 #include <limits.h>
 #include <ctype.h>
 #include <elf.h>

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Tom Tromey]

Alan> Yes, the following compiles.  Fortunately all gdb files that include
Alan> gdbsupport/common-utils.h also include bfd.h by one means or another.

Sorry, I misunderstood.  I thought the proposal was to put the
function in ansidecl.h.

gdbserver can't generally include bfd.  Maybe it works sometimes, but I
imagine if you do a gdbserver-only build, bfd.h won't even be created.
(At least, it shouldn't be, because gdbserver doesn't require bfd.)

So I think some other approach is needed.  Either ansidecl.h, or a new
header; or rename either the BFD or GDB function and go from there.

I'm happy to implement it if you like.

thanks,
Tom

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Mike Frysinger]

On 21 Mar 2021 23:42, Alan Modra via Binutils wrote:
> On Sat, Mar 20, 2021 at 12:58:10PM -0600, Tom Tromey wrote:
> > >>>>> "Alan" == Alan Modra via Binutils <binutils@sourceware.org> writes:
> > 
> > Alan> Forcing gdb to remove their startswith is a bit rude.
> > 
> > FWIW I think it would be fine, assuming it compiles, considering that
> > the functions have identical intended semantics.
> 
> Yes, the following compiles.  Fortunately all gdb files that include
> gdbsupport/common-utils.h also include bfd.h by one means or another.
> 
> Committed.
> 
> bfd/
> 	* bfd-in.h (startswith): New inline.
> 	(CONST_STRNEQ): Use startswith.
> 	* bfd-in2.h: Regenerate.
> gdbsupport/
> 	* common-utils.h (startswith): Delete version now supplied by bfd.h.
> libctf/
> 	* ctf-impl.h: Include string.h.

putting this in the exported header is problematic.  as you see with the libctf
code, anyone including bfd.h now has to make sure string.h is included first
even if they don't use the header themselves.  this is generally considered bad
form for headers.

along those lines, a bunch of sim ports are broken now as they include bfd.h
but not all happen to include string.h first.
-mike

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Alan Modra]

On Sun, Mar 21, 2021 at 08:13:06PM -0600, Tom Tromey wrote:
> Alan> Yes, the following compiles.  Fortunately all gdb files that include
> Alan> gdbsupport/common-utils.h also include bfd.h by one means or another.
> 
> Sorry, I misunderstood.  I thought the proposal was to put the
> function in ansidecl.h.
> 
> gdbserver can't generally include bfd.  Maybe it works sometimes, but I
> imagine if you do a gdbserver-only build, bfd.h won't even be created.
> (At least, it shouldn't be, because gdbserver doesn't require bfd.)
> 
> So I think some other approach is needed.  Either ansidecl.h, or a new
> header; or rename either the BFD or GDB function and go from there.

A new header would be best, I think.

> I'm happy to implement it if you like.

Yes, that way you'll get it done properly.  :)  Sorry for the breakage.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

On 3/22/21 9:06 AM, Alan Modra via Gdb-patches wrote:
> On Sun, Mar 21, 2021 at 08:13:06PM -0600, Tom Tromey wrote:
>> Alan> Yes, the following compiles.  Fortunately all gdb files that include
>> Alan> gdbsupport/common-utils.h also include bfd.h by one means or another.
>>
>> Sorry, I misunderstood.  I thought the proposal was to put the
>> function in ansidecl.h.
>>
>> gdbserver can't generally include bfd.  Maybe it works sometimes, but I
>> imagine if you do a gdbserver-only build, bfd.h won't even be created.
>> (At least, it shouldn't be, because gdbserver doesn't require bfd.)
>>
>> So I think some other approach is needed.  Either ansidecl.h, or a new
>> header; or rename either the BFD or GDB function and go from there.
> 
> A new header would be best, I think.
> 
>> I'm happy to implement it if you like.
> 
> Yes, that way you'll get it done properly.  :)  Sorry for the breakage.
> 

Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are 
complaining about "-Werror=implicit-function-declaration" regarding 
strncmp and strlen.

Is this the breakage you're talking about? Just so I know what to expect 
when it gets fixed.

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Martin Liška]

On 3/22/21 1:06 PM, Alan Modra wrote:
> On Sun, Mar 21, 2021 at 08:13:06PM -0600, Tom Tromey wrote:
>> Alan> Yes, the following compiles.  Fortunately all gdb files that include
>> Alan> gdbsupport/common-utils.h also include bfd.h by one means or another.
>>
>> Sorry, I misunderstood.  I thought the proposal was to put the
>> function in ansidecl.h.
>>
>> gdbserver can't generally include bfd.  Maybe it works sometimes, but I
>> imagine if you do a gdbserver-only build, bfd.h won't even be created.
>> (At least, it shouldn't be, because gdbserver doesn't require bfd.)
>>
>> So I think some other approach is needed.  Either ansidecl.h, or a new
>> header; or rename either the BFD or GDB function and go from there.
> 
> A new header would be best, I think.
> 
>> I'm happy to implement it if you like.
> 
> Yes, that way you'll get it done properly.  :)  Sorry for the breakage.
> 

Hello.

I feel also responsible for the current compilation problems, I pulled the trigger.

Anyway for the sim failures. What about directly including <string.h> in bfd-in.h?

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index fda9fe0198e..d35e2ece516 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -38,6 +38,7 @@ extern "C" {
  #include "diagnostics.h"
  #include <stdarg.h>
  #include <sys/stat.h>
+#include <string.h>
  
  #if defined (__STDC__) || defined (ALMOST_STDC) || defined (HAVE_STRINGIZE)
  #ifndef SABER

Or do I miss something?
Thanks,
Martin

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Alan Modra]

On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
> complaining about "-Werror=implicit-function-declaration" regarding strncmp
> and strlen.
> 
> Is this the breakage you're talking about? Just so I know what to expect
> when it gets fixed.

Yes.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Alan Modra]

On Mon, Mar 22, 2021 at 05:42:50PM +0100, Martin Liška wrote:
> Anyway for the sim failures. What about directly including <string.h> in bfd-in.h?

That would be OK if we didn't care about really old systems.  See for
example the way bfd/sysdep.h includes string.h.

But all of this horrible old code that likely hasn't been tested is
eons should simply disappear.  Instead binutils should be using the
gnulib import already available in the binutils-gdb repository to
support old systems.  I've had that project on my todo list for quite
a while.  We'd be able to include stdint.h and stdbool.h for example,
throwing away bfd_stdint.h and replacing bfd_boolean with bool.

Meanwhile, this is a tidied version of the patch I sent you last
night, Tom.  If you already have one of your own then please ignore
this.

---
Subject: str-util.h

Defining startswith in bfd.h breaks sim targets that don't happen to
include string.h before bfd.h, and since bfd.h is exported, might
break other projects that use bfd.h.

include/
	* str-util.h: New file.
bfd/
	* sysdep.h: Include str-util.h.
	* bfd-in.h (startswith): Delete.
	* bfd-in2.h: Regenerate.
binutils/
	* sysdep.h: Include str-util.h.
gas/
	* as.h: Include str-util.h.
gdbsupport/
	* common-utils.h: Include str-util.h.
ld/
	* sysdep.h: Include str-util.h.
opcodes/
	* sysdep.h: Include str-util.h.

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index fda9fe0198..92391bb6a3 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -554,11 +554,3 @@ struct ecoff_debug_swap;
 struct ecoff_extr;
 struct bfd_link_info;
 struct bfd_link_hash_entry;
-
-/* Return TRUE if the start of STR matches PREFIX, FALSE otherwise.  */
-
-static inline bfd_boolean
-startswith (const char *str, const char *prefix)
-{
-  return strncmp (str, prefix, strlen (prefix)) == 0;
-}
diff --git a/bfd/sysdep.h b/bfd/sysdep.h
index 338d731112..b04847ffc3 100644
--- a/bfd/sysdep.h
+++ b/bfd/sysdep.h
@@ -119,6 +119,8 @@ extern char *strrchr ();
 
 #include "filenames.h"
 
+#include "str-util.h"
+
 #if !HAVE_DECL_FFS
 extern int ffs (int);
 #endif
diff --git a/binutils/sysdep.h b/binutils/sysdep.h
index 183bb01653..022029a3f9 100644
--- a/binutils/sysdep.h
+++ b/binutils/sysdep.h
@@ -78,6 +78,8 @@ extern char *strrchr ();
 
 #include "binary-io.h"
 
+#include "str-util.h"
+
 #if !HAVE_DECL_STPCPY
 extern char *stpcpy (char *, const char *);
 #endif
diff --git a/gas/as.h b/gas/as.h
index d6ac208289..a0346ad062 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -97,6 +97,8 @@
 /* Define the standard progress macros.  */
 #include "progress.h"
 
+#include "str-util.h"
+
 /* Other stuff from config.h.  */
 #ifdef NEED_DECLARATION_ENVIRON
 extern char **environ;
diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
index 1de747f186..65569ffe78 100644
--- a/gdbsupport/common-utils.h
+++ b/gdbsupport/common-utils.h
@@ -45,6 +45,8 @@
 
 #include "gdb_string_view.h"
 
+#include "str-util.h"
+
 /* xmalloc(), xrealloc() and xcalloc() have already been declared in
    "libiberty.h". */
 
diff --git a/include/str-util.h b/include/str-util.h
new file mode 100644
index 0000000000..24aba56563
--- /dev/null
+++ b/include/str-util.h
@@ -0,0 +1,27 @@
+/* String utility functions used by GDB and binutils.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   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 STR_UTIL_H
+#define STR_UTIL_H
+
+/* Return 1 if the start of STR matches PREFIX, 0 otherwise.  */
+
+static inline int
+startswith (const char *str, const char *prefix)
+{
+  return strncmp (str, prefix, strlen (prefix)) == 0;
+}
+#endif
diff --git a/ld/sysdep.h b/ld/sysdep.h
index 206c02c8c5..9e97d2d95b 100644
--- a/ld/sysdep.h
+++ b/ld/sysdep.h
@@ -68,6 +68,8 @@ extern char *strrchr ();
 #include "fopen-same.h"
 #endif
 
+#include "str-util.h"
+
 #ifdef HAVE_FCNTL_H
 #include <fcntl.h>
 #else
diff --git a/opcodes/sysdep.h b/opcodes/sysdep.h
index bcac6d851c..a0732861b7 100644
--- a/opcodes/sysdep.h
+++ b/opcodes/sysdep.h
@@ -52,6 +52,8 @@
 #endif
 #endif
 
+#include "str-util.h"
+
 #if !HAVE_DECL_STPCPY
 extern char *stpcpy (char *__dest, const char *__src);
 #endif


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Mike Frysinger]

On 23 Mar 2021 10:32, Alan Modra wrote:
> On Mon, Mar 22, 2021 at 05:42:50PM +0100, Martin Liška wrote:
> > Anyway for the sim failures. What about directly including <string.h> in bfd-in.h?
> 
> That would be OK if we didn't care about really old systems.  See for
> example the way bfd/sysdep.h includes string.h.
> 
> But all of this horrible old code that likely hasn't been tested is
> eons should simply disappear.  Instead binutils should be using the
> gnulib import already available in the binutils-gdb repository to
> support old systems.  I've had that project on my todo list for quite
> a while.  We'd be able to include stdint.h and stdbool.h for example,
> throwing away bfd_stdint.h and replacing bfd_boolean with bool.

i agree with the idea that we've accumulated a lot of cruft that we never
test and it's unclear how many users still rely on it.  for the sim, i've
made C11 a requirement inline with GDB's C++11 requirement.  that means i
can assume <string.h> among other things.

> Meanwhile, this is a tidied version of the patch I sent you last
> night, Tom.  If you already have one of your own then please ignore
> this.

this fixes the sim for me, thanks
-mike

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

On 3/22/21 7:56 PM, Alan Modra wrote:
> On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
>> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
>> complaining about "-Werror=implicit-function-declaration" regarding strncmp
>> and strlen.
>>
>> Is this the breakage you're talking about? Just so I know what to expect
>> when it gets fixed.
> 
> Yes.
> 

Thanks. Are there plans to address this or should I come up with a patch?

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Alan Modra]

On Thu, Mar 25, 2021 at 07:53:04AM -0300, Luis Machado wrote:
> On 3/22/21 7:56 PM, Alan Modra wrote:
> > On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
> > > Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
> > > complaining about "-Werror=implicit-function-declaration" regarding strncmp
> > > and strlen.
> > > 
> > > Is this the breakage you're talking about? Just so I know what to expect
> > > when it gets fixed.
> > 
> > Yes.
> > 
> 
> Thanks. Are there plans to address this or should I come up with a patch?

I posted a patch here
https://sourceware.org/pipermail/binutils/2021-March/115863.html
It's been tested with gdb, sim and binutils builds.

Tom offered to solve the problem himself, so I'm waiting on that or
for someone in the gdb camp to review my patch.  Since I messed this
up in the first place by taking a comment by Tom as a go-ahead rather
than first posting a patch for proper review by gdb maintainers, I'm
being a little cautious in committing the above patch.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

On 3/25/21 8:54 AM, Alan Modra wrote:
> On Thu, Mar 25, 2021 at 07:53:04AM -0300, Luis Machado wrote:
>> On 3/22/21 7:56 PM, Alan Modra wrote:
>>> On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
>>>> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
>>>> complaining about "-Werror=implicit-function-declaration" regarding strncmp
>>>> and strlen.
>>>>
>>>> Is this the breakage you're talking about? Just so I know what to expect
>>>> when it gets fixed.
>>>
>>> Yes.
>>>
>>
>> Thanks. Are there plans to address this or should I come up with a patch?
> 
> I posted a patch here
> https://sourceware.org/pipermail/binutils/2021-March/115863.html
> It's been tested with gdb, sim and binutils builds.
> 
> Tom offered to solve the problem himself, so I'm waiting on that or
> for someone in the gdb camp to review my patch.  Since I messed this
> up in the first place by taking a comment by Tom as a go-ahead rather
> than first posting a patch for proper review by gdb maintainers, I'm
> being a little cautious in committing the above patch.
> 

Ah, I missed that patch. Let me give that a try with the arm/aarch64 
sims and I'll let you know how that works.

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

Hi,

On 3/25/21 8:54 AM, Alan Modra wrote:
> On Thu, Mar 25, 2021 at 07:53:04AM -0300, Luis Machado wrote:
>> On 3/22/21 7:56 PM, Alan Modra wrote:
>>> On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
>>>> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
>>>> complaining about "-Werror=implicit-function-declaration" regarding strncmp
>>>> and strlen.
>>>>
>>>> Is this the breakage you're talking about? Just so I know what to expect
>>>> when it gets fixed.
>>>
>>> Yes.
>>>
>>
>> Thanks. Are there plans to address this or should I come up with a patch?
> 
> I posted a patch here
> https://sourceware.org/pipermail/binutils/2021-March/115863.html
> It's been tested with gdb, sim and binutils builds.
> 
> Tom offered to solve the problem himself, so I'm waiting on that or
> for someone in the gdb camp to review my patch.  Since I messed this
> up in the first place by taking a comment by Tom as a go-ahead rather
> than first posting a patch for proper review by gdb maintainers, I'm
> being a little cautious in committing the above patch.
> 

I applied the above patch to today's master binutils-gdb and gave 
configure's --enable-targets=all a try, but I ran into the following:

In file included from ../../../repos/binutils-gdb/bfd/archive.c:135:
./bfd.h:568:1: error: redefinition of ‘startswith’
   568 | startswith (const char *str, const char *prefix)
       | ^~~~~~~~~~
In file included from ../../../repos/binutils-gdb/bfd/sysdep.h:122,
                  from ../../../repos/binutils-gdb/bfd/archive.c:134:
../../../repos/binutils-gdb/bfd/../include/str-util.h:23:1: note: 
previous definition of ‘startswith’ was here
    23 | startswith (const char *str, const char *prefix)
       | ^~~~~~~~~~

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Mike Frysinger]

On 25 Mar 2021 16:47, Luis Machado wrote:
> On 3/25/21 8:54 AM, Alan Modra wrote:
> > On Thu, Mar 25, 2021 at 07:53:04AM -0300, Luis Machado wrote:
> >> On 3/22/21 7:56 PM, Alan Modra wrote:
> >>> On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
> >>>> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
> >>>> complaining about "-Werror=implicit-function-declaration" regarding strncmp
> >>>> and strlen.
> >>>>
> >>>> Is this the breakage you're talking about? Just so I know what to expect
> >>>> when it gets fixed.
> >>>
> >>> Yes.
> >>>
> >>
> >> Thanks. Are there plans to address this or should I come up with a patch?
> > 
> > I posted a patch here
> > https://sourceware.org/pipermail/binutils/2021-March/115863.html
> > It's been tested with gdb, sim and binutils builds.
> > 
> > Tom offered to solve the problem himself, so I'm waiting on that or
> > for someone in the gdb camp to review my patch.  Since I messed this
> > up in the first place by taking a comment by Tom as a go-ahead rather
> > than first posting a patch for proper review by gdb maintainers, I'm
> > being a little cautious in committing the above patch.
> > 
> 
> I applied the above patch to today's master binutils-gdb and gave 
> configure's --enable-targets=all a try, but I ran into the following:
> 
> In file included from ../../../repos/binutils-gdb/bfd/archive.c:135:
> ./bfd.h:568:1: error: redefinition of ‘startswith’
>    568 | startswith (const char *str, const char *prefix)
>        | ^~~~~~~~~~
> In file included from ../../../repos/binutils-gdb/bfd/sysdep.h:122,
>                   from ../../../repos/binutils-gdb/bfd/archive.c:134:
> ../../../repos/binutils-gdb/bfd/../include/str-util.h:23:1: note: 
> previous definition of ‘startswith’ was here
>     23 | startswith (const char *str, const char *prefix)
>        | ^~~~~~~~~~

Alan's patch didn't include the regenerated bfd.h inputs, so you'll
have to do that on your side.
-mike

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

On 3/25/21 7:31 PM, Mike Frysinger wrote:
> On 25 Mar 2021 16:47, Luis Machado wrote:
>> On 3/25/21 8:54 AM, Alan Modra wrote:
>>> On Thu, Mar 25, 2021 at 07:53:04AM -0300, Luis Machado wrote:
>>>> On 3/22/21 7:56 PM, Alan Modra wrote:
>>>>> On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
>>>>>> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
>>>>>> complaining about "-Werror=implicit-function-declaration" regarding strncmp
>>>>>> and strlen.
>>>>>>
>>>>>> Is this the breakage you're talking about? Just so I know what to expect
>>>>>> when it gets fixed.
>>>>>
>>>>> Yes.
>>>>>
>>>>
>>>> Thanks. Are there plans to address this or should I come up with a patch?
>>>
>>> I posted a patch here
>>> https://sourceware.org/pipermail/binutils/2021-March/115863.html
>>> It's been tested with gdb, sim and binutils builds.
>>>
>>> Tom offered to solve the problem himself, so I'm waiting on that or
>>> for someone in the gdb camp to review my patch.  Since I messed this
>>> up in the first place by taking a comment by Tom as a go-ahead rather
>>> than first posting a patch for proper review by gdb maintainers, I'm
>>> being a little cautious in committing the above patch.
>>>
>>
>> I applied the above patch to today's master binutils-gdb and gave
>> configure's --enable-targets=all a try, but I ran into the following:
>>
>> In file included from ../../../repos/binutils-gdb/bfd/archive.c:135:
>> ./bfd.h:568:1: error: redefinition of ‘startswith’
>>     568 | startswith (const char *str, const char *prefix)
>>         | ^~~~~~~~~~
>> In file included from ../../../repos/binutils-gdb/bfd/sysdep.h:122,
>>                    from ../../../repos/binutils-gdb/bfd/archive.c:134:
>> ../../../repos/binutils-gdb/bfd/../include/str-util.h:23:1: note:
>> previous definition of ‘startswith’ was here
>>      23 | startswith (const char *str, const char *prefix)
>>         | ^~~~~~~~~~
> 
> Alan's patch didn't include the regenerated bfd.h inputs, so you'll
> have to do that on your side.
> -mike
> 

Oops. Regenerated now. I gave it a try again and it worked OK.

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

Hi Alan, Tom,

On 3/26/21 8:44 AM, Luis Machado wrote:
> On 3/25/21 7:31 PM, Mike Frysinger wrote:
>> On 25 Mar 2021 16:47, Luis Machado wrote:
>>> On 3/25/21 8:54 AM, Alan Modra wrote:
>>>> On Thu, Mar 25, 2021 at 07:53:04AM -0300, Luis Machado wrote:
>>>>> On 3/22/21 7:56 PM, Alan Modra wrote:
>>>>>> On Mon, Mar 22, 2021 at 01:13:00PM -0300, Luis Machado wrote:
>>>>>>> Just FTR, I'm seeing breakage in sim/aarch64 and sim/arm. Both are
>>>>>>> complaining about "-Werror=implicit-function-declaration" 
>>>>>>> regarding strncmp
>>>>>>> and strlen.
>>>>>>>
>>>>>>> Is this the breakage you're talking about? Just so I know what to 
>>>>>>> expect
>>>>>>> when it gets fixed.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>
>>>>> Thanks. Are there plans to address this or should I come up with a 
>>>>> patch?
>>>>
>>>> I posted a patch here
>>>> https://sourceware.org/pipermail/binutils/2021-March/115863.html
>>>> It's been tested with gdb, sim and binutils builds.
>>>>
>>>> Tom offered to solve the problem himself, so I'm waiting on that or
>>>> for someone in the gdb camp to review my patch.  Since I messed this
>>>> up in the first place by taking a comment by Tom as a go-ahead rather
>>>> than first posting a patch for proper review by gdb maintainers, I'm
>>>> being a little cautious in committing the above patch.
>>>>
>>>
>>> I applied the above patch to today's master binutils-gdb and gave
>>> configure's --enable-targets=all a try, but I ran into the following:
>>>
>>> In file included from ../../../repos/binutils-gdb/bfd/archive.c:135:
>>> ./bfd.h:568:1: error: redefinition of ‘startswith’
>>>     568 | startswith (const char *str, const char *prefix)
>>>         | ^~~~~~~~~~
>>> In file included from ../../../repos/binutils-gdb/bfd/sysdep.h:122,
>>>                    from ../../../repos/binutils-gdb/bfd/archive.c:134:
>>> ../../../repos/binutils-gdb/bfd/../include/str-util.h:23:1: note:
>>> previous definition of ‘startswith’ was here
>>>      23 | startswith (const char *str, const char *prefix)
>>>         | ^~~~~~~~~~
>>
>> Alan's patch didn't include the regenerated bfd.h inputs, so you'll
>> have to do that on your side.
>> -mike
>>
> 
> Oops. Regenerated now. I gave it a try again and it worked OK.

Given this has been broken for a little while, and given Alan's patch 
fixes the problem for arm/aarch64 sims (from what I tested), can we push 
this for now and get it addressed in some other way later, if someone 
wants it?

I think someone's hit this on riscv as well.

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Martin Liška]

> Given this has been broken for a little while, and given Alan's patch fixes the problem for arm/aarch64 sims (from what I tested), can we push this for now and get it addressed in some other way later, if someone wants it?
> 
> I think someone's hit this on riscv as well.

It's fixed now with Alan's patch:

commit 57ae980e3290c0c1a9fb4a93144cc5b24457f05a
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Mar 31 10:02:08 2021 +1030

     Include string.h in bfd.h and delete LITMEMCPY, LITSTRCPY
     
     This fixes the issue that startswith depends on strncpy being
     declared, and not all projects using bfd.h include string.h before
     bfd.h.  I've also deleted some macros that don't find much use
     anywhere.
     
     bfd/
             * bfd-in.h: Include string.h.
             (LITMEMCPY, LITSTRCPY): Delete.
             * bfd-in2.h: Regenerate.
     binutils/
             * prdbg.c (pr_function_type): Replace LITSTTCPY with strcpy.

Cheers,
Martin

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Luis Machado]

On 3/31/21 10:12 AM, Martin Liška wrote:
>> Given this has been broken for a little while, and given Alan's patch 
>> fixes the problem for arm/aarch64 sims (from what I tested), can we 
>> push this for now and get it addressed in some other way later, if 
>> someone wants it?
>>
>> I think someone's hit this on riscv as well.
> 
> It's fixed now with Alan's patch:
> 
> commit 57ae980e3290c0c1a9fb4a93144cc5b24457f05a
> Author: Alan Modra <amodra@gmail.com>
> Date:   Wed Mar 31 10:02:08 2021 +1030
> 
>      Include string.h in bfd.h and delete LITMEMCPY, LITSTRCPY
>      This fixes the issue that startswith depends on strncpy being
>      declared, and not all projects using bfd.h include string.h before
>      bfd.h.  I've also deleted some macros that don't find much use
>      anywhere.
>      bfd/
>              * bfd-in.h: Include string.h.
>              (LITMEMCPY, LITSTRCPY): Delete.
>              * bfd-in2.h: Regenerate.
>      binutils/
>              * prdbg.c (pr_function_type): Replace LITSTTCPY with strcpy.
> 
> Cheers,
> Martin

Thanks Alan/Martin. I've confirmed the arm/aarch64 sim failures are 
fixed now.

Re: [PATCH] Add startswith function and use it instead of CONST_STRNEQ.

[Tom Tromey]

Hi.  I'm sorry about the delay on this.

Alan> But all of this horrible old code that likely hasn't been tested is
Alan> eons should simply disappear.

gdb has this issue to a lesser degree as well, where it checks for
headers that have not been missing in 20 years.  This kind of rot is one
of the downsides of the autoconf approach.

Alan> Instead binutils should be using the
Alan> gnulib import already available in the binutils-gdb repository to
Alan> support old systems.  I've had that project on my todo list for quite
Alan> a while.  We'd be able to include stdint.h and stdbool.h for example,
Alan> throwing away bfd_stdint.h and replacing bfd_boolean with bool.

We've had mixed experiences with gnulib in gdb.  Partly that's because
gdb is in C++; but also partly because gnulib is fairly opinionated
about some things, and if you want to be more flexible, it's difficult.

Anyway, I think it would be a good thing to try.  And, it's relatively
easy now that gnulib has moved to the top-level.  The main difficulty
that I would anticipate is that I think there are some gnulib modules
that can't be used for gdb; but if binutils needed these then we'd be in
a bit of a quandary.

I suppose though that the gnulib model is to patch things upstream and
so we could try to work out problems that way.

Alan> Meanwhile, this is a tidied version of the patch I sent you last
Alan> night, Tom.  If you already have one of your own then please ignore
Alan> this.

This looks good to me.  Thank you.

Alan> +#ifndef STR_UTIL_H
Alan> +#define STR_UTIL_H
Alan> +
Alan> +/* Return 1 if the start of STR matches PREFIX, 0 otherwise.  */
Alan> +
Alan> +static inline int
Alan> +startswith (const char *str, const char *prefix)
Alan> +{
Alan> +  return strncmp (str, prefix, strlen (prefix)) == 0;

In keeping with the above, I tend to think that adding a <string.h>
include in this file would be fine, and unlikely to break the build on
any real system.

If there is one, that would be interesting information.

Here's what gnulib has to say about string.h:

https://www.gnu.org/software/gnulib/manual/html_node/string_002eh.html

... if it were missing anywhere, I suppose I'd expect a bullet point to
that effect.

Tom