[PATCH] Don't check PST is NULL in read_symtab

[PATCH] Don't check PST is NULL in read_symtab

[Yao Qi]

Hi,
The "method" read_symtab of 'struct partial_symtab" is called in this
way,

  (*pst->read_symtab) (objfile, pst);

so argument PST can't be NULL.  This patch is to remove the checking
that PST is NULL.  I'll re-indent dwarf2_psymtab_to_symtab when check
this patch in.
Rebuild GDB for all targets.  Is it OK?

gdb:

2013-01-11  Yao Qi  <yao@codesourcery.com>

	* dbxread.c (dbx_psymtab_to_symtab_1): Don't check PST is NULL.
	* dwarf2read.c (dwarf2_psymtab_to_symtab): Likewise.
	* mdebugread.c (mdebug_psymtab_to_symtab): Likewise.
	* xcoffread.c (xcoff_psymtab_to_symtab_1): Likewise.
---
 gdb/dbxread.c    |    3 ---
 gdb/dwarf2read.c |    3 ---
 gdb/mdebugread.c |    3 ---
 gdb/xcoffread.c  |    3 ---
 4 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index ebe4237..8305a5d 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2406,9 +2406,6 @@ dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
   struct cleanup *old_chain;
   int i;
 
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered (gdb_stderr, "Psymtab for %s already read in.  "
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e2088f1..f2d6a0a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -6410,8 +6410,6 @@ locate_pdi_sibling (const struct die_reader_specs *reader,
 static void
 dwarf2_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (pst != NULL)
-    {
       if (pst->readin)
 	{
 	  warning (_("bug: psymtab for %s is already read in."),
@@ -6451,7 +6449,6 @@ dwarf2_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 	  if (info_verbose)
 	    printf_filtered (_("done.\n"));
 	}
-    }
 
   process_cu_includes ();
 }
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 856ca9d..0be1095 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -278,9 +278,6 @@ static char *mdebug_next_symbol_text (struct objfile *);
 static void
 mdebug_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (!pst)
-    return;
-
   if (info_verbose)
     {
       printf_filtered (_("Reading in symbols for %s..."), pst->filename);
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 9fe8621..f4dcae2 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -1860,9 +1860,6 @@ xcoff_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
 static void
 xcoff_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered
-- 
1.7.7.6

Re: [PATCH] Don't check PST is NULL in read_symtab

[Joel Brobecker]

> The "method" read_symtab of 'struct partial_symtab" is called in this
> way,
> 
>   (*pst->read_symtab) (objfile, pst);
> 
> so argument PST can't be NULL.

I confirmed it using a quick grep.

> I'll re-indent dwarf2_psymtab_to_symtab when check this patch in.

FWIW, you can also do the re-indent, and then post two parallel
patches: One is the actual patch, the second is a sub-part of
the actual patch where whitespace differences are ignored.
That's usually how I verify that my reformatting did not cause
any intentional changes...

> 2013-01-11  Yao Qi  <yao@codesourcery.com>
> 
> 	* dbxread.c (dbx_psymtab_to_symtab_1): Don't check PST is NULL.
> 	* dwarf2read.c (dwarf2_psymtab_to_symtab): Likewise.
> 	* mdebugread.c (mdebug_psymtab_to_symtab): Likewise.
> 	* xcoffread.c (xcoff_psymtab_to_symtab_1): Likewise.

If PST must not be NULL, I think that it should be documented in
psympriv.h. I am also wondering whether an assertion would be
useful or not; maybe not, just thinking out loud.

Tom and Doug are more involved in this area, so they'll probably
have a more definitive answer.

-- 
Joel

Re: [PATCH] Don't check PST is NULL in read_symtab

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> If PST must not be NULL, I think that it should be documented in
Joel> psympriv.h. I am also wondering whether an assertion would be
Joel> useful or not; maybe not, just thinking out loud.

Joel> Tom and Doug are more involved in this area, so they'll probably
Joel> have a more definitive answer.

An assert would be fine by me.

Tom

Re: [PATCH] Don't check PST is NULL in read_symtab

[Yao Qi]

On 01/11/2013 10:34 PM, Tom Tromey wrote:
> Joel> If PST must not be NULL, I think that it should be documented in
> Joel> psympriv.h. I am also wondering whether an assertion would be
> Joel> useful or not; maybe not, just thinking out loud.
>
> Joel> Tom and Doug are more involved in this area, so they'll probably
> Joel> have a more definitive answer.
>
> An assert would be fine by me.

An assert that PST is not NULL at the beginning of these functions 
dbx_psymtab_to_symtab_1, dwarf2_psymtab_to_symtab, 
mdebug_psymtab_to_symtab, and xcoff_psymtab_to_symtab_1?

-- 
Yao (齐尧)

Re: [PATCH] Don't check PST is NULL in read_symtab

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> On 01/11/2013 10:34 PM, Tom Tromey wrote:
Joel> If PST must not be NULL, I think that it should be documented in
Joel> psympriv.h. I am also wondering whether an assertion would be
Joel> useful or not; maybe not, just thinking out loud.
>> 
Joel> Tom and Doug are more involved in this area, so they'll probably
Joel> have a more definitive answer.
>> 
>> An assert would be fine by me.

Yao> An assert that PST is not NULL at the beginning of these functions
Yao> dbx_psymtab_to_symtab_1, dwarf2_psymtab_to_symtab,
Yao> mdebug_psymtab_to_symtab, and xcoff_psymtab_to_symtab_1?

Sure.  Though I am ok with just a comment as well.

Tom

[committed]: [PATCH] Don't check PST is NULL in read_symtab

[Yao Qi]

On 01/11/2013 11:06 PM, Tom Tromey wrote:
> Sure.  Though I am ok with just a comment as well.

Below is what I committed, with the comments added.

-- 
Yao (齐尧)

gdb:
    
2013-01-14  Yao Qi  <yao@codesourcery.com>
    
	* dbxread.c (dbx_psymtab_to_symtab_1): Don't check PST is NULL.
	(dbx_psymtab_to_symtab): Likewise.
	* dwarf2read.c (dwarf2_psymtab_to_symtab): Likewise.
	* mdebugread.c (mdebug_psymtab_to_symtab): Likewise.
	* xcoffread.c (xcoff_psymtab_to_symtab_1): Likewise.

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index ebe4237..16496d1 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2406,9 +2406,6 @@ dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
   struct cleanup *old_chain;
   int i;
 
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered (gdb_stderr, "Psymtab for %s already read in.  "
@@ -2455,7 +2452,7 @@ dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
 }
 
 /* Read in all of the symbols for a given psymtab for real.
-   Be verbose about it if the user wants that.  */
+   Be verbose about it if the user wants that.  PST is not NULL.  */
 
 static void
 dbx_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
@@ -2463,9 +2460,6 @@ dbx_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
   bfd *sym_bfd;
   struct cleanup *back_to = NULL;
 
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered (gdb_stderr, "Psymtab for %s already read in.  "
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e2088f1..7af89c6 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -6405,52 +6405,50 @@ locate_pdi_sibling (const struct die_reader_specs *reader,
   return skip_children (reader, info_ptr);
 }
 
-/* Expand this partial symbol table into a full symbol table.  */
+/* Expand this partial symbol table into a full symbol table.  PST is
+   not NULL.  */
 
 static void
 dwarf2_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (pst != NULL)
+  if (pst->readin)
     {
-      if (pst->readin)
+      warning (_("bug: psymtab for %s is already read in."),
+	       pst->filename);
+    }
+  else
+    {
+      if (info_verbose)
 	{
-	  warning (_("bug: psymtab for %s is already read in."),
-		   pst->filename);
+	  printf_filtered (_("Reading in symbols for %s..."),
+			   pst->filename);
+	  gdb_flush (gdb_stdout);
 	}
-      else
-	{
-	  if (info_verbose)
-	    {
-	      printf_filtered (_("Reading in symbols for %s..."),
-			       pst->filename);
-	      gdb_flush (gdb_stdout);
-	    }
 
-	  /* Restore our global data.  */
-	  dwarf2_per_objfile = objfile_data (objfile, dwarf2_objfile_data_key);
+      /* Restore our global data.  */
+      dwarf2_per_objfile = objfile_data (objfile, dwarf2_objfile_data_key);
 
-	  /* If this psymtab is constructed from a debug-only objfile, the
-	     has_section_at_zero flag will not necessarily be correct.  We
-	     can get the correct value for this flag by looking at the data
-	     associated with the (presumably stripped) associated objfile.  */
-	  if (objfile->separate_debug_objfile_backlink)
-	    {
-	      struct dwarf2_per_objfile *dpo_backlink
-	        = objfile_data (objfile->separate_debug_objfile_backlink,
-		                dwarf2_objfile_data_key);
+      /* If this psymtab is constructed from a debug-only objfile, the
+	 has_section_at_zero flag will not necessarily be correct.  We
+	 can get the correct value for this flag by looking at the data
+	 associated with the (presumably stripped) associated objfile.  */
+      if (objfile->separate_debug_objfile_backlink)
+	{
+	  struct dwarf2_per_objfile *dpo_backlink
+	    = objfile_data (objfile->separate_debug_objfile_backlink,
+			    dwarf2_objfile_data_key);
 
-	      dwarf2_per_objfile->has_section_at_zero
-		= dpo_backlink->has_section_at_zero;
-	    }
+	  dwarf2_per_objfile->has_section_at_zero
+	    = dpo_backlink->has_section_at_zero;
+	}
 
-	  dwarf2_per_objfile->reading_partial_symbols = 0;
+      dwarf2_per_objfile->reading_partial_symbols = 0;
 
-	  psymtab_to_symtab_1 (pst);
+      psymtab_to_symtab_1 (pst);
 
-	  /* Finish up the debug error message.  */
-	  if (info_verbose)
-	    printf_filtered (_("done.\n"));
-	}
+      /* Finish up the debug error message.  */
+      if (info_verbose)
+	printf_filtered (_("done.\n"));
     }
 
   process_cu_includes ();
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 856ca9d..2eb0536 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -273,14 +273,11 @@ static char *mdebug_next_symbol_text (struct objfile *);
 /* Exported procedure: Builds a symtab from the PST partial one.
    Restores the environment in effect when PST was created, delegates
    most of the work to an ancillary procedure, and sorts
-   and reorders the symtab list at the end.  */
+   and reorders the symtab list at the end.  PST is not NULL.  */
 
 static void
 mdebug_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (!pst)
-    return;
-
   if (info_verbose)
     {
       printf_filtered (_("Reading in symbols for %s..."), pst->filename);
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 9fe8621..41aaf02 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -1855,14 +1855,11 @@ xcoff_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
 }
 
 /* Read in all of the symbols for a given psymtab for real.
-   Be verbose about it if the user wants that.  */
+   Be verbose about it if the user wants that.  PST is not NULL.  */
 
 static void
 xcoff_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered

[committed]: [PATCH] Don't check PST is NULL in read_symtab

[Yao Qi]

On 01/11/2013 11:06 PM, Tom Tromey wrote:
> Sure.  Though I am ok with just a comment as well.

Below is what I committed, with the comments added.

-- 
Yao (齐尧)

gdb:
    
2013-01-14  Yao Qi  <yao@codesourcery.com>
    
	* dbxread.c (dbx_psymtab_to_symtab_1): Don't check PST is NULL.
	(dbx_psymtab_to_symtab): Likewise.
	* dwarf2read.c (dwarf2_psymtab_to_symtab): Likewise.
	* mdebugread.c (mdebug_psymtab_to_symtab): Likewise.
	* xcoffread.c (xcoff_psymtab_to_symtab_1): Likewise.

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index ebe4237..16496d1 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2406,9 +2406,6 @@ dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
   struct cleanup *old_chain;
   int i;
 
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered (gdb_stderr, "Psymtab for %s already read in.  "
@@ -2455,7 +2452,7 @@ dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
 }
 
 /* Read in all of the symbols for a given psymtab for real.
-   Be verbose about it if the user wants that.  */
+   Be verbose about it if the user wants that.  PST is not NULL.  */
 
 static void
 dbx_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
@@ -2463,9 +2460,6 @@ dbx_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
   bfd *sym_bfd;
   struct cleanup *back_to = NULL;
 
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered (gdb_stderr, "Psymtab for %s already read in.  "
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e2088f1..7af89c6 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -6405,52 +6405,50 @@ locate_pdi_sibling (const struct die_reader_specs *reader,
   return skip_children (reader, info_ptr);
 }
 
-/* Expand this partial symbol table into a full symbol table.  */
+/* Expand this partial symbol table into a full symbol table.  PST is
+   not NULL.  */
 
 static void
 dwarf2_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (pst != NULL)
+  if (pst->readin)
     {
-      if (pst->readin)
+      warning (_("bug: psymtab for %s is already read in."),
+	       pst->filename);
+    }
+  else
+    {
+      if (info_verbose)
 	{
-	  warning (_("bug: psymtab for %s is already read in."),
-		   pst->filename);
+	  printf_filtered (_("Reading in symbols for %s..."),
+			   pst->filename);
+	  gdb_flush (gdb_stdout);
 	}
-      else
-	{
-	  if (info_verbose)
-	    {
-	      printf_filtered (_("Reading in symbols for %s..."),
-			       pst->filename);
-	      gdb_flush (gdb_stdout);
-	    }
 
-	  /* Restore our global data.  */
-	  dwarf2_per_objfile = objfile_data (objfile, dwarf2_objfile_data_key);
+      /* Restore our global data.  */
+      dwarf2_per_objfile = objfile_data (objfile, dwarf2_objfile_data_key);
 
-	  /* If this psymtab is constructed from a debug-only objfile, the
-	     has_section_at_zero flag will not necessarily be correct.  We
-	     can get the correct value for this flag by looking at the data
-	     associated with the (presumably stripped) associated objfile.  */
-	  if (objfile->separate_debug_objfile_backlink)
-	    {
-	      struct dwarf2_per_objfile *dpo_backlink
-	        = objfile_data (objfile->separate_debug_objfile_backlink,
-		                dwarf2_objfile_data_key);
+      /* If this psymtab is constructed from a debug-only objfile, the
+	 has_section_at_zero flag will not necessarily be correct.  We
+	 can get the correct value for this flag by looking at the data
+	 associated with the (presumably stripped) associated objfile.  */
+      if (objfile->separate_debug_objfile_backlink)
+	{
+	  struct dwarf2_per_objfile *dpo_backlink
+	    = objfile_data (objfile->separate_debug_objfile_backlink,
+			    dwarf2_objfile_data_key);
 
-	      dwarf2_per_objfile->has_section_at_zero
-		= dpo_backlink->has_section_at_zero;
-	    }
+	  dwarf2_per_objfile->has_section_at_zero
+	    = dpo_backlink->has_section_at_zero;
+	}
 
-	  dwarf2_per_objfile->reading_partial_symbols = 0;
+      dwarf2_per_objfile->reading_partial_symbols = 0;
 
-	  psymtab_to_symtab_1 (pst);
+      psymtab_to_symtab_1 (pst);
 
-	  /* Finish up the debug error message.  */
-	  if (info_verbose)
-	    printf_filtered (_("done.\n"));
-	}
+      /* Finish up the debug error message.  */
+      if (info_verbose)
+	printf_filtered (_("done.\n"));
     }
 
   process_cu_includes ();
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 856ca9d..2eb0536 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -273,14 +273,11 @@ static char *mdebug_next_symbol_text (struct objfile *);
 /* Exported procedure: Builds a symtab from the PST partial one.
    Restores the environment in effect when PST was created, delegates
    most of the work to an ancillary procedure, and sorts
-   and reorders the symtab list at the end.  */
+   and reorders the symtab list at the end.  PST is not NULL.  */
 
 static void
 mdebug_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (!pst)
-    return;
-
   if (info_verbose)
     {
       printf_filtered (_("Reading in symbols for %s..."), pst->filename);
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 9fe8621..41aaf02 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -1855,14 +1855,11 @@ xcoff_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
 }
 
 /* Read in all of the symbols for a given psymtab for real.
-   Be verbose about it if the user wants that.  */
+   Be verbose about it if the user wants that.  PST is not NULL.  */
 
 static void
 xcoff_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-  if (!pst)
-    return;
-
   if (pst->readin)
     {
       fprintf_unfiltered

Re: [PATCH] Don't check PST is NULL in read_symtab

[Yao Qi]

On 01/11/2013 12:52 PM, Joel Brobecker wrote:
> FWIW, you can also do the re-indent, and then post two parallel
> patches: One is the actual patch, the second is a sub-part of
> the actual patch where whitespace differences are ignored.
> That's usually how I verify that my reformatting did not cause
> any intentional changes...

I see.  Below is the patch to indent the code.  Thanks for your
suggestion, Joel.

> 
>> >2013-01-11  Yao Qi<yao@codesourcery.com>
>> >
>> >	* dbxread.c (dbx_psymtab_to_symtab_1): Don't check PST is NULL.
>> >	* dwarf2read.c (dwarf2_psymtab_to_symtab): Likewise.
>> >	* mdebugread.c (mdebug_psymtab_to_symtab): Likewise.
>> >	* xcoffread.c (xcoff_psymtab_to_symtab_1): Likewise.
> If PST must not be NULL, I think that it should be documented in
> psympriv.h. I am also wondering whether an assertion would be
> useful or not; maybe not, just thinking out loud.
> 

IMO, we don't need an assertion to check PST, because the function is
used in this way,

  (*pst->read_symtab) (objfile, pst);

which guarantees PST cant' be NULL.  "PST" here is analogous to "this"
object in C++, and we don't have to document function read_symtab that
"this object must not be NULL".

-- 
Yao (齐尧)

gdb:

2013-01-11  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (dwarf2_psymtab_to_symtab): Code indent.
---
 gdb/dwarf2read.c |   62 +++++++++++++++++++++++++++---------------------------
 1 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index f2d6a0a..2cefee9 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -6410,45 +6410,45 @@ locate_pdi_sibling (const struct die_reader_specs *reader,
 static void
 dwarf2_psymtab_to_symtab (struct objfile *objfile, struct partial_symtab *pst)
 {
-      if (pst->readin)
+  if (pst->readin)
+    {
+      warning (_("bug: psymtab for %s is already read in."),
+	       pst->filename);
+    }
+  else
+    {
+      if (info_verbose)
 	{
-	  warning (_("bug: psymtab for %s is already read in."),
-		   pst->filename);
+	  printf_filtered (_("Reading in symbols for %s..."),
+			   pst->filename);
+	  gdb_flush (gdb_stdout);
 	}
-      else
-	{
-	  if (info_verbose)
-	    {
-	      printf_filtered (_("Reading in symbols for %s..."),
-			       pst->filename);
-	      gdb_flush (gdb_stdout);
-	    }
 
-	  /* Restore our global data.  */
-	  dwarf2_per_objfile = objfile_data (objfile, dwarf2_objfile_data_key);
+      /* Restore our global data.  */
+      dwarf2_per_objfile = objfile_data (objfile, dwarf2_objfile_data_key);
 
-	  /* If this psymtab is constructed from a debug-only objfile, the
-	     has_section_at_zero flag will not necessarily be correct.  We
-	     can get the correct value for this flag by looking at the data
-	     associated with the (presumably stripped) associated objfile.  */
-	  if (objfile->separate_debug_objfile_backlink)
-	    {
-	      struct dwarf2_per_objfile *dpo_backlink
-	        = objfile_data (objfile->separate_debug_objfile_backlink,
-		                dwarf2_objfile_data_key);
+      /* If this psymtab is constructed from a debug-only objfile, the
+	 has_section_at_zero flag will not necessarily be correct.  We
+	 can get the correct value for this flag by looking at the data
+	 associated with the (presumably stripped) associated objfile.  */
+      if (objfile->separate_debug_objfile_backlink)
+	{
+	  struct dwarf2_per_objfile *dpo_backlink
+	    = objfile_data (objfile->separate_debug_objfile_backlink,
+			    dwarf2_objfile_data_key);
 
-	      dwarf2_per_objfile->has_section_at_zero
-		= dpo_backlink->has_section_at_zero;
-	    }
+	  dwarf2_per_objfile->has_section_at_zero
+	    = dpo_backlink->has_section_at_zero;
+	}
 
-	  dwarf2_per_objfile->reading_partial_symbols = 0;
+      dwarf2_per_objfile->reading_partial_symbols = 0;
 
-	  psymtab_to_symtab_1 (pst);
+      psymtab_to_symtab_1 (pst);
 
-	  /* Finish up the debug error message.  */
-	  if (info_verbose)
-	    printf_filtered (_("done.\n"));
-	}
+      /* Finish up the debug error message.  */
+      if (info_verbose)
+	printf_filtered (_("done.\n"));
+    }
 
   process_cu_includes ();
 }
-- 
1.7.7.6

Re: [PATCH] Don't check PST is NULL in read_symtab

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

> IMO, we don't need an assertion to check PST, because the function is
> used in this way,
> 
>   (*pst->read_symtab) (objfile, pst);

I am fine without the assertion as well. But if we followed your
argument, we would never need an assertion. For me, assertions
achieve two goals:
  1. Clearly document an expectation;
  2. Cause a semi-friendly abortion, rather than a mysterious behavior
     or crash.
As of today, the way this function is called indeed guarantees that
PST is never NULL. But someone adding a call at a later date might
introduce a bug and cause it to be called with PST == NULL...

> 2013-01-11  Yao Qi  <yao@codesourcery.com>
> 
> 	* dwarf2read.c (dwarf2_psymtab_to_symtab): Code indent.

Let's take an example to show you what I meant in my first suggestion.
Attached is a bogus change I made a function in dwarf2read. I added
an "else" around a large block of code. The first patch shows the
actual change, with code reindentation. That's the real patch which
would be checked in eventually - but it's barely readable. So,
to better show the real changes, I also attach the result of
"git diff/show -b", where whitespace-only changes are ignored.

-- 
Joel

[-- Attachment #2: dwarf2read-actual.diff --]
[-- Type: text/x-diff, Size: 2678 bytes --]

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e2088f1..d590248 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1918,39 +1918,41 @@ dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info)
 
   if (dwarf2_section_empty_p (info))
     return;
+  else
+    {
+      abfd = sectp->owner;
 
-  abfd = sectp->owner;
+      /* If the section has relocations, we must read it ourselves.
+	 Otherwise we attach it to the BFD.  */
+      if ((sectp->flags & SEC_RELOC) == 0)
+	{
+	  const gdb_byte *bytes = gdb_bfd_map_section (sectp, &info->size);
 
-  /* If the section has relocations, we must read it ourselves.
-     Otherwise we attach it to the BFD.  */
-  if ((sectp->flags & SEC_RELOC) == 0)
-    {
-      const gdb_byte *bytes = gdb_bfd_map_section (sectp, &info->size);
+	  /* We have to cast away const here for historical reasons.
+	     Fixing dwarf2read to be const-correct would be quite nice.  */
+	  info->buffer = (gdb_byte *) bytes;
+	  return;
+	}
 
-      /* We have to cast away const here for historical reasons.
-	 Fixing dwarf2read to be const-correct would be quite nice.  */
-      info->buffer = (gdb_byte *) bytes;
-      return;
-    }
+      buf = obstack_alloc (&objfile->objfile_obstack, info->size);
+      info->buffer = buf;
 
-  buf = obstack_alloc (&objfile->objfile_obstack, info->size);
-  info->buffer = buf;
+      /* When debugging .o files, we may need to apply relocations; see
+	 http://sourceware.org/ml/gdb-patches/2002-04/msg00136.html .
+	 We never compress sections in .o files, so we only need to
+	 try this when the section is not compressed.  */
+      retbuf = symfile_relocate_debug_section (objfile, sectp, buf);
+      if (retbuf != NULL)
+	{
+	  info->buffer = retbuf;
+	  return;
+	}
 
-  /* When debugging .o files, we may need to apply relocations; see
-     http://sourceware.org/ml/gdb-patches/2002-04/msg00136.html .
-     We never compress sections in .o files, so we only need to
-     try this when the section is not compressed.  */
-  retbuf = symfile_relocate_debug_section (objfile, sectp, buf);
-  if (retbuf != NULL)
-    {
-      info->buffer = retbuf;
-      return;
+      if (bfd_seek (abfd, sectp->filepos, SEEK_SET) != 0
+	  || bfd_bread (buf, info->size, abfd) != info->size)
+	error (_("Dwarf Error: Can't read DWARF data from '%s'"),
+	       bfd_get_filename (abfd));
     }
-
-  if (bfd_seek (abfd, sectp->filepos, SEEK_SET) != 0
-      || bfd_bread (buf, info->size, abfd) != info->size)
-    error (_("Dwarf Error: Can't read DWARF data from '%s'"),
-	   bfd_get_filename (abfd));
 }
 
 /* A helper function that returns the size of a section in a safe way.

[-- Attachment #3: dwarf2read-spaces_ignored.diff --]
[-- Type: text/x-diff, Size: 733 bytes --]

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e2088f1..d590248 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1918,7 +1918,8 @@ dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info)
 
   if (dwarf2_section_empty_p (info))
     return;
-
+  else
+    {
       abfd = sectp->owner;
 
       /* If the section has relocations, we must read it ourselves.
@@ -1951,6 +1952,7 @@ dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info)
 	  || bfd_bread (buf, info->size, abfd) != info->size)
 	error (_("Dwarf Error: Can't read DWARF data from '%s'"),
 	       bfd_get_filename (abfd));
+    }
 }
 
 /* A helper function that returns the size of a section in a safe way.

Re: [PATCH] Don't check PST is NULL in read_symtab

[Pedro Alves]

On 01/11/2013 03:05 PM, Joel Brobecker wrote:
>> IMO, we don't need an assertion to check PST, because the function is
>> used in this way,
>>
>>   (*pst->read_symtab) (objfile, pst);
> 
> I am fine without the assertion as well. But if we followed your
> argument, we would never need an assertion. For me, assertions
> achieve two goals:
>   1. Clearly document an expectation;
>   2. Cause a semi-friendly abortion, rather than a mysterious behavior
>      or crash.
> As of today, the way this function is called indeed guarantees that
> PST is never NULL. But someone adding a call at a later date might
> introduce a bug and cause it to be called with PST == NULL...

FWIW, I agree with both of you.  I agree with assertion's
roles.  But I also agree with Yao that for functions that implement
a class-like interface and take a "this" pointer, there's no need
to sprinkle the codebase with "gdb_assert (self != NULL)" checks.
BUT (!), when reading one of those functions, it's a bit more obvious
and self-describing that the function takes a "this"-style pointer
when the parameter is actually called "self", and / or at least is
the first parameter in the function's signature.  Like:

static void
dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
{

static void
dbx_psymtab_to_symtab_1 (struct partial_symtab *pst, struct objfile *objfile)
{

static void
dbx_psymtab_to_symtab_1 (struct partial_symtab *self, struct objfile *objfile)
{


(It'd be even better for grepability/readability if the implementations
and hook name agreed, like:

- result->read_symtab = dbx_psymtab_to_symtab;
+ result->read_symtab = dbx_read_symtab;

or

- result->read_symtab = dbx_psymtab_to_symtab;
+ result->psymtab_to_symtab = dbx_psymtab_to_symtab;

...

)

-- 
Pedro Alves

Re: [PATCH] Don't check PST is NULL in read_symtab

[Joel Brobecker]

> FWIW, I agree with both of you.  I agree with assertion's
> roles.  But I also agree with Yao that for functions that implement
> a class-like interface and take a "this" pointer, there's no need
> to sprinkle the codebase with "gdb_assert (self != NULL)" checks.
> BUT (!), when reading one of those functions, it's a bit more obvious
> and self-describing that the function takes a "this"-style pointer
> when the parameter is actually called "self", and / or at least is
> the first parameter in the function's signature.  Like:

I agree with everything said above. Just to be extra clear, in
case I appeared to be insisting - I was just thinking about
the assert, not requesting it.

> static void
> dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
> {
> 
> static void
> dbx_psymtab_to_symtab_1 (struct partial_symtab *pst, struct objfile *objfile)
> {
> 
> static void
> dbx_psymtab_to_symtab_1 (struct partial_symtab *self, struct objfile *objfile)
> {

I like "self" quite a bit, except that it's a little bit less
descriptive that "pst".  But for a "method", I think that's
acceptable.

> (It'd be even better for grepability/readability if the implementations
> and hook name agreed, like:
> 
> - result->read_symtab = dbx_psymtab_to_symtab;
> + result->read_symtab = dbx_read_symtab;
> 
> or
> 
> - result->read_symtab = dbx_psymtab_to_symtab;
> + result->psymtab_to_symtab = dbx_psymtab_to_symtab;

Agree as well.  I don't mind making these two changes after Yao's
commit goes it (unless Yao wants to take care of it, I don't mean
to cut in).

-- 
Joel

Re: [PATCH] Don't check PST is NULL in read_symtab

[Yao Qi]

On 01/12/2013 02:44 AM, Joel Brobecker wrote:
>> >(It'd be even better for grepability/readability if the implementations
>> >and hook name agreed, like:
>> >
>> >- result->read_symtab = dbx_psymtab_to_symtab;
>> >+ result->read_symtab = dbx_read_symtab;
>> >
>> >or
>> >
>> >- result->read_symtab = dbx_psymtab_to_symtab;
>> >+ result->psymtab_to_symtab = dbx_psymtab_to_symtab;
> Agree as well.  I don't mind making these two changes after Yao's
> commit goes it (unless Yao wants to take care of it, I don't mean
> to cut in).

I'll take care of this in a separate patch.

-- 
Yao (齐尧)