Small patch to enable build of gdb-7.6 for GNU/Hurd

Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

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

Hello,

Since gdb 7.4.1 a PATH_MAX usage was introduced in the file
gdb/nto-tdep.c:nto_init_solib_absolute_prefix(). This function is only
called in gdb/nto-procfs.c. This file is QNX Neutrino specific and is
not compiled for Hurd (while nto-tdep.c, also QNX-specific is).

The attached patch solve_PATH_MAX_issue.patch fixes the FTBFS for
GNU/Hurd. Alternately the build should be changed to exclude compilation
of nto-tdep.c for non-QNX architectures. The compilation of various
targets specific code is enabled by --enable-targets=all in
the .../gdb/Makefile ALL_TARGET_OBS variable. This seems to be common of
builds for all architectures in Debian.

In case you need feedback from me use Cc: since I'm not subscribed to
gdb-patches.

Thanks,
Svante Signell


[-- Attachment #2: solve_PATH_MAX_issue.patch --]
[-- Type: text/x-patch, Size: 1128 bytes --]

--- a/gdb/nto-tdep.c	2013-05-23 14:28:24.000000000 +0000
+++ b/gdb/nto-tdep.c	2013-05-23 15:01:24.000000000 +0000
@@ -147,9 +147,11 @@ nto_find_and_open_solib (char *solib, un
 void
 nto_init_solib_absolute_prefix (void)
 {
-  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
+  char *buf, *arch_path;
   char *nto_root, *endian;
   const char *arch;
+  int arch_len, len;
+#define FMT "set solib-absolute-prefix %s"
 
   nto_root = nto_target ();
   if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
@@ -172,9 +174,13 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
+  arch_len = strlen (nto_root) + 1 + strlen (arch) + strlen (endian) + 1;
+  arch_path = alloca (arch_len);
+  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
 
-  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
+  len = strlen (FMT) - 2 + strlen (arch_path) + 1;
+  buf =  alloca (len);
+  xsnprintf (buf, len, FMT, arch_path);
   execute_command (buf, 0);
 }
 

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Tom Tromey]

>>>>> "Svante" == Svante Signell <svante.signell@gmail.com> writes:

Svante> In case you need feedback from me use Cc: since I'm not subscribed to
Svante> gdb-patches.

It needs a ChangeLog entry.

Svante> -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
Svante> +  arch_len = strlen (nto_root) + 1 + strlen (arch) + strlen (endian) + 1;
Svante> +  arch_path = alloca (arch_len);
Svante> +  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
 
Svante> -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
Svante> +  len = strlen (FMT) - 2 + strlen (arch_path) + 1;
Svante> +  buf =  alloca (len);
Svante> +  xsnprintf (buf, len, FMT, arch_path);
Svante>    execute_command (buf, 0);

I think it would be simpler to use a single xstrprintf plus a cleanup.

Tom

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

On Thu, 2013-05-23 at 11:07 -0600, Tom Tromey wrote:
> >>>>> "Svante" == Svante Signell <svante.signell@gmail.com> writes:
> 
> Svante> In case you need feedback from me use Cc: since I'm not subscribed to
> Svante> gdb-patches.
> 
> It needs a ChangeLog entry.
> 
> Svante> -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
> Svante> +  arch_len = strlen (nto_root) + 1 + strlen (arch) + strlen (endian) + 1;
> Svante> +  arch_path = alloca (arch_len);
> Svante> +  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
>  
> Svante> -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
> Svante> +  len = strlen (FMT) - 2 + strlen (arch_path) + 1;
> Svante> +  buf =  alloca (len);
> Svante> +  xsnprintf (buf, len, FMT, arch_path);
> Svante>    execute_command (buf, 0);
> 
> I think it would be simpler to use a single xstrprintf plus a cleanup.

Thanks for your prompt reply. The code construct I used was the same as
the function defined before in the same source file:
int nto_find_and_open_solib (char *solib, unsigned o_flags, char
**temp_pathname)

How to create a correct ChangeLog entry? Any tools available, emacs?

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Tom Tromey]

>>>>> "Svante" == Svante Signell <svante.signell@gmail.com> writes:

Svante> Thanks for your prompt reply. The code construct I used was the same as
Svante> the function defined before in the same source file:
Svante> int nto_find_and_open_solib (char *solib, unsigned o_flags, char
Svante> **temp_pathname)

I guess I don't care that much.

Svante> How to create a correct ChangeLog entry? Any tools available, emacs?

Haha, yeah, pretty much only emacs.
See C-h f add-change-log-entry

Tom

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

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

