Fix HP_SMALL_TIMING_AVAIL undef warnings

  Here is my attempt to fix some of the undef warnings (those for
HP_SMALL_TIMING_AVAIL).  Does this look like the right way to
fix this?  At the risk of opening a can of worms, do we want to
change the name of this macro to match the other HP_TIMING_*
macros in this section?

I tested this on my MIPS machine and all the object files remained
the same except for nscd/nscd_stat.o (has a date stamp in it) and
rt/get_clockfreq.o (line number change in debug info due to the extra
line in sysdeps/generic/hp-timing.h).

It might be good if an alpha maintainer could check this since
that is the only target that defines HP_SMALL_TIMING_AVAIL.

OK to check in?


2014-04-25  Steve Ellcey  <sellcey@mips.com>

	* sysdeps/generic/hp-timing.h (HP_SMALL_TIMING_AVAIL):
	Set default value.
	* elf/dl-support.c: Use #if to check HP_SMALL_TIMING_AVAIL.
	* elf/rtld.c: Ditto.
  

On 25 April 2014 23:53, Steve Ellcey <sellcey@mips.com> wrote:
>
> Here is my attempt to fix some of the undef warnings (those for
> HP_SMALL_TIMING_AVAIL).  Does this look like the right way to
> fix this?  At the risk of opening a can of worms, do we want to
> change the name of this macro to match the other HP_TIMING_*
> macros in this section?
>
> I tested this on my MIPS machine and all the object files remained
> the same except for nscd/nscd_stat.o (has a date stamp in it) and
> rt/get_clockfreq.o (line number change in debug info due to the extra
> line in sysdeps/generic/hp-timing.h).
>
> It might be good if an alpha maintainer could check this since
> that is the only target that defines HP_SMALL_TIMING_AVAIL.
>
> OK to check in?
>
>
> 2014-04-25  Steve Ellcey  <sellcey@mips.com>
>
>         * sysdeps/generic/hp-timing.h (HP_SMALL_TIMING_AVAIL):
>         Set default value.
>         * elf/dl-support.c: Use #if to check HP_SMALL_TIMING_AVAIL.
>         * elf/rtld.c: Ditto.
>
>
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index e435436..e9cfa64 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -130,7 +130,7 @@ void *_dl_random;
>  #include <dl-procinfo.c>
>
>  /* We expect less than a second for relocation.  */
> -#ifdef HP_SMALL_TIMING_AVAIL
> +#if HP_SMALL_TIMING_AVAIL
>  # undef HP_TIMING_AVAIL
>  # define HP_TIMING_AVAIL HP_SMALL_TIMING_AVAIL
>  #endif
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 9d121dc..5dcb61a 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -196,7 +196,7 @@ static struct libname_list _dl_rtld_libname;
>  static struct libname_list _dl_rtld_libname2;
>
>  /* We expect less than a second for relocation.  */
> -#ifdef HP_SMALL_TIMING_AVAIL
> +#if HP_SMALL_TIMING_AVAIL
>  # undef HP_TIMING_AVAIL
>  # define HP_TIMING_AVAIL HP_SMALL_TIMING_AVAIL
>  #endif
> diff --git a/sysdeps/generic/hp-timing.h b/sysdeps/generic/hp-timing.h
> index eddc971..775495d 100644
> --- a/sysdeps/generic/hp-timing.h
> +++ b/sysdeps/generic/hp-timing.h
> @@ -30,6 +30,8 @@
>
>     - HP_TIMING_AVAIL: test for availability.
>
> +   - HP_SMALL_TIMING_AVAIL: test for availability.
> +
>     - HP_TIMING_INLINE: this macro is non-zero if the functionality is not
>       implemented using function calls but instead uses some inlined code
>       which might simply consist of a few assembler instructions.  We have to
> @@ -66,6 +68,7 @@
>
>  /* Provide dummy definitions.  */
>  #define HP_TIMING_AVAIL                (0)
> +#define HP_SMALL_TIMING_AVAIL   (0)
>  #define HP_TIMING_INLINE       (0)
>  typedef int hp_timing_t;
>  #define HP_TIMING_ZERO(Var)

This looks like it is the right thing to do, however I believe the
warnings will persist on:

ia64
x86_64
i386
powerpc32
powerpc64
sparc32
sparc64

Which all use their own hp-timing.h.
  

On Mon, 28 Apr 2014, Will Newton wrote:

> This looks like it is the right thing to do, however I believe the
> warnings will persist on:
> 
> ia64
> x86_64
> i386
> powerpc32
> powerpc64
> sparc32
> sparc64
> 
> Which all use their own hp-timing.h.

Indeed.  For such a patch you need to:

* Work out what header should define the macro.

* Ensure that it does define the macro, for all configurations of glibc.

* Go through all uses of the macro that test #ifdef / #ifndef / #if 
defined / #elif defined, and change them to use #if conditionals.  Make 
sure that all the users include the relevant header before the tests.  If 
some don't include the header, there may be pre-existing bugs from testing 
whether a macro is defined without including the header that defines it - 
in which case you need (a) to work out whether the test in the file is 
actually correct, or if it is bitrotten, and (b) to call out these cases 
in the patch submission, giving your analysis of whether the test is 
actually correct or not.
  

On Mon, 2014-04-28 at 14:53 +0000, Joseph S. Myers wrote:
> On Mon, 28 Apr 2014, Will Newton wrote:
> 
> > This looks like it is the right thing to do, however I believe the
> > warnings will persist on:
> > 
> > ia64
> > x86_64
> > i386
> > powerpc32
> > powerpc64
> > sparc32
> > sparc64
> > 
> > Which all use their own hp-timing.h.
> 
> Indeed.  For such a patch you need to:

It seems a bit ugly to have to define 
HP_SMALL_TIMING_AVAIL as 0 in 8 different hp-timing.h header files.
Would the idea of each of these hp-timing.h header files including the
default hp-timing.h header file and then only overriding the macros they
need to redefine be considered acceptable?  I see a few uses of
"#include_next" in glibc but not in any header files where a platform is
redefining a generic header.  Those all seem to be complete
replacements.  How about a new header file of default macro definitions
(not necessarily just the timing macros)?  Is that something that could
be considered to provide default values for macros?  Or has that idea
already been dismissed?

Steve Ellcey
sellcey@mips.com