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

[Kai Tietz <Kai dot Tietz at onevision dot com>]

binutils-owner@sourceware.org wrote on 22.09.2010 07:59:52:

> 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

Hmm, not sure what you mean here in the COFF_WITH_PE function. As far as I 
see my patch, I didn't touched that part. Also I see here in the 
non-COFF_WITH_PE no reason to add this is_dbg flag. What get I wrong here?

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

That's ok

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

Well, see here Alan's patch

2010-09-16  Alan Modra

        * ld.texinfo (NOLOAD): Do not erroneously state that contents will
        appear in output file.
        * ldlang.c (lang_add_section): Clear SEC_HAS_CONTENTS on noload
        unless SEC_COFF_SHARED_LIBRARY.
        (map_input_to_output_sections): Don't set SEC_HAS_CONTENTS for 
noload
        output sections.
        (lang_size_sections_1): Don't test SEC_NEVER_LOAD when deciding
        to update dot in region.  Ditto when setting SEC_ALLOC if dot
        advanced due to assignment.
        * ldwrite.c (build_link_order): Don't test SEC_NEVER_LOAD.

By this I read that either a debugging section has not to set 
SEC_NEVER_LOAD on elf, as no output would be written then for it. So the 
combination of debugging and never-load is IMHO something what can just 
happen for pe-coff. As here this section flags has a bit different 
meaning. It is btw even questionable, if we should check for pe-coff at 
all in linker about this flag, but well, for now debugging section output 
for pe-coff is here of more interest.

Regards,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

Attachment: dbg_section.diff
Description: Binary data