Ping on HP-UX patch for a clean build

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Ping on HP-UX patch for a clean build

Steve Ellcey
I am still looking for approval and checkin of the following patch:

http://sourceware.org/ml/binutils/2005-05/msg00218.html

There was some follow-up to this patch suggesting I provide the
definition of getc_unlocked when one wasn't found.  I explained why I
didn't think that was the right thing to do.

John David Anglin also found this problem on hppa2.0w-hp-hpux11.11,
See http://sourceware.org/ml/binutils/2005-05/msg00638.html.

Steve Ellcey
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

James E Wilson
On Wed, 2005-06-01 at 11:39, Steve Ellcey wrote:
> I am still looking for approval and checkin of the following patch:
> http://sourceware.org/ml/binutils/2005-05/msg00218.html

I'd been meaning to to check my POSIX manuals at home for info on the
*_unlocked but kept forgetting to do so.  Turns out that my 15 year old
copy of POSIX.1 doesn't have them.  Looks like I have to buy a new set
of manuals.  Meanwhile, all I know is what I've gleaned from reading the
linux man pages and the glibc sources.

Anyways, I believe I understand what is going on here.  glibc is thread
safe by default, so single threaded apps should use the *_unlocked
functions to improve performance.  HPUX is thread safe only if
-D_REENTRANT, so there is no need to use the *_unlocked functions for
performance reasons.  There is also the issue that it might possibly be
unsafe to use it in this case.  I haven't seen any evidence to support
that, but I have incomplete docs, and since it isn't doing anything
useful here, there is no need to take the risk.

The check you are adding for NEED_DECLARATION_GETC_UNLOCKED in strings.c
is unlike anything else in binutils, but does actually make sense in
this case, and is roughly equivalent to the corresponding gcc code.  It
really should have a comment explaining why we are doing this though.
Maybe something like
  /* Only use getc_unlocked if we found a declaration for it.
     Otherwise, libc is not thread safe by default, and we should not
     use it.  This should test HAVE_DECL_GETC_UNLOCKED if configure
     used the standard autoconf AC_CHECK_DECLS test.  */

One thing that isn't clear to me is why you just didn't add the
AC_CHECK_DECLS test in the first place.  Sure, the existing configure.in
file has out of date BFD_NEED_DECLARATION checks, but that shouldn't
prevent you from handling getc_unlocked the correct way via
AC_CHECK_DECLS, unless I missed something.  You can leave the problem of
fixing the rest of the BFD_NEED_DECLARATION checks to someone else.
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

Steve Ellcey
> From: James E Wilson <[hidden email]>

> Maybe something like
>   /* Only use getc_unlocked if we found a declaration for it.
>      Otherwise, libc is not thread safe by default, and we should not
>      use it.  This should test HAVE_DECL_GETC_UNLOCKED if configure
>      used the standard autoconf AC_CHECK_DECLS test.  */

Sounds reasonable.

> One thing that isn't clear to me is why you just didn't add the
> AC_CHECK_DECLS test in the first place.  Sure, the existing configure.in
> file has out of date BFD_NEED_DECLARATION checks, but that shouldn't
> prevent you from handling getc_unlocked the correct way via
> AC_CHECK_DECLS, unless I missed something.  You can leave the problem of
> fixing the rest of the BFD_NEED_DECLARATION checks to someone else.

The configure I have for the binutils subdirectory was built with
autoconf 2.13.  I am pretty sure I tried using AC_CHECK_DECLS with
autoconf 2.13 and it did not work (wasn't recognized).  When we start
requiring autoconf 2.57 or later then we can use AC_CHECK_DECLS.

Steve Ellcey
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

James E Wilson
On Thu, 2005-06-02 at 14:58, Steve Ellcey wrote:
> The configure I have for the binutils subdirectory was built with
> autoconf 2.13.  I am pretty sure I tried using AC_CHECK_DECLS with
> autoconf 2.13 and it did not work (wasn't recognized).  When we start
> requiring autoconf 2.57 or later then we can use AC_CHECK_DECLS.

binutils/configure.in has at the top
        AC_PREREQ(2.57)
This happened May 15, which was after your original patch.  Maybe using
AC_CHECK_DECLS would work now?
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

Steve Ellcey
> binutils/configure.in has at the top
> AC_PREREQ(2.57)
> This happened May 15, which was after your original patch.  Maybe using
> AC_CHECK_DECLS would work now?
> --
> Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Yes, this was updated after my patch, so AC_CHECK_DECLS should work now.
Would you like me to update my patch and resubmit it or do you just
want to tweak it?

Steve Ellcey
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

James E Wilson
On Thu, 2005-06-02 at 15:35, Steve Ellcey wrote:
> Yes, this was updated after my patch, so AC_CHECK_DECLS should work now.
> Would you like me to update my patch and resubmit it or do you just
> want to tweak it?

I don't have any HPUX machine to test on, so it is easier for me if you
do this.
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

Steve Ellcey
> On Thu, 2005-06-02 at 15:35, Steve Ellcey wrote:
> > Yes, this was updated after my patch, so AC_CHECK_DECLS should work now.
> > Would you like me to update my patch and resubmit it or do you just
> > want to tweak it?
>
> I don't have any HPUX machine to test on, so it is easier for me if you
> do this.
> --
> Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Here is a new patch done with AC_CHECK_DECLS and using the latest sources.
I tested on IA64 HP-UX and IA64 Linux and had no unexpected failures on
either platform.  I also verified that IA64 Linux continued to use the
getc_unlocked function.

Steve Ellcey
[hidden email]


binutils/ChangeLog:

2005-06-03  Steve Ellcey  <[hidden email]>

        configure.in: Check for getc_unlocked prototype.
        configure: Regenerate
        config.in: Regenerate
        strings.c: Only call getc_unlocked if we have seen a prototype.


*** src.orig/binutils/configure.in Fri Jun  3 09:02:25 2005
--- src/binutils/configure.in Fri Jun  3 09:02:00 2005
*************** BFD_NEED_DECLARATION(strstr)
*** 217,222 ****
--- 217,223 ----
  BFD_NEED_DECLARATION(sbrk)
  BFD_NEED_DECLARATION(getenv)
  BFD_NEED_DECLARATION(environ)
+ AC_CHECK_DECLS(getc_unlocked)
 
  BFD_BINARY_FOPEN
 
*** src.orig/binutils/strings.c Fri Jun  3 09:02:17 2005
--- src/binutils/strings.c Fri Jun  3 09:06:14 2005
*************** get_char (FILE *stream, file_off *addres
*** 447,453 ****
  {
   if (stream == NULL)
     return EOF;
! #ifdef HAVE_GETC_UNLOCKED
   c = getc_unlocked (stream);
  #else
   c = getc (stream);
--- 447,458 ----
  {
   if (stream == NULL)
     return EOF;
!
!  /* Only use getc_unlocked if we found a declaration for it.
!     Otherwise, libc is not thread safe by default, and we
!     should not use it.  */
!
! #if defined(HAVE_GETC_UNLOCKED) && HAVE_DECL_GETC_UNLOCKED
   c = getc_unlocked (stream);
  #else
   c = getc (stream);
Reply | Threaded
Open this post in threaded view
|

Re: Ping on HP-UX patch for a clean build

James E Wilson
On Fri, 2005-06-03 at 09:11, Steve Ellcey wrote:
> configure.in: Check for getc_unlocked prototype.
> configure: Regenerate
> config.in: Regenerate
> strings.c: Only call getc_unlocked if we have seen a prototype.

OK.  I checked this in for you with a corrected ChangeLog entry.
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com