Re: [PATCH] Mach-O: make "objdump -S" work

[Tristan Gingold <gingold at adacore dot com>]

On Nov 26, 2011, at 10:07 AM, shinichiro hamaji wrote:

> Hello,
> 
> I'm new to here but would like to report an issue and propose a patch.
> 
> I'm using objdump -S to disassemble objects with source view long time
> for ELF, but it seems not to work for Mach-O binaries.
> 
> I've noticed bfd_mach_o_find_nearest_line is not implemented and it
> looked not difficult to fix. I wrote a patch, it compiles with
> --enable-targets=all, and "make check" seems to be OK
> (5 tests in ld are failing but this happens even before my patch
> on my box. Otherwise all make check succeeded).
> 
> I'm putting my patch at the bottom of this email. I'm not sure if this
> is the best way to propose patches to binutils from a non-committer.
> I chose this way just because I saw some committers are sending emails
> like this. Please let me know if there is a more recommended way.
> 
> I'm thinking to write a few more patches if this attempt
> succeeds. I've noticed some functions are not implemented for Mach-O
> and there might be memory leaks (valgrind complains when I run
> "objdump -x" for mach-o objects and I'm guessing coffgen.c should call
> bfd_dwarf2_cleanup_debug_info).
> 
> Any kind of suggestions will be appreciated.

Thank you for this patch.

The Mach-O part looks right, although you have to know that this won't work with executables.  Maybe you should exit early for executables and shared libraries.

One note for COFF maintainers (Dave CC:): you don't cleanup debug info.  Is there a trade-off I don't know ?

A few comments in the patch.

Tristan.

