Re: [PATCH] Make global file function names link unique

[Geoffrey Keating <geoffk at apple dot com>]

On 18/12/2006, at 1:26 PM, John David Anglin wrote:

> > If you can't guarantee that any name is unique to this file, you need
> > to fall down to the last case in this function, where it already does
> > all kinds of crc32 operations.  If you're going to do that, or do
> > something like what you have in the patch below, please only do it on
> > targets that actually need it.
>
> The enclosed version only adds the crc32 for 'I' and 'D' when the
> target doesn't handle constructors/destructors (i.e., it uses  
> collect2).
> I believe that adding the crc32 is necessary in all cases when 'F'
> and 'N' are used.
>
> Adding randomness based on flag_random_seed causes the assembly
> output to change from one compilation to another.  This creates
> several new testsuite regressions that I would prefer to avoid.

I agree that you should avoid testsuite regressions, but not at the  
cost of introducing actual bugs!  It would be better to just fix the  
testsuite, for example by having it pass -frandom-seed.

> Also note that the last case uses different names for the crc32
> string.
>
> Aside from changes to comments and the 'if', the patch is identical
> to the one approved here:
> http://gcc.gnu.org/ml/gcc-patches/2006-01/msg00804.html
> and subsequently removed here:
> http://gcc.gnu.org/ml/gcc-patches/2006-11/msg00003.html
>
> Tested on hppa2.0-hp-hpux11.11 and hppa-unknown-linux-gnu.
>
> OK?

The property of first_global_object_name is that  
first_global_object_name is set to this value only for this  
compilation in the whole link.  If this is not happening on HP-UX, it  
would be better to fix that rather than work around it.  For example,  
in the first patch you quoted, this happens "because ... overrides  
new", so perhaps an override of new should not be placed in  
first_global_object_name (although I do not see why this causes a  
problem; there can be only one override of new in a whole program).   
This is the only purpose of first_global_object_name, so if this can  
not be made to work then first_global_object_name is useless and  
should be removed.

I think you need to add a comment explaining what's really going on in  
detail.  Even after your reading the URLs you quoted, reading the  
original PR, and reading your mail about this patch, I do not  
understand why there is a problem and am very suspicious that you are  
merely working around a problem that is really somewhere else.

Certainly if you do not add a comment you can expect that at some  
point in the future this patch will be removed just like last time,  
because no-one will understand why it is necessary.

> Dave
> --
> J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
> National Research Council of Canada              (613) 990-0752  
> (FAX: 952-6602)
>
> 2006-12-18  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
>
> 	PR middle-end/30151
> 	PR middle-end/30174
> 	* tree.c (get_file_function_name): Make global names link unique.
>
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 119963)
> +++ tree.c	(working copy)
> @@ -6283,7 +6283,7 @@
>        *p = '_';
>  }
>
> -/* Generate a name for a special-purpose function function.
> +/* Generate a name for a special-purpose function.
>     The generated name may need to be unique across the whole link.
>     TYPE is some string to identify the purpose of this function to  
> the
>     linker or collect2; it must start with an uppercase letter,
> @@ -6300,9 +6300,29 @@
>    const char *p;
>    char *q;
>
> -  /* If we already have a name we know to be unique, just use  
> that.  */
> +  /* If we already have a name we know to be unique to this  
> translation
> +     unit, just use it.  */
>    if (first_global_object_name)
> -    p = first_global_object_name;
> +    {
> +      p = first_global_object_name;
> +
> +      /* The generated name needs to be unique across the whole  
> link for
> +	 types 'I' and 'D' when constructors/destructors are not handled
> +	 by the target, and for types 'F' and 'N'.  */
> +      if ((type[0] != 'I' && type[0] != 'D') || ! 
> targetm.have_ctors_dtors)
> +	{
> +	  const char *file = main_input_filename;
> +
> +	  if (!file)
> +	    file = input_filename;
> +
> +	  /* Use the CRC32 string for file, because the full pathname
> +	     might be quite long.  */
> +	  q = alloca (strlen (p) + 10);
> +	  sprintf (q, "%s_%08X", p, crc32_string (0, file));
> +	  p = q;
> +	}
> +    }
>    /* If the target is handling the constructors/destructors, they
>       will be local to this file and the name is only necessary for
>       debugging purposes.  */

Attachment: smime.p7s
Description: S/MIME cryptographic signature