On Thu, 2013-05-23 at 11:27 -0600, Tom Tromey wrote:
> >>>>> "Svante" == Svante Signell <svante.signell@gmail.com> writes:
> 
> Svante> Thanks for your prompt reply. The code construct I used was the same as
> Svante> the function defined before in the same source file:
> Svante> int nto_find_and_open_solib (char *solib, unsigned o_flags, char
> Svante> **temp_pathname)
> 
> I guess I don't care that much.
> 
> Svante> How to create a correct ChangeLog entry? Any tools available, emacs?
> 
> Haha, yeah, pretty much only emacs.
> See C-h f add-change-log-entry

A ChangeLog entry is attached (good enough?)

Svante

[-- Attachment #2: ChangeLog.add --]
[-- Type: text/plain, Size: 187 bytes --]

2013-05-23  Svante Signell  <svante.signell@gmail.com>

	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
	problems for systems not defining PATH_MAX by using alloca
	instead.

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Sergio Durigan Junior]

Thanks for your patch, Svante.

On Thursday, May 23 2013, Svante Signell wrote:

> --- a/gdb/nto-tdep.c	2013-05-23 14:28:24.000000000 +0000
> +++ b/gdb/nto-tdep.c	2013-05-23 15:01:24.000000000 +0000
> @@ -147,9 +147,11 @@ nto_find_and_open_solib (char *solib, un
>  void
>  nto_init_solib_absolute_prefix (void)
>  {
> -  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
> +  char *buf, *arch_path;
>    char *nto_root, *endian;
>    const char *arch;
> +  int arch_len, len;
> +#define FMT "set solib-absolute-prefix %s"
>  
>    nto_root = nto_target ();
>    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> @@ -172,9 +174,13 @@ nto_init_solib_absolute_prefix (void)
>  	       == BFD_ENDIAN_BIG ? "be" : "le";
>      }
>  
> -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
> +  arch_len = strlen (nto_root) + 1 + strlen (arch) + strlen (endian) + 1;
> +  arch_path = alloca (arch_len);
> +  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
>  
> -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
> +  len = strlen (FMT) - 2 + strlen (arch_path) + 1;
> +  buf =  alloca (len);
> +  xsnprintf (buf, len, FMT, arch_path);

When I define things in the middle of functions/code, I usually #undef
them just after I finished using, to avoid conflicts with others
#defines.  Specially when the #define is called "FMT", which is very
generic IMO.

But that is just a good practice I try to follow.

-- 
Sergio

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Sergio Durigan Junior]

On Thursday, May 23 2013, Svante Signell wrote:

> A ChangeLog entry is attached (good enough?)
>
> Svante
>
> 2013-05-23  Svante Signell  <svante.signell@gmail.com>
>
> 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> 	problems for systems not defining PATH_MAX by using alloca
> 	instead.

It seems good enough for me.  I am not a maintainer, though.

-- 
Sergio

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Joel Brobecker]

On Thu, May 23, 2013 at 11:59:46PM -0300, Sergio Durigan Junior wrote:
> Thanks for your patch, Svante.
> 
> On Thursday, May 23 2013, Svante Signell wrote:
> 
> > --- a/gdb/nto-tdep.c	2013-05-23 14:28:24.000000000 +0000
> > +++ b/gdb/nto-tdep.c	2013-05-23 15:01:24.000000000 +0000
> > @@ -147,9 +147,11 @@ nto_find_and_open_solib (char *solib, un
> >  void
> >  nto_init_solib_absolute_prefix (void)
> >  {
> > -  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
> > +  char *buf, *arch_path;
> >    char *nto_root, *endian;
> >    const char *arch;
> > +  int arch_len, len;
> > +#define FMT "set solib-absolute-prefix %s"
> >  
> >    nto_root = nto_target ();
> >    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> > @@ -172,9 +174,13 @@ nto_init_solib_absolute_prefix (void)
> >  	       == BFD_ENDIAN_BIG ? "be" : "le";
> >      }
> >  
> > -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
> > +  arch_len = strlen (nto_root) + 1 + strlen (arch) + strlen (endian) + 1;
> > +  arch_path = alloca (arch_len);
> > +  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
> >  
> > -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
> > +  len = strlen (FMT) - 2 + strlen (arch_path) + 1;
> > +  buf =  alloca (len);
> > +  xsnprintf (buf, len, FMT, arch_path);
> 
> When I define things in the middle of functions/code, I usually #undef
> them just after I finished using, to avoid conflicts with others
> #defines.  Specially when the #define is called "FMT", which is very
> generic IMO.

Agreed.

And quite honestly, I find this use of a macro in the middle of
a function quite ugly and unnecessary, no matter what was done
before.  My preference mirrors Tom's suggestion, but failing that,
I'd rather FMT be a const char * or a const char [].

I do not think that this file has a responsible maintainer, so
it falls under Global Maintainership...

-- 
Joel

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Pedro Alves]