> 
> Thanks!
> 
> 
> diff --git a/bfd/ChangeLog b/bfd/ChangeLog
> index 3755368..cc3d98a 100644
> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog
> @@ -1,3 +1,23 @@
> +2011-11-26  Shinichiro Hamaji  <shinichiro.hamaji@gmail.com>
> +
> +	* dwarf2.c (bfd_dwarf2_cleanup_debug_info): Accept stash as an
> +	argument like other functions to support formats other than ELF.
> +	* elf-bfd.h (bfd_dwarf2_cleanup_debug_info): Move to bfd-in.h.
> +	* elf.c (_bfd_elf_close_and_cleanup): Pass dwarf2_find_line_info
> +	in tdata as a parameter.
> +	* libbfd-in.h (bfd_dwarf2_cleanup_debug_info): Move from
> +	elf-bfd.h.
> +	* libbfd.h (bfd_dwarf2_cleanup_debug_info): Regenerate.
> +	* mach-o-target.c (bfd_mach_o_close_and_cleanup): Remove the
> +	fallback macro.
> +	(bfd_mach_o_find_nearest_line): Likewise.
> +	* mach-o.c (bfd_mach_o_find_nearest_line): Add the definition
> +	which calls _bfd_dwarf2_find_nearest_line.
> +	(bfd_mach_o_close_and_cleanup): Likewise.
> +	* mach-o.h (mach_o_data_struct): Add dwarf2_find_line_info.
> +	(bfd_mach_o_find_nearest_line): Add declaration.
> +	(bfd_mach_o_close_and_cleanup): Add declaration.
> +
> 2011-11-23  Tristan Gingold  <gingold@adacore.com>
> 
> 	* vms-lib.c (get_idxlen): Add comments.  Fix type in sizeof.
> diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
> index 577f278..2670bfd 100644
> --- a/bfd/dwarf2.c
> +++ b/bfd/dwarf2.c
> @@ -3527,17 +3527,14 @@ _bfd_dwarf2_find_inliner_info (bfd *abfd
> ATTRIBUTE_UNUSED,
> }
> 
> void
> -_bfd_dwarf2_cleanup_debug_info (bfd *abfd)
> +_bfd_dwarf2_cleanup_debug_info (bfd *abfd, void **pinfo)
> {
> -  struct comp_unit *each;
>   struct dwarf2_debug *stash;
> +  struct comp_unit *each;
> 
> -  if (abfd == NULL || elf_tdata (abfd) == NULL)
> -    return;
> -
> -  stash = (struct dwarf2_debug *) elf_tdata (abfd)->dwarf2_find_line_info;
> +  stash = (struct dwarf2_debug *) *pinfo;

I think you should initialize stash at declaration.

> 
> -  if (stash == NULL)
> +  if (abfd == NULL || stash == NULL)
>     return;
> 
>   for (each = stash->all_comp_units; each; each = each->next_unit)
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index d6e2ab2..f7e87aa 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -2132,9 +2132,6 @@ extern unsigned int _bfd_elf_common_section_index
> extern asection *_bfd_elf_common_section
>   (asection *);
> 
> -extern void _bfd_dwarf2_cleanup_debug_info
> -  (bfd *);
> -
> extern bfd_vma _bfd_elf_default_got_elt_size
> (bfd *, struct bfd_link_info *, struct elf_link_hash_entry *, bfd *,
>  unsigned long);
> diff --git a/bfd/elf.c b/bfd/elf.c
> index dc6a9bb..e839100 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -7698,11 +7698,12 @@ _bfd_elf_validate_reloc (bfd *abfd, arelent *areloc)
> bfd_boolean
> _bfd_elf_close_and_cleanup (bfd *abfd)
> {
> -  if (bfd_get_format (abfd) == bfd_object)
> +  struct elf_obj_tdata *tdata = elf_tdata (abfd);
> +  if (bfd_get_format (abfd) == bfd_object && tdata != NULL)
>     {
> -      if (elf_tdata (abfd) != NULL && elf_shstrtab (abfd) != NULL)
> +      if (elf_shstrtab (abfd) != NULL)
> 	_bfd_elf_strtab_free (elf_shstrtab (abfd));
> -      _bfd_dwarf2_cleanup_debug_info (abfd);
> +      _bfd_dwarf2_cleanup_debug_info (abfd, &tdata->dwarf2_find_line_info);
>     }
> 
>   return _bfd_generic_close_and_cleanup (abfd);
> diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
> index ccc6c65..7db09e4 100644
> --- a/bfd/libbfd-in.h
> +++ b/bfd/libbfd-in.h
> @@ -548,7 +548,11 @@ bfd_boolean _bfd_generic_find_line
> /* Find inliner info after calling bfd_find_nearest_line. */
> extern bfd_boolean _bfd_dwarf2_find_inliner_info
>   (bfd *, const char **, const char **, unsigned int *, void **);
> -
> +
> +/* Clean up the data used to handle DWARF 2 debugging information. */
> +extern void _bfd_dwarf2_cleanup_debug_info
> +  (bfd *, void **);
> +
> /* Create a new section entry.  */
> extern struct bfd_hash_entry *bfd_section_hash_newfunc
>   (struct bfd_hash_entry *, struct bfd_hash_table *, const char *);
> diff --git a/bfd/libbfd.h b/bfd/libbfd.h
> index d08c2ba..e459f82 100644
> --- a/bfd/libbfd.h
> +++ b/bfd/libbfd.h
> @@ -553,7 +553,11 @@ bfd_boolean _bfd_generic_find_line
> /* Find inliner info after calling bfd_find_nearest_line. */
> extern bfd_boolean _bfd_dwarf2_find_inliner_info
>   (bfd *, const char **, const char **, unsigned int *, void **);
> -
> +
> +/* Clean up the data used to handle DWARF 2 debugging information. */
> +extern void _bfd_dwarf2_cleanup_debug_info
> +  (bfd *, void **);
> +
> /* Create a new section entry.  */
> extern struct bfd_hash_entry *bfd_section_hash_newfunc
>   (struct bfd_hash_entry *, struct bfd_hash_table *, const char *);
> diff --git a/bfd/mach-o-target.c b/bfd/mach-o-target.c
> index 29682c9..9bc6d14 100644
> --- a/bfd/mach-o-target.c
> +++ b/bfd/mach-o-target.c
> @@ -25,13 +25,11 @@
> #ifndef MACH_O_TARGET_COMMON_DEFINED
> #define MACH_O_TARGET_COMMON_DEFINED
> 
> -#define bfd_mach_o_close_and_cleanup
> _bfd_generic_close_and_cleanup
> #define bfd_mach_o_bfd_free_cached_info
> _bfd_generic_bfd_free_cached_info
> #define bfd_mach_o_get_section_contents_in_window
> _bfd_generic_get_section_contents_in_window
> #define bfd_mach_o_bfd_is_target_special_symbol       ((bfd_boolean
> (*) (bfd *, asymbol *)) bfd_false)
> #define bfd_mach_o_bfd_is_local_label_name
> bfd_generic_is_local_label_name
> #define bfd_mach_o_get_lineno                         _bfd_nosymbols_get_lineno
> -#define bfd_mach_o_find_nearest_line
> _bfd_nosymbols_find_nearest_line
> #define bfd_mach_o_find_inliner_info
> _bfd_nosymbols_find_inliner_info
> #define bfd_mach_o_bfd_make_debug_symbol
> _bfd_nosymbols_bfd_make_debug_symbol
> #define bfd_mach_o_read_minisymbols
> _bfd_generic_read_minisymbols
> diff --git a/bfd/mach-o.c b/bfd/mach-o.c
> index 96b993c..b15765a 100644
> --- a/bfd/mach-o.c
> +++ b/bfd/mach-o.c
> @@ -4191,6 +4191,39 @@ bfd_mach_o_core_file_failing_signal (bfd *abfd
> ATTRIBUTE_UNUSED)
>   return 0;
> }
> 
> +bfd_boolean
> +bfd_mach_o_find_nearest_line (bfd *abfd,
> +			      asection *section,
> +			      asymbol **symbols,
> +			      bfd_vma offset,
> +			      const char **filename_ptr,
> +			      const char **functionname_ptr,
> +			      unsigned int *line_ptr)
> +{
> +  bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
> +  if (_bfd_dwarf2_find_nearest_line (abfd, dwarf_debug_sections,
> +				     section, symbols, offset,
> +				     filename_ptr, functionname_ptr,
> +				     line_ptr, 0,
> +				     &mdata->dwarf2_find_line_info))
> +    {
> +      return TRUE;
> +    }

Extra { } pair.

> +  return FALSE;
> +}
> +
> +bfd_boolean
> +bfd_mach_o_close_and_cleanup (bfd *abfd)
> +{
> +  bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
> +  if (bfd_get_format (abfd) == bfd_object && mdata != NULL)
> +    {
> +      _bfd_dwarf2_cleanup_debug_info (abfd, &mdata->dwarf2_find_line_info);
> +    }

Extra { } pair ?

> +
> +  return _bfd_generic_close_and_cleanup (abfd);
> +}
> +
> #define bfd_mach_o_bfd_reloc_type_lookup _bfd_norelocs_bfd_reloc_type_lookup
> #define bfd_mach_o_bfd_reloc_name_lookup _bfd_norelocs_bfd_reloc_name_lookup
> 
> diff --git a/bfd/mach-o.h b/bfd/mach-o.h
> index 53d97da..564df26 100644
> --- a/bfd/mach-o.h
> +++ b/bfd/mach-o.h
> @@ -516,6 +516,9 @@ typedef struct mach_o_data_struct
>      is expected.  */
>   bfd_mach_o_symtab_command *symtab;
>   bfd_mach_o_dysymtab_command *dysymtab;
> +
> +  /* A place to stash dwarf2 info for this bfd.  */
> +  void *dwarf2_find_line_info;
> }
> bfd_mach_o_data_struct;
> 
> @@ -587,6 +590,10 @@ unsigned int
> bfd_mach_o_get_section_type_from_name (const char *);
> unsigned int bfd_mach_o_get_section_attribute_from_name (const char *);
> void bfd_mach_o_normalize_section_name (const char *, const char *,
>                                         const char **, flagword *);
> +bfd_boolean bfd_mach_o_find_nearest_line (bfd *, asection *, asymbol **,
> +                                          bfd_vma, const char **,
> +                                          const char **, unsigned int *);
> +bfd_boolean bfd_mach_o_close_and_cleanup (bfd *);
> 
> extern const bfd_target mach_o_fat_vec;