Re: [PATCH v3] ld: Make archive member file extension comparisons case insensitive when cross compiling too

[Jan Beulich <jbeulich@suse.com>]

On 23.08.2022 23:11, Martin Storsjö wrote:
> On Windows, filename_cmp is case insensitive, but when cross compiling
> with libraries that may contain members with uppercase file names, we
> should keep those comparisons case insensitive when running the build
> tools on other OSes too.
> ---
> v3: Renamed the function to fileext_cmp as suggested. (This also fixes
> building on Windows, where there already existed a non-static
> function named stricmp.)

I was actually surprised you did get away with using the earlier name;
I did have the potential name collision in mind, but didn't spell it
out because there was an (imo) more relevant reason for the rename.

> --- a/ld/emultempl/pe.em
> +++ b/ld/emultempl/pe.em
> @@ -171,6 +171,23 @@ static int is_underscoring (void)
>    return pe_leading_underscore;
>  }
>  
> +/* A case insensitive comparison, regardless of the host platform,
> +   used for comparing file extensions. */
> +static int fileext_cmp (const char *s1, const char *s2)
> +{
> +  for (;;)
> +    {
> +      int c1 = TOLOWER (*s1++);
> +      int c2 = TOLOWER (*s2++);
> +
> +      if (c1 != c2)
> +        return (c1 - c2);
> +
> +      if (c1 == '\0')
> +        return 0;
> +    }
> +}

I'm not going to insist, but since you're touching all involved call
sites anyway, two suggestions:

1) Don't convert the 2nd string to lower case, but instead require
that this always be lower case when passed in. The function isn't a
generic string comparison one anymore with the name change, after
all.

2) Omit the leading . in the string literals passed, incrementing the
first argument to compensate. Besides saving a little bit of space
and work, the function name now also carries in its name what is
being done, so the leading .-s don't really carry that much extra
information to the reader anymore.

Jan

> @@ -1666,7 +1683,7 @@ gld${EMULATION_NAME}_after_open (void)
>  		       extension, and use that for the remainder of the
>  		       comparisons.  */
>  		    pnt = strrchr (bfd_get_filename (is3->the_bfd), '.');
> -		    if (pnt != NULL && filename_cmp (pnt, ".dll") == 0)
> +		    if (pnt != NULL && fileext_cmp (pnt, ".dll") == 0)
>  		      break;
>  		  }
>  
> @@ -1683,7 +1700,7 @@ gld${EMULATION_NAME}_after_open (void)
>  			/* Skip static members, ie anything with a .obj
>  			   extension.  */
>  			pnt = strrchr (bfd_get_filename (is2->the_bfd), '.');
> -			if (pnt != NULL && filename_cmp (pnt, ".obj") == 0)
> +			if (pnt != NULL && fileext_cmp (pnt, ".obj") == 0)
>  			  continue;
>  
>  			if (filename_cmp (bfd_get_filename (is3->the_bfd),
> @@ -1701,7 +1718,7 @@ gld${EMULATION_NAME}_after_open (void)
>  	       then leave the filename alone.  */
>  	    pnt = strrchr (bfd_get_filename (is->the_bfd), '.');
>  
> -	    if (is_ms_arch && (filename_cmp (pnt, ".dll") == 0))
> +	    if (is_ms_arch && (fileext_cmp (pnt, ".dll") == 0))
>  	      {
>  		int idata2 = 0, reloc_count=0;
>  		asection *sec;
> @@ -1857,7 +1874,7 @@ gld${EMULATION_NAME}_unrecognized_file (lang_input_statement_type *entry ATTRIBU
>  #ifdef DLL_SUPPORT
>    const char *ext = entry->filename + strlen (entry->filename) - 4;
>  
> -  if (filename_cmp (ext, ".def") == 0 || filename_cmp (ext, ".DEF") == 0)
> +  if (fileext_cmp (ext, ".def") == 0)
>      {
>        pe_def_file = def_file_parse (entry->filename, pe_def_file);
>