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

[Nick Clifton <nickc@redhat.com>]

Hi Jan, Hi Mark,

> 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):

Normally I would only be worried about excess casting if it is likely
to obscure a problem.  Unnecessary casting might be niggling from a
readability point of view, but I would not normally consider it to be
a reason to reject a patch.  Dangerous casting is another matter though...


>> +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.

Agreed - the casts are not needed here.

> Personally I view _any_ cast
> as latently dangerous, and hence I'd prefer if casts were used only
> if there's no other option.

Well I think casts from a void type to something else are usually reasonable,
But otherwise I would agree that they are often suspicious.


On a related note - I would consider this line to be problematic:

   sc_in = xmalloc (num_sc * sizeof (struct in_sc));

The code here implies that "sc_in" is a pointer to the "struct in_sc" type.
If at some future date the code is changed and the type of "sc_in" is changed
then the above line will still work, but the wrong amount of space will be
allocated.  So I would suggest changing it to either:

   sc_in = xmalloc (num_sc * sizeof (* sc_in));

Or:

   sc_in = xmalloc (num_sc * sizeof * sc_in);  /* I like this version, but nobody else does ... :-) */

Or:

   sc_in = XNEWVEC (typeof (sc_in), num_sc);


>> +  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?

The code certainly is messy.  I do not like the implicit casting of a void
pointer to a structure pointer, so personally I would keep the assignment
cast and try to eliminate the cast of *data by using an intermediary variable:

   uint32_t * ptr = * data;
   sc = (struct section_contribution *) (ptr + 1); /* Skip the version word.  */

Even this looks wrong to me, since it assumes that it is OK to cast a 4 byte
aligned pointer to a structure pointer, with no guarantee that the structure
does not need a larger alignment.  I get that the code is computing the
contents of a section which starts with a version number followed by a set
of filled in objects, and that this is hard to express cleanly in C.  But still
I would not be surprised if a static analysis tool flagged this code as a
potential problem.

Cheers
   Nick