Re: [patch pe-coff]: Add support for IAT directory field

[Dave Korn <dave dot korn dot cygwin at gmail dot com>]

On 20/09/2010 09:36, Kai Tietz wrote:
> PING
> 
> Regards,
> Kai

    Good morning Kai,

> +      				  "__IAT_start__", FALSE, FALSE, TRUE);

  Mixed spaces/TABs at the start of the line.

+	        pe_data
(abfd)->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE].VirtualAddress =

  Here, eight spaces after the first TAB should be a second TAB.

> +        }

  Also here at the end, eight spaces should be a tab.

> +        {
> +          if (! strncmp (secname, ".idata\$", 7))
> +            place = &hold[orphan_idata];
> +          else
> +            place = &hold[orphan_rodata];
> +	}

  Likewise here, formatting vs. spaces and TABs.  I'd probably be inclined to
just use a ternary operator here like

       else if ((s->flags & SEC_READONLY) == 0)
 	place = &hold[orphan_data];
       else if ((s->flags & SEC_CODE) == 0)
-	place = &hold[orphan_rodata];
+	place = ! strncmp (secname, ".idata\$", 7)
+		? &hold[orphan_idata]
+		: &hold[orphan_rodata];
       else
 	place = &hold[orphan_text];

that but it doesn't matter.  Formatting issues aside, I have one issue with
the actual code, in the way you treat results looked up in the hash table; for
example, here...

> +      h1 = coff_link_hash_lookup (coff_hash_table (info),
> +      				  "__IAT_start__", FALSE, FALSE, TRUE);
> +      if (h1 != NULL
> +	  && h1->root.u.def.section != NULL
> +	  && h1->root.u.def.section->output_section != NULL)
> +	{

  I don't think you should unconditionally assume that the type of h1 is
necessarily a defined symbol here.  All the other code in that function
verifies the expected type of symbol in h1->root.type, and so should your code.

  OK once that and the formatting is fixed.

    cheers,
      DaveK