This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch pe-coff]: Fix output of debug sections in image


On 20/09/2010 20:33, Kai Tietz wrote:
> Hello,
> 
> this patch fixes the output of debugging sections in PE-image.
> 
> ChangeLog bfd
> 
> 	* coffcode.h (sec_to_styp_flags): Adjust debug
> 	sections to be conform to pe-coff specification
> 	and avoid marking them as excluded.
> 	(styp_to_sec_flags): Doing reverse mapping.

  At the start of sec_to_styp_flags:

> +  int is_dbg = 0;
> +
> +  if (CONST_STRNEQ (sec_name, DOT_DEBUG)
> +#ifdef COFF_LONG_SECTION_NAMES
> +      || CONST_STRNEQ (sec_name, GNU_LINKONCE_WI)
> +#endif
> +      || CONST_STRNEQ (sec_name, ".stab"))
> +    is_dbg = 1;

  Rather than duplicating these string tests, I think it would be more
maintainable to just add "is_dbg = TRUE;" to each of the relevant clauses that
use those same tests below:

   else if (CONST_STRNEQ (sec_name, DOT_DEBUG))
     {
+      is_dbg = TRUE;
       /* Handle the XCOFF debug section and DWARF2 debug sections.  */
       if (!sec_name[6])
 	styp_flags = STYP_XCOFF_DEBUG;
       else
 	styp_flags = STYP_DEBUG_INFO;
     }
   else if (CONST_STRNEQ (sec_name, ".stab"))
     {
+      is_dbg = TRUE;
       styp_flags = STYP_DEBUG_INFO;
     }
 #ifdef COFF_LONG_SECTION_NAMES
   else if (CONST_STRNEQ (sec_name, GNU_LINKONCE_WI))
     {
+      is_dbg = TRUE;
       styp_flags = STYP_DEBUG_INFO;
     }
 #endif

  Also, I would like you to use bfd_boolean and TRUE/FALSE rather than an int;
it's always good if we can use the type system to both convey and enforce
semantics on our data.  Likewise in styp_to_sec_flags, please use bfd_boolean.

> ChangeLog ld
> 
> 	* ldlang.c (lang_add_section): Allow for debugging
> 	section to be marked as noload but to keep content.
> 	(IGNORE_SECTION): Likewise.
> 	(lang_check_section_addresses): Likewise.
> 	* ldwrite.c (build_link_order): Likewise.

  In lang_add_section, we have this:

> @@ -2245,7 +2245,7 @@ lang_add_section (lang_statement_list_ty
>      case noload_section:
>        flags &= ~SEC_LOAD;
>        flags |= SEC_NEVER_LOAD;
> -      if ((flags & SEC_COFF_SHARED_LIBRARY) == 0)
> +      if ((flags & (SEC_COFF_SHARED_LIBRARY | SEC_DEBUGGING)) == 0)
>  	flags &= ~SEC_HAS_CONTENTS;
>        break;
>      }

  If you look at the other reference to SEC_COFF_SHARED_LIBRARY, down in
lang_size_sections_1, you'll see it's guarded with ...

	    if (((bfd_get_flavour (link_info.output_bfd)
		  == bfd_target_ecoff_flavour)
		 || (bfd_get_flavour (link_info.output_bfd)
		     == bfd_target_coff_flavour))
		&& [ ... term using SEC_COFF_SHARED_LIBRARY ... ]

  While you're in there anyway, please add a corresponding check of the owning
BFD (using section->owner) on the lang_add_section "case noload_section:"
clause, so that it only ORs the coff-specific flag into the mask that it ANDs
with "flags" if it actually is a coff section.  (It doesn't matter exactly how
you do this, with a ternary operator or a temporary variable, or if you just
add a separate second if-clause; I just don't think we should be checking that
flag on non-coff targets as we currently are!)

  Finally, in the IGNORE_SECTION changes:

>  #define IGNORE_SECTION(s) \
> -  ((s->flags & SEC_NEVER_LOAD) != 0				\
> +  ((s->flags & (SEC_NEVER_LOAD | SEC_DEBUGGING)) == SEC_NEVER_LOAD	\
>     || (s->flags & SEC_ALLOC) == 0				\
>     || ((s->flags & SEC_THREAD_LOCAL) != 0			\
>  	&& (s->flags & SEC_LOAD) == 0))
> @@ -4590,7 +4590,7 @@ lang_check_section_addresses (void)
>    for (s = link_info.output_bfd->sections; s != NULL; s = s->next)
>      {
>        /* Only consider loadable sections with real contents.  */
> -      if ((s->flags & SEC_NEVER_LOAD)
> +      if ((s->flags & (SEC_NEVER_LOAD | SEC_DEBUGGING)) == SEC_NEVER_LOAD
>  	  || !(s->flags & SEC_LOAD)
>  	  || !(s->flags & SEC_ALLOC)
>  	  || s->size == 0)

... and in ldwrite.c/build_link_order, ...

> @@ -245,7 +245,8 @@ build_link_order (lang_statement_union_t
>  		link_order = bfd_new_link_order (link_info.output_bfd,
>  						 output_section);
>  
> -		if (i->flags & SEC_NEVER_LOAD)
> +		if ((i->flags & SEC_NEVER_LOAD) != 0
> +		    && (i->flags & SEC_DEBUGGING) == 0)
>  		  {
>  		    /* We've got a never load section inside one which
>  		       is going to be output, we'll change it into a

... am I right in thinking that finding SEC_DEBUGGING=1 on a SEC_NEVER_LOAD
section should only happen on COFF?  I'm not sure we should change the
behaviour for ELF targets, so unless they can be guaranteed never to use that
combination (I don't know what the ELF spec says about debug sections),
shouldn't we also be checking for coff-flavoured bfds here too?  If you think
we don't need to, because of some invariant condition or other, then it would
be ok to just add a comment explaining why.

    cheers,
      DaveK


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]