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: support compressed sections in addr2line, objdump, readelf


Hi Craig,

Ian Lance Taylor and Cary Coutant have given the patch a provisional
look-through, without finding any problems.  Please give a look and
let me know if you think it's good to commit.

The patch looks pretty good, although there are a few formatting issues:


+/* Extracted from compress.c.  */
+bfd_boolean bfd_uncompress_section_contents
+   (bfd_byte **buffer, bfd_size_type *size);

Please put the function name on a separate line after its return type, rather than following it on the same line. (This is helpful to people who use emacs).


+/* Read a section into its appropriate place in the dwarf2_debug
+ * struct (indicated by SECTION_BUFFER and SECTION_SIZE).  If syms is
+ * not NULL, use bfd_simple_get_relocated_section_contents to read the
+ * section contents, otherwise use bfd_get_section_contents.  */

Please do not start comment lines with asterisks. Comments are meant to be read as straightforward sentences without any funky formatting.


+static bfd_boolean
+read_section (bfd *abfd,
+              const char* section_name, const char* compressed_section_name,
+              asymbol** syms, bfd_uint64_t offset,
+              bfd_byte **section_buffer, unsigned long *section_size)
+{
+  asection *msec;
+  int section_is_compressed = 0;

Why is "section_is_compressed" an int instead of a bfd_boolean ?


+  if (! read_section (unit->abfd, ".debug_str", ".zdebug_str",
+  if (! read_section (abfd, ".debug_abbrev", ".zdebug_abbrev",
+  if (! read_section (abfd, ".debug_line", ".zdebug_line",

Hmm - can the compressed section name always be deduced from the uncompressed name ? If so then there is no need for the compressed name to be provided to read_section(), which might help to reduce code bugs from a mis-typed compressed section name.



+      /* There can be more than one DWARF2 info section in a BFD these
+         days.  First handle the easy case there's only one.  If

Missing word: "First handle the easy case *when* there's only one."



One final question - how did you test your patch ?


Cheers
  Nick


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