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] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |