Re: [PATCH] ld: Write globals stream in PDB

[Nick Clifton <nickc@redhat.com>]

Hi Mark,

> Parses the .debug$S sections of object files for DEBUG_S_SYMBOLS
> subsections, creating a globals stream based on the records we find.
> These also get copied into the module stream for each object file.

A few comments on the patch ...

> +struct global
> +{
> +  struct global *next;
> +  uint32_t offset;
> +  uint32_t hash;
> +  uint32_t refcount;
> +  unsigned int index;
> +  uint8_t data[];
> +};

I dislike variable length arrays, espcially ones that do not have an
associated length field for bounds checking.  I understand the need,
but I would definitely advise being more paranoid...


> +/* Compare an entry in the string table with a string.  */
> +static int
> +eq_string_table_entry (const void *a, const void *b)

Is this really an "int" function ?  It looks like it returns a bool
to me.


> +/* Remap a type reference within a CodeView symbol.  */
> +static bool remap_symbol_type (void *data, struct type_entry **map,
> +			       uint32_t num_types)

Formatting - please put the function name at the start of the next line.


> +      switch (type)
> +	{
> +	case S_GPROC32:
> +	case S_LPROC32:
> +	  scope_level++;
> +	  break;
> +
> +	case S_END:
> +	case S_PROC_ID_END:
> +	  scope_level--;
> +
> +	  if (scope_level == 0)
> +	    return data;
> +
> +	  break;
> +	}

Please can you add a default case for this switch.  Probably with
an error result of some kind, unless other types are expected and
can be ignored.  (There are several other switch statements like
this too).


I also found that the patch no longer applies to today's sources, so if you
could rebase it that would be great.

Cheers
   Nick