On 05/24/2013 05:27 AM, Joel Brobecker wrote:
> On Thu, May 23, 2013 at 11:59:46PM -0300, Sergio Durigan Junior wrote:
>> Thanks for your patch, Svante.
>>
>> On Thursday, May 23 2013, Svante Signell wrote:
>>
>>> --- a/gdb/nto-tdep.c	2013-05-23 14:28:24.000000000 +0000
>>> +++ b/gdb/nto-tdep.c	2013-05-23 15:01:24.000000000 +0000
>>> @@ -147,9 +147,11 @@ nto_find_and_open_solib (char *solib, un
>>>  void
>>>  nto_init_solib_absolute_prefix (void)
>>>  {
>>> -  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
>>> +  char *buf, *arch_path;
>>>    char *nto_root, *endian;
>>>    const char *arch;
>>> +  int arch_len, len;
>>> +#define FMT "set solib-absolute-prefix %s"
>>>  
>>>    nto_root = nto_target ();
>>>    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
>>> @@ -172,9 +174,13 @@ nto_init_solib_absolute_prefix (void)
>>>  	       == BFD_ENDIAN_BIG ? "be" : "le";
>>>      }
>>>  
>>> -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
>>> +  arch_len = strlen (nto_root) + 1 + strlen (arch) + strlen (endian) + 1;
>>> +  arch_path = alloca (arch_len);
>>> +  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
>>>  
>>> -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
>>> +  len = strlen (FMT) - 2 + strlen (arch_path) + 1;
>>> +  buf =  alloca (len);
>>> +  xsnprintf (buf, len, FMT, arch_path);
>>
>> When I define things in the middle of functions/code, I usually #undef
>> them just after I finished using, to avoid conflicts with others
>> #defines.  Specially when the #define is called "FMT", which is very
>> generic IMO.
> 
> Agreed.
> 
> And quite honestly, I find this use of a macro in the middle of
> a function quite ugly and unnecessary, no matter what was done
> before.  My preference mirrors Tom's suggestion, but failing that,
> I'd rather FMT be a const char * or a const char [].

Agreed.

const char fmt[] = "..." is better than 'const char *', as the
latter gives you an unnecessary extra pointer.

Make that 'static const char fmt[] = "...";' even.

But really alloca for potentially large/unbounded buffers is evil.
xstrprintf plus a cleanup would definitely be my preference.

-- 
Pedro Alves

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

On Fri, 2013-05-24 at 09:53 +0100, Pedro Alves wrote:
> On 05/24/2013 05:27 AM, Joel Brobecker wrote:
> > On Thu, May 23, 2013 at 11:59:46PM -0300, Sergio Durigan Junior wrote:
> >> Thanks for your patch, Svante.
> >>
> >> On Thursday, May 23 2013, Svante Signell wrote:
> >>
...
> > And quite honestly, I find this use of a macro in the middle of
> > a function quite ugly and unnecessary, no matter what was done
> > before.  My preference mirrors Tom's suggestion, but failing that,
> > I'd rather FMT be a const char * or a const char [].
> 
> Agreed.
> 
> const char fmt[] = "..." is better than 'const char *', as the
> latter gives you an unnecessary extra pointer.
> 
> Make that 'static const char fmt[] = "...";' even.
> 
> But really alloca for potentially large/unbounded buffers is evil.
> xstrprintf plus a cleanup would definitely be my preference.

I will change to use xstrprintf instead. Updated patch with ChangeLog
entry will follow shortly. Is it OK to modify also the other (preceding)
function in the same way (for consistency)?

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Sergio Durigan Junior]

On Friday, May 24 2013, Svante Signell wrote:

> I will change to use xstrprintf instead. Updated patch with ChangeLog
> entry will follow shortly. Is it OK to modify also the other (preceding)
> function in the same way (for consistency)?

Thanks.  It is OK IMO, but I'd prefer if you did that in a separate
patch.

-- 
Sergio

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

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

On Fri, 2013-05-24 at 06:13 -0300, Sergio Durigan Junior wrote:
> On Friday, May 24 2013, Svante Signell wrote:
> 
> > I will change to use xstrprintf instead. Updated patch with ChangeLog
> > entry will follow shortly. Is it OK to modify also the other (preceding)
> > function in the same way (for consistency)?
> 
> Thanks.  It is OK IMO, but I'd prefer if you did that in a separate
> patch.

Attached is an updated patch for the build problems on systems where
PATH_MAX is not defined.

Thanks,
Svante


