Re: [PATCH] ld: Sort section contributions in PDB files

[Jan Beulich <jbeulich@suse.com>]

On 20.02.2023 15:13, Mark Harmstone wrote:
> Microsoft's DIA library, and thus also MSVC and WinDbg, expects section
> contributions to be ordered by section number and offset, otherwise it's
> unable to resolve line numbers.
> ---
>  ld/pdb.c                                  | 81 +++++++++++++++++------
>  ld/testsuite/ld-pe/pdb2-section-contrib.d |  6 +-
>  2 files changed, 65 insertions(+), 22 deletions(-)

In principle okay, but I'd like to re-raise the question of excess
casting (hence including Nick and Alan as the far more experienced
binutils maintainers):

> @@ -4118,6 +4125,27 @@ find_section_number (bfd *abfd, asection *sect)
>    return 0;
>  }
>  
> +/* Used as parameter to qsort, to sort section contributions by section and
> +   offset.  */
> +static int
> +section_contribs_compare (const void *p1, const void *p2)
> +{
> +  const struct in_sc *sc1 = (const struct in_sc *) p1;
> +  const struct in_sc *sc2 = (const struct in_sc *) p2;

In ANSI C there's no need for these casts; it may be that they were
needed in pre-ANSI dialects like K&R. Personally I view _any_ cast
as latently dangerous, and hence I'd prefer if casts were used only
if there's no other option.

> @@ -4177,32 +4209,43 @@ create_section_contrib_substream (bfd *abfd, void **data, uint32_t *size)
>      {
>        for (asection *s = in->sections; s; s = s->next)
>  	{
> -	  uint16_t sect_num;
> -
>  	  if (s->size == 0 || discarded_section (s))
>  	    continue;
>  
> -	  sect_num = find_section_number (abfd, s->output_section);
> -
> -	  memcpy (&sc->characteristics,
> -		  sect_flags + ((sect_num - 1) * sizeof (uint32_t)),
> -		  sizeof (uint32_t));
> -
> -	  bfd_putl16 (sect_num, &sc->section);
> -	  bfd_putl16 (0, &sc->padding1);
> -	  bfd_putl32 (s->output_offset, &sc->offset);
> -	  bfd_putl32 (s->size, &sc->size);
> -	  bfd_putl16 (mod_index, &sc->module_index);
> -	  bfd_putl16 (0, &sc->padding2);
> -	  bfd_putl32 (0, &sc->data_crc);
> -	  bfd_putl32 (0, &sc->reloc_crc);
> +	  sc2->s = s;
> +	  sc2->sect_num = find_section_number (abfd, s->output_section);
> +	  sc2->mod_index = mod_index;
>  
> -	  sc++;
> +	  sc2++;
>  	}
>  
>        mod_index++;
>      }
>  
> +  qsort (sc_in, num_sc, sizeof (struct in_sc), section_contribs_compare);
> +
> +  sc =
> +    (struct section_contribution *) ((uint8_t *) *data + sizeof (uint32_t));

This one's more interesting: Some cast is needed here at least as long as
we don't mean to allow use of GNU extensions (here: arithmetic on pointers
to void). But seeing that this causes a line length issue, at a minimum
I'd recommend to go with

  sc = (void *) ((uint8_t *) *data + sizeof (uint32_t));

(Ideally sc would be pointer-to-const and the cast here then also one to
pointer-to-const.)

Nick, Alan - thoughts?

Jan