[-- Attachment #2: solve_PATH_MAX_issue.patch --]
[-- Type: text/x-patch, Size: 830 bytes --]

--- a/gdb/nto-tdep.c	2013-05-23 14:28:24.000000000 +0000
+++ b/gdb/nto-tdep.c	2013-05-27 10:54:13.000000000 +0000
@@ -147,7 +147,7 @@ nto_find_and_open_solib (char *solib, un
 void
 nto_init_solib_absolute_prefix (void)
 {
-  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
+  char *buf, *arch_path;
   char *nto_root, *endian;
   const char *arch;
 
@@ -172,10 +172,11 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
-
-  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
+  arch_path = xstrprintf ("%s/%s%s", nto_root, arch, endian);
+  buf = xstrprintf ("set solib-absolute-prefix \"%s\"", arch_path);
+  xfree(arch_path);
   execute_command (buf, 0);
+  xfree(buf);
 }
 
 char **

[-- Attachment #3: ChangeLog.solve_PATH_MAX_issue --]
[-- Type: text/plain, Size: 201 bytes --]

2013-05-27  Svante Signell  <srs@hurd-2013.my.own.domain>

	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
	problems for systems not defining PATH_MAX by using xstrprintf and
	a cleanup.

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Joel Brobecker]

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

> 2013-05-27  Svante Signell  <srs@hurd-2013.my.own.domain>
> 
> 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> 	problems for systems not defining PATH_MAX by using xstrprintf and
> 	a cleanup.

Is the domain name above really valid?

> Attached is an updated patch for the build problems on systems where
> PATH_MAX is not defined.

In this case, I think you need to use what we call a "cleanup",
because execute_command might trigger an "error" (GDB's poor man's
exception mechanism), thus prevening the last xfree from releasing
buf.

A formatting nit: The GNU Coding Standard, which we follow in GDB,
requires a space before opening parens.

While looking at this code, I don't think you'll need 2 xstrprintf
either. Can you try the attached patch?

-- 
Joel

[-- Attachment #2: nto-tdep.diff --]
[-- Type: text/x-diff, Size: 997 bytes --]

diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
index 748869f..cf4fd60 100644
--- a/gdb/nto-tdep.c
+++ b/gdb/nto-tdep.c
@@ -147,9 +147,10 @@ nto_find_and_open_solib (char *solib, unsigned o_flags, char **temp_pathname)
 void
 nto_init_solib_absolute_prefix (void)
 {
-  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
+  char buf;
   char *nto_root, *endian;
   const char *arch;
+  struct cleanup *old_chain;
 
   nto_root = nto_target ();
   if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
@@ -172,10 +173,11 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
-
-  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
+  buf = xstrprintf ("set solib-absolute-prefix \"%s/%s%s\"",
+		    nto_root, arch, endian);
+  old_chain = make_cleanup (xfree, buf);
   execute_command (buf, 0);
+  make_cleanup (old_chain);
 }
 
 char **

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

On Mon, 2013-05-27 at 16:10 +0400, Joel Brobecker wrote:
> > 2013-05-27  Svante Signell  <srs@hurd-2013.my.own.domain>
> > 
> > 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> > 	problems for systems not defining PATH_MAX by using xstrprintf and
> > 	a cleanup.
> 
> Is the domain name above really valid?

No, it's a local one. I forgot to change that one.

> > Attached is an updated patch for the build problems on systems where
> > PATH_MAX is not defined.
> 
> In this case, I think you need to use what we call a "cleanup",
> because execute_command might trigger an "error" (GDB's poor man's
> exception mechanism), thus prevening the last xfree from releasing
> buf.

I thought of that problem and looking into the code for execute_command
convinced me that that command should always return. Obviously I was
wrong, you are the experts.

> A formatting nit: The GNU Coding Standard, which we follow in GDB,
> requires a space before opening parens.

I just realized that after submitting the mail.

> While looking at this code, I don't think you'll need 2 xstrprintf
> either. Can you try the attached patch?

Yes, of course this code is much neater. I will report build status
soon.

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

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

On Mon, 2013-05-27 at 14:21 +0200, Svante Signell wrote:
> On Mon, 2013-05-27 at 16:10 +0400, Joel Brobecker wrote:
> > > 2013-05-27  Svante Signell  <srs@hurd-2013.my.own.domain>
> > > 
> > > 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> > > 	problems for systems not defining PATH_MAX by using xstrprintf and
> > > 	a cleanup.
> > 
> > Is the domain name above really valid?
> 
> No, it's a local one. I forgot to change that one.

Attaching an ChangeLog entry with a correct domain

> > While looking at this code, I don't think you'll need 2 xstrprintf
> > either. Can you try the attached patch?
> 
> Yes, of course this code is much neater. I will report build status
> soon.

I had to change the second make_cleanup call to do_cleanups instead, see
the updated patch. OK now?


[-- Attachment #2: ChangeLog.solve_PATH_MAX_issue --]
[-- Type: text/plain, Size: 206 bytes --]

2013-05-27  Svante Signell  <svante.signell@gmail.com>

	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
	problems for systems not defining PATH_MAX by using xstrprintf
	followed by a cleanup.



[-- Attachment #3: nto-tdep.diff --]
[-- Type: text/x-patch, Size: 996 bytes --]

diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
index 748869f..cf4fd60 100644
--- a/gdb/nto-tdep.c
+++ b/gdb/nto-tdep.c
@@ -147,9 +147,10 @@ nto_find_and_open_solib (char *solib, unsigned o_flags, char **temp_pathname)
 void
 nto_init_solib_absolute_prefix (void)
 {
-  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
+  char buf;
   char *nto_root, *endian;
   const char *arch;
+  struct cleanup *old_chain;
 
   nto_root = nto_target ();
   if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
@@ -172,10 +173,11 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
-
-  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
+  buf = xstrprintf ("set solib-absolute-prefix \"%s/%s%s\"",
+		    nto_root, arch, endian);
+  old_chain = make_cleanup (xfree, buf);
   execute_command (buf, 0);
+  do_cleanups (old_chain);
 }
 
 char **

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

On Mon, 2013-05-27 at 15:48 +0200, Svante Signell wrote:
> On Mon, 2013-05-27 at 14:21 +0200, Svante Signell wrote:
> > On Mon, 2013-05-27 at 16:10 +0400, Joel Brobecker wrote:
> > > > 2013-05-27  Svante Signell  <srs@hurd-2013.my.own.domain>
> > > > 
> > > > 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> > > > 	problems for systems not defining PATH_MAX by using xstrprintf and
> > > > 	a cleanup.
> > > 
> > > Is the domain name above really valid?
> > 
> > No, it's a local one. I forgot to change that one.
> 
> Attaching an ChangeLog entry with a correct domain
> 
> > > While looking at this code, I don't think you'll need 2 xstrprintf
> > > either. Can you try the attached patch?
> > 
> > Yes, of course this code is much neater. I will report build status
> > soon.
> 
> I had to change the second make_cleanup call to do_cleanups instead, see
> the updated patch. OK now?

Additionally, I think the char buf; statement should read char *buf; in
the patch.

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Joel Brobecker]

> Additionally, I think the char buf; statement should read char *buf; in
> the patch.

Clearly lots of errors in the patch I sent. Can you apply the changes
you suggested, recompile to make sure it at least builds, and post
again here. Because there is no testing involved, I'd like more people
to have a chance to review it before it gets checked in.

Thank you,
-- 
Joel

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Thomas Schwinge]

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

Hi!

On Mon, 27 May 2013 15:48:20 +0200, Svante Signell <svante.signell@gmail.com> wrote:
> On Mon, 2013-05-27 at 14:21 +0200, Svante Signell wrote:
> > On Mon, 2013-05-27 at 16:10 +0400, Joel Brobecker wrote:
> > > > 2013-05-27  Svante Signell  <srs@hurd-2013.my.own.domain>
> > > > 
> > > > 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> > > > 	problems for systems not defining PATH_MAX by using xstrprintf and
> > > > 	a cleanup.

> > > While looking at this code, I don't think you'll need 2 xstrprintf
> > > either. Can you try the attached patch?

> I had to change the second make_cleanup call to do_cleanups instead, see
> the updated patch.

Right.

> 2013-05-27  Svante Signell  <svante.signell@gmail.com>
> 
> 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> 	problems for systems not defining PATH_MAX by using xstrprintf
> 	followed by a cleanup.

GNU ChangeLogs are to describe what is changed, not why, so I'd use
something like: »Replace xsnprintf usage with fixed-size buffers with
xstrprintf«.

> diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
> index 748869f..cf4fd60 100644
> --- a/gdb/nto-tdep.c
> +++ b/gdb/nto-tdep.c
> @@ -147,9 +147,10 @@ nto_find_and_open_solib (char *solib, unsigned o_flags, char **temp_pathname)
>  void
>  nto_init_solib_absolute_prefix (void)
>  {
> -  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
> +  char buf;

»char *buf«, as you already figured out.

>    char *nto_root, *endian;
>    const char *arch;
> +  struct cleanup *old_chain;
>  
>    nto_root = nto_target ();
>    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> @@ -172,10 +173,11 @@ nto_init_solib_absolute_prefix (void)
>  	       == BFD_ENDIAN_BIG ? "be" : "le";
>      }
>  
> -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
> -
> -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
> +  buf = xstrprintf ("set solib-absolute-prefix \"%s/%s%s\"",
> +		    nto_root, arch, endian);

The original code does not put double-quotes around the (former)
arch_path argument to »set solib-absolute-prefix«.  Do we have to be
concerned here about quoting/path names containing double-quotes et al.?

> +  old_chain = make_cleanup (xfree, buf);
>    execute_command (buf, 0);
> +  do_cleanups (old_chain);
>  }


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

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

On Mon, 2013-05-27 at 18:30 +0400, Joel Brobecker wrote:
> > Additionally, I think the char buf; statement should read char *buf; in
> > the patch.
> 
> Clearly lots of errors in the patch I sent. Can you apply the changes
> you suggested, recompile to make sure it at least builds, and post
> again here. Because there is no testing involved, I'd like more people
> to have a chance to review it before it gets checked in.
> 
> Thank you,

Ok, updated patch and ChangeLog entry attached.


[-- Attachment #2: ChangeLog.solve_PATH_MAX_issue --]
[-- Type: text/plain, Size: 206 bytes --]

2013-05-27  Svante Signell  <svante.signell@gmail.com>

	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
	problems for systems not defining PATH_MAX by using xstrprintf
	followed by a cleanup.



[-- Attachment #3: solve_PATH_MAX_issue.patch --]
[-- Type: text/x-patch, Size: 922 bytes --]

--- a/gdb/nto-tdep.c
+++ b/gdb/nto-tdep.c
@@ -147,9 +147,10 @@ nto_find_and_open_solib (char *solib, unsigned o_flags, char **temp_pathname)
 void
 nto_init_solib_absolute_prefix (void)
 {
-  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
+  char *buf;
   char *nto_root, *endian;
   const char *arch;
+  struct cleanup *old_chain;
 
   nto_root = nto_target ();
   if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
@@ -172,10 +173,11 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
-
-  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
+  buf = xstrprintf ("set solib-absolute-prefix \"%s/%s%s\"",
+		    nto_root, arch, endian);
+  old_chain = make_cleanup (xfree, buf);
   execute_command (buf, 0);
+  do_cleanups (old_chain);
 }
 
 char **

New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

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

On Mon, 2013-05-27 at 18:30 +0400, Joel Brobecker wrote:

> Clearly lots of errors in the patch I sent. Can you apply the changes
> you suggested, recompile to make sure it at least builds, and post
> again here. Because there is no testing involved, I'd like more people
> to have a chance to review it before it gets checked in.
> 
> Thank you,

Note: Separate patch for another function: find_and_open_solib as
requested.

Attaching a patch and Chengelog entry for the function
nto_find_and_open_solib preceding nto_init_solib_absolute_prefix. There
I did not see any use of the make_cleanup/do_cleanups functions
(assuming open and openp always return). Therefore xfree is used
directly. (Maybe line break of the first xsnprintf statement is not
correctly placed, what does GCS say here?)

[-- Attachment #2: ChangeLog.use_xstrprintf --]
[-- Type: text/plain, Size: 219 bytes --]

2013-05-27  Svante Signell  <svante.signell@gmail.com>

	* nto-dep.c (nto_find_and_open_solib): Use xstrprintf followed by
	a cleanup instead of alloca to be consistent with function
	(nto_init_solib_absolute_prefix).


[-- Attachment #3: use_xstrprintf.patch --]
[-- Type: text/x-patch, Size: 1663 bytes --]

--- a/gdb/nto-tdep.c	2013-05-27 15:58:07.000000000 +0200
+++ b/gdb/nto-tdep.c	2013-05-27 16:23:50.000000000 +0200
@@ -89,9 +89,7 @@ nto_find_and_open_solib (char *solib, un
   char *buf, *arch_path, *nto_root, *endian;
   const char *base;
   const char *arch;
-  int arch_len, len, ret;
-#define PATH_FMT \
-  "%s/lib:%s/usr/lib:%s/usr/photon/lib:%s/usr/photon/dll:%s/lib/dll"
+  int ret;
 
   nto_root = nto_target ();
   if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
@@ -117,22 +115,20 @@ nto_find_and_open_solib (char *solib, un
   /* In case nto_root is short, add strlen(solib)
      so we can reuse arch_path below.  */
 
-  arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
-	      + strlen (solib));
-  arch_path = alloca (arch_len);
-  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
+  arch_path = xstrprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
 
-  len = strlen (PATH_FMT) + strlen (arch_path) * 5 + 1;
-  buf = alloca (len);
-  xsnprintf (buf, len, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
+  buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib:\"%s\"/usr/photon/dll:\"%s\"/lib/dll", arch_path, arch_path, arch_path, arch_path,
 	     arch_path);
+  xfree (arch_path);
 
   base = lbasename (solib);
   ret = openp (buf, 1, base, o_flags, temp_pathname);
+  xfree (buf);
   if (ret < 0 && base != solib)
     {
-      xsnprintf (arch_path, arch_len, "/%s", solib);
+      arch_path = xstrprintf ("/%s", solib);
       ret = open (arch_path, o_flags, 0);
+      xfree (arch_path);
       if (temp_pathname)
 	{
 	  if (ret >= 0)

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Joel Brobecker]

> 2013-05-27  Svante Signell  <svante.signell@gmail.com>
> 
> 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> 	problems for systems not defining PATH_MAX by using xstrprintf
> 	followed by a cleanup.

As noticed by Thomas, he's right that we are introducing extra
quoting of the command's argument, and I don't think we should be
doing that in this patch. We can discuss the need to do so, but
it should be done separately.

Other than that, the patch looks good to me. Just give it a few more
days (say until Monday) before we commit the patch. Do you have an
FSF copyright assignment on file?

> --- a/gdb/nto-tdep.c
> +++ b/gdb/nto-tdep.c
> @@ -147,9 +147,10 @@ nto_find_and_open_solib (char *solib, unsigned o_flags, char **temp_pathname)
>  void
>  nto_init_solib_absolute_prefix (void)
>  {
> -  char buf[PATH_MAX * 2], arch_path[PATH_MAX];
> +  char *buf;
>    char *nto_root, *endian;
>    const char *arch;
> +  struct cleanup *old_chain;
>  
>    nto_root = nto_target ();
>    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> @@ -172,10 +173,11 @@ nto_init_solib_absolute_prefix (void)
>  	       == BFD_ENDIAN_BIG ? "be" : "le";
>      }
>  
> -  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
> -
> -  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
> +  buf = xstrprintf ("set solib-absolute-prefix \"%s/%s%s\"",
> +		    nto_root, arch, endian);
> +  old_chain = make_cleanup (xfree, buf);
>    execute_command (buf, 0);
> +  do_cleanups (old_chain);
>  }
>  
>  char **


-- 
Joel

Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Joel Brobecker]

> Attaching a patch and Chengelog entry for the function
> nto_find_and_open_solib preceding nto_init_solib_absolute_prefix. There
> I did not see any use of the make_cleanup/do_cleanups functions
> (assuming open and openp always return). Therefore xfree is used
> directly.

In this case, I would KISS, and go with cleanups.  Otherwise,
you have to remember when you can free. With cleanups, you just
mark it right after allocation and do_cleanups before returning.
And we are putting more and more tools in place to make sure we
do not forget to do the cleanups...

> 2013-05-27  Svante Signell  <svante.signell@gmail.com>
> 
> 	* nto-dep.c (nto_find_and_open_solib): Use xstrprintf followed by
> 	a cleanup instead of alloca to be consistent with function
> 	(nto_init_solib_absolute_prefix).

My comments below...

> --- a/gdb/nto-tdep.c	2013-05-27 15:58:07.000000000 +0200
> +++ b/gdb/nto-tdep.c	2013-05-27 16:23:50.000000000 +0200
> @@ -89,9 +89,7 @@ nto_find_and_open_solib (char *solib, un
>    char *buf, *arch_path, *nto_root, *endian;
>    const char *base;
>    const char *arch;
> -  int arch_len, len, ret;
> -#define PATH_FMT \
> -  "%s/lib:%s/usr/lib:%s/usr/photon/lib:%s/usr/photon/dll:%s/lib/dll"
> +  int ret;
>  
>    nto_root = nto_target ();
>    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> @@ -117,22 +115,20 @@ nto_find_and_open_solib (char *solib, un
>    /* In case nto_root is short, add strlen(solib)
>       so we can reuse arch_path below.  */
>  
> -  arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
> -	      + strlen (solib));
> -  arch_path = alloca (arch_len);
> -  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
> +  arch_path = xstrprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);

You forgot to remove "arch_path, arch_len" from the arguments of
the call above. As a result, i suspect that this patch does not
compile, and that's the minimum we need to do...

>  
> -  len = strlen (PATH_FMT) + strlen (arch_path) * 5 + 1;
> -  buf = alloca (len);
> -  xsnprintf (buf, len, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
> +  buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib:\"%s\"/usr/photon/dll:\"%s\"/lib/dll", arch_path, arch_path, arch_path, arch_path,
>  	     arch_path);

Same as in the other patch in the same file: Your patch is adding
double-quoting of the arg_path, which we may want if you have
a compelling argument, but should be discussed independently of
this patch.

I do not know of any absolute guideline whether to split the format
string or not, but I probably would, like so:

  buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib"
                      ":\"%s\"/usr/photon/dll:\"%s\"/lib/dll",
                    arch_path, arch_path, arch_path, arch_path,
                    arch_path);

Be sure, also, to try to restrict your line length to 70 characters if
you can. That's our soft limit, with 80 being the absolute limit (under
normal circumstances).

> +  xfree (arch_path);
>  
>    base = lbasename (solib);
>    ret = openp (buf, 1, base, o_flags, temp_pathname);
> +  xfree (buf);
>    if (ret < 0 && base != solib)
>      {
> -      xsnprintf (arch_path, arch_len, "/%s", solib);
> +      arch_path = xstrprintf ("/%s", solib);
>        ret = open (arch_path, o_flags, 0);
> +      xfree (arch_path);
>        if (temp_pathname)
>  	{
>  	  if (ret >= 0)


-- 
Joel

Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

On Wed, 2013-05-29 at 13:26 +0400, Joel Brobecker wrote:
> > 2013-05-27  Svante Signell  <svante.signell@gmail.com>
> > 
> > 	* nto-tdep.c (nto_init_solib_absolute_prefix): Solve build
> > 	problems for systems not defining PATH_MAX by using xstrprintf
> > 	followed by a cleanup.
> 
> As noticed by Thomas, he's right that we are introducing extra
> quoting of the command's argument, and I don't think we should be
> doing that in this patch. We can discuss the need to do so, but
> it should be done separately.
> 
> Other than that, the patch looks good to me. Just give it a few more
> days (say until Monday) before we commit the patch. Do you have an
> FSF copyright assignment on file?

Not yet, I'll send a filled-in form in a few days.

Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell]

On Wed, 2013-05-29 at 13:42 +0400, Joel Brobecker wrote:
> > Attaching a patch and Chengelog entry for the function
> > nto_find_and_open_solib preceding nto_init_solib_absolute_prefix. There
> > I did not see any use of the make_cleanup/do_cleanups functions
> > (assuming open and openp always return). Therefore xfree is used
> > directly.
> 
> In this case, I would KISS, and go with cleanups.  Otherwise,
> you have to remember when you can free. With cleanups, you just
> mark it right after allocation and do_cleanups before returning.
> And we are putting more and more tools in place to make sure we
> do not forget to do the cleanups...

OK, I will use the *_cleanup(s) functions instead.

> > 2013-05-27  Svante Signell  <svante.signell@gmail.com>
> > 
> > 	* nto-dep.c (nto_find_and_open_solib): Use xstrprintf followed by
> > 	a cleanup instead of alloca to be consistent with function
> > 	(nto_init_solib_absolute_prefix).
> 
> My comments below...
> 
> > --- a/gdb/nto-tdep.c	2013-05-27 15:58:07.000000000 +0200
> > +++ b/gdb/nto-tdep.c	2013-05-27 16:23:50.000000000 +0200
> > @@ -89,9 +89,7 @@ nto_find_and_open_solib (char *solib, un
> >    char *buf, *arch_path, *nto_root, *endian;
> >    const char *base;
> >    const char *arch;
> > -  int arch_len, len, ret;
> > -#define PATH_FMT \
> > -  "%s/lib:%s/usr/lib:%s/usr/photon/lib:%s/usr/photon/dll:%s/lib/dll"
> > +  int ret;
> >  
> >    nto_root = nto_target ();
> >    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> > @@ -117,22 +115,20 @@ nto_find_and_open_solib (char *solib, un
> >    /* In case nto_root is short, add strlen(solib)
> >       so we can reuse arch_path below.  */
> >  
> > -  arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
> > -	      + strlen (solib));
> > -  arch_path = alloca (arch_len);
> > -  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
> > +  arch_path = xstrprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
> 
> You forgot to remove "arch_path, arch_len" from the arguments of
> the call above. As a result, i suspect that this patch does not
> compile, and that's the minimum we need to do...

I have updated my patch and it built. I copied the patch to a directory,
attached it to the mail, and checked by opening the attachment with
gedit. Then updated the patch to that directory and rechecked with gedit
again and the updated patch was there (not deleting the old attachment)
Still the old version was attached :( Maybe an evolution issue.

> >  
> > -  len = strlen (PATH_FMT) + strlen (arch_path) * 5 + 1;
> > -  buf = alloca (len);
> > -  xsnprintf (buf, len, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
> > +  buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib:\"%s\"/usr/photon/dll:\"%s\"/lib/dll", arch_path, arch_path, arch_path, arch_path,
> >  	     arch_path);
> 
> Same as in the other patch in the same file: Your patch is adding
> double-quoting of the arg_path, which we may want if you have
> a compelling argument, but should be discussed independently of
> this patch.

OK, an updated patch will remove these quotes.

> I do not know of any absolute guideline whether to split the format
> string or not, but I probably would, like so:
> 
>   buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib"
>                       ":\"%s\"/usr/photon/dll:\"%s\"/lib/dll",
>                     arch_path, arch_path, arch_path, arch_path,
>                     arch_path);
> 
> Be sure, also, to try to restrict your line length to 70 characters if
> you can. That's our soft limit, with 80 being the absolute limit (under
> normal circumstances).

OK, will do.

> > +  xfree (arch_path);
> >  
> >    base = lbasename (solib);
> >    ret = openp (buf, 1, base, o_flags, temp_pathname);
> > +  xfree (buf);
> >    if (ret < 0 && base != solib)
> >      {
> > -      xsnprintf (arch_path, arch_len, "/%s", solib);
> > +      arch_path = xstrprintf ("/%s", solib);
> >        ret = open (arch_path, o_flags, 0);
> > +      xfree (arch_path);
> >        if (temp_pathname)
> >  	{
> >  	  if (ret >= 0)
> 
>