Crash in write_exp_msymbol for coff targets.

Crash in write_exp_msymbol for coff targets.

[Pedro Alves]

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

Hi all,

The TLS without debugging info support introduced a bug for coff based 
targets.
While printing for example a global symbol's value I am getting a 
segfault in parse.c:write_exp_msymbol,
at:
  if (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL)

The problem is that minimal symbols may not have a bfd section set.

The attached patch fixes it, but is it correct?
I see in coffread.c, that prim_record_minimal_symbol_and_info is always 
called with a NULL
bfd section, whilst in elfread.c, is is not. Is this a limitation of the 
coff format? Should coffread.c
be fixed instead?

I caught this while running the testsuite for the arm-wince gdbserver 
port I am working on.

Cheers,
Pedro Alves

---

2006-11-16  Pedro Alves  <pedro_alves@portugalmail.pt>

    * parse.c (write_exp_msymbol): Check if SYMBOL_BFD_SECTION (msymbol) 
is NULL
    before dereferencing it.


[-- Attachment #2: nodeb.diff --]
[-- Type: text/plain, Size: 457 bytes --]

--- parse.c.org	2006-11-16 20:37:10.000000000 +0000
+++ parse.c	2006-11-16 00:19:52.000000000 +0000
@@ -408,7 +408,8 @@ write_exp_msymbol (struct minimal_symbol
 
   write_exp_elt_opcode (OP_LONG);
 
-  if (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL)
+  if (SYMBOL_BFD_SECTION (msymbol)
+      && (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL))
     {
       bfd *bfd = SYMBOL_BFD_SECTION (msymbol)->owner;
       struct objfile *ofp;

Re: Crash in write_exp_msymbol for coff targets.

[Daniel Jacobowitz]

On Thu, Nov 16, 2006 at 08:53:01PM +0000, Pedro Alves wrote:
> Hi all,
> 
> The TLS without debugging info support introduced a bug for coff based 
> targets.
> While printing for example a global symbol's value I am getting a 
> segfault in parse.c:write_exp_msymbol,
> at:
>  if (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL)
> 
> The problem is that minimal symbols may not have a bfd section set.
> 
> The attached patch fixes it, but is it correct?
> I see in coffread.c, that prim_record_minimal_symbol_and_info is always 
> called with a NULL
> bfd section, whilst in elfread.c, is is not. Is this a limitation of the 
> coff format? Should coffread.c
> be fixed instead?

Honestly, I'm not quite sure.  You've got the section index, so maybe
in prim_record_minimal_symbol_and_info it could fill in a NULL
bfd_section from the objfile sections table?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Crash in write_exp_msymbol for coff targets.

[Pedro Alves]

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

Daniel Jacobowitz wrote:
> On Thu, Nov 16, 2006 at 08:53:01PM +0000, Pedro Alves wrote:
>> Hi all,
>>
>> The TLS without debugging info support introduced a bug for coff based 
>> targets.
>> While printing for example a global symbol's value I am getting a 
>> segfault in parse.c:write_exp_msymbol,
>> at:
>>  if (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL)
>>
>> The problem is that minimal symbols may not have a bfd section set.
>>
>> The attached patch fixes it, but is it correct?
>> I see in coffread.c, that prim_record_minimal_symbol_and_info is always 
>> called with a NULL
>> bfd section, whilst in elfread.c, is is not. Is this a limitation of the 
>> coff format? Should coffread.c
>> be fixed instead?
> 
> Honestly, I'm not quite sure.  You've got the section index, so maybe
> in prim_record_minimal_symbol_and_info it could fill in a NULL
> bfd_section from the objfile sections table?
> 


Like in the attached patch1.diff?

Or, it isn't safe to index the objfile->sections by section index,
and we have to look them up linearly? That is what patch2.diff does.
In that version, I've repeated the search on coffread.c, caching the last
section looked up. Only slightly tested, but I got around around 50% cache
hit on a few exes. (Premature optimization?)

If v1 is the way to go, do we still need both 'int section' and
'asection *bfd_section' passed in to prim_record_minimal_symbol_and_info?

Both versions also fix the segfault that started this thread.

Cheers,
Pedro Alves

---

patch v1

2006-11-16 Pedro Alves <pedro_alves@portugalmail.pt>

* minsyms.c (prim_record_minimal_symbol_and_info): If bfd_section
is NULL, get it from the objfile sections table.

---

patch v2

2006-11-16 Pedro Alves <pedro_alves@portugalmail.pt>

* minsyms.c (prim_record_minimal_symbol_and_info): If bfd_section
is NULL, get it from the objfile sections table.
* coffread.c (coff_symtab_read): Get the bfd_section from the
objfile sections table, caching the result.


[-- Attachment #2: patch1.diff --]
[-- Type: text/plain, Size: 588 bytes --]

Index: minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.47
diff -u -p -r1.47 minsyms.c
--- minsyms.c	17 Oct 2006 20:17:44 -0000	1.47
+++ minsyms.c	16 Nov 2006 23:29:53 -0000
@@ -679,6 +679,8 @@ prim_record_minimal_symbol_and_info (con
 
   SYMBOL_VALUE_ADDRESS (msymbol) = address;
   SYMBOL_SECTION (msymbol) = section;
+  if (bfd_section == NULL)
+    bfd_section = objfile->sections[section].the_bfd_section;
   SYMBOL_BFD_SECTION (msymbol) = bfd_section;
 
   MSYMBOL_TYPE (msymbol) = ms_type;

[-- Attachment #3: patch2.diff --]
[-- Type: text/plain, Size: 2169 bytes --]

Index: minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.47
diff -u -p -r1.47 minsyms.c
--- minsyms.c	17 Oct 2006 20:17:44 -0000	1.47
+++ minsyms.c	16 Nov 2006 23:16:22 -0000
@@ -679,6 +679,18 @@ prim_record_minimal_symbol_and_info (con
 
   SYMBOL_VALUE_ADDRESS (msymbol) = address;
   SYMBOL_SECTION (msymbol) = section;
+  if (bfd_section == NULL)
+    {
+      struct obj_section *osec;
+      ALL_OBJFILE_OSECTIONS (objfile, osec)
+        {
+          if (osec->the_bfd_section->index == section)
+            {
+              bfd_section = osec->the_bfd_section;
+              break;
+            }
+        }
+    }
   SYMBOL_BFD_SECTION (msymbol) = bfd_section;
 
   MSYMBOL_TYPE (msymbol) = ms_type;
Index: coffread.c
===================================================================
RCS file: /cvs/src/src/gdb/coffread.c,v
retrieving revision 1.63
diff -u -p -r1.63 coffread.c
--- coffread.c	17 Dec 2005 22:33:59 -0000	1.63
+++ coffread.c	16 Nov 2006 23:16:25 -0000
@@ -699,6 +699,9 @@ coff_symtab_read (long symtab_offset, un
   long fcn_line_ptr = 0;
   int val;
   CORE_ADDR tmpaddr;
+  /* Avoid unnecessary lookups.  */
+  int cached_sec = -1;
+  struct bfd_section* cached_bfd_section = NULL;
 
   /* Work around a stdio bug in SunOS4.1.1 (this makes me nervous....
      it's hard to know I've really worked around it.  The fix should be
@@ -926,9 +929,22 @@ coff_symtab_read (long symtab_offset, un
 	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
 	      {
 		struct minimal_symbol *msym;
+		if (cached_sec != sec)
+		  {
+		    struct obj_section *osec;
+		    cached_sec = sec;
+		    ALL_OBJFILE_OSECTIONS (objfile, osec)
+		      {
+		        if (osec->the_bfd_section->index == sec)
+		          {
+		            cached_bfd_section = osec->the_bfd_section;
+		            break;
+		          }
+		      }
+		  }
 		msym = prim_record_minimal_symbol_and_info
 		  (cs->c_name, tmpaddr, ms_type, NULL,
-		   sec, NULL, objfile);
+		   sec, cached_bfd_section, objfile);
 		if (msym)
 		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
 	      }

Re: Crash in write_exp_msymbol for coff targets.

[Daniel Jacobowitz]

On Thu, Nov 16, 2006 at 11:39:38PM +0000, Pedro Alves wrote:
> Like in the attached patch1.diff?
> 
> Or, it isn't safe to index the objfile->sections by section index,
> and we have to look them up linearly? That is what patch2.diff does.
> In that version, I've repeated the search on coffread.c, caching the last
> section looked up. Only slightly tested, but I got around around 50% cache
> hit on a few exes. (Premature optimization?)

I'm somewhat worried about the numbering :-(  It looks like "int section"
is only useful for ANOFFSET / struct section_offsets.  And that
suggests there's no useful way to get from those numbers to the
bfd_section or vice versa.  What an awful mess.

I suppose the only way to fix this will be to overhaul the associated
code and reduce the number of numberings in use.  But in the mean time,
we should use your original patch that checked for non-NULL.  Sorry
for the runaround.

Shall I commit it for you?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Crash in write_exp_msymbol for coff targets.

[Pedro Alves]

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

Daniel Jacobowitz wrote:
> On Thu, Nov 16, 2006 at 11:39:38PM +0000, Pedro Alves wrote:
>   
>> Like in the attached patch1.diff?
>>
>> Or, it isn't safe to index the objfile->sections by section index,
>> and we have to look them up linearly? That is what patch2.diff does.
>> In that version, I've repeated the search on coffread.c, caching the last
>> section looked up. Only slightly tested, but I got around around 50% cache
>> hit on a few exes. (Premature optimization?)
>>     
>
> I'm somewhat worried about the numbering :-(  It looks like "int section"
> is only useful for ANOFFSET / struct section_offsets.  And that
> suggests there's no useful way to get from those numbers to the
> bfd_section or vice versa.  What an awful mess.
>
>   
I see. What about this? (Attached)

2006-11-16 Pedro Alves <pedro_alves@portugalmail.pt>

* coffread.c (cs_to_bfd_section): New function.
(coff_symtab_read): Use cs_to_bfd_section.

cs_to_section could be then converted to bfd_section_to_section.

> I suppose the only way to fix this will be to overhaul the associated
> code and reduce the number of numberings in use.  But in the mean time,
> we should use your original patch that checked for non-NULL.  Sorry
> for the runaround.
>
>   
No prob.

> Shall I commit it for you?
>
>   
Yes please, no write access.

Cheers,
Pedro Alves


[-- Attachment #2: patch3.diff --]
[-- Type: text/plain, Size: 1295 bytes --]

Index: coffread.c
===================================================================
RCS file: /cvs/src/src/gdb/coffread.c,v
retrieving revision 1.63
diff -u -p -r1.63 coffread.c
--- coffread.c	17 Dec 2005 22:33:59 -0000	1.63
+++ coffread.c	17 Nov 2006 00:45:46 -0000
@@ -284,6 +284,19 @@ cs_to_section (struct coff_symbol *cs, s
   return off;
 }
 
+/* Return the bfd_section that CS points to.  */
+static struct bfd_section*
+cs_to_bfd_section (struct coff_symbol *cs, struct objfile *objfile)
+{
+  asection *sect = NULL;
+  struct find_targ_sec_arg args;
+
+  args.targ_index = cs->c_secnum;
+  args.resultp = &sect;
+  bfd_map_over_sections (objfile->obfd, find_targ_sec, &args);
+  return sect;
+}
+
 /* Return the address of the section of a COFF symbol.  */
 
 static CORE_ADDR cs_section_address (struct coff_symbol *, bfd *);
@@ -926,9 +939,10 @@ coff_symtab_read (long symtab_offset, un
 	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
 	      {
 		struct minimal_symbol *msym;
+		struct bfd_section *bfd_section = cs_to_bfd_section (cs, objfile);
 		msym = prim_record_minimal_symbol_and_info
 		  (cs->c_name, tmpaddr, ms_type, NULL,
-		   sec, NULL, objfile);
+		   sec, bfd_section, objfile);
 		if (msym)
 		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
 	      }

Re: Crash in write_exp_msymbol for coff targets.

[Pedro Alves]

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

Pedro Alves wrote: 
> I see. What about this? (Attached)
>
> 2006-11-16 Pedro Alves <pedro_alves@portugalmail.pt>
>
> * coffread.c (cs_to_bfd_section): New function.
> (coff_symtab_read): Use cs_to_bfd_section.
>
> cs_to_section could be then converted to bfd_section_to_section.

Humm, not complete. This should be:

2006-11-16 Pedro Alves <pedro_alves@portugalmail.pt>

* symtab.h (prim_record_minimal_symbol_and_bfd_section): Declare.
* minsyms.c (prim_record_minimal_symbol_and_bfd_section): Rename from 
prim_record_minimal_symbol
and add bfd_section parameter.
(prim_record_minimal_symbol): New version; wraps 
prim_record_minimal_symbol_and_bfd_section.
* coffread.c (cs_to_bfd_section): New function.
(coff_symtab_read): Use cs_to_bfd_section.

Cheers,
Pedro Alves


[-- Attachment #2: patch4.diff --]
[-- Type: text/plain, Size: 4526 bytes --]

Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.98
diff -u -p -r1.98 symtab.h
--- symtab.h	17 Oct 2006 20:17:45 -0000	1.98
+++ symtab.h	17 Nov 2006 01:12:14 -0000
@@ -1147,6 +1147,12 @@ extern struct minimal_symbol *prim_recor
    enum minimal_symbol_type,
    char *info, int section, asection * bfd_section, struct objfile *);
 
+extern void prim_record_minimal_symbol_and_bfd_section
+  (const char *name, CORE_ADDR address,
+   enum minimal_symbol_type ms_type,
+   asection *bfd_section,
+   struct objfile *objfile);
+
 extern unsigned int msymbol_hash_iw (const char *);
 
 extern unsigned int msymbol_hash (const char *);
Index: minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.47
diff -u -p -r1.47 minsyms.c
--- minsyms.c	17 Oct 2006 20:17:44 -0000	1.47
+++ minsyms.c	17 Nov 2006 01:12:15 -0000
@@ -605,8 +605,9 @@ init_minimal_symbol_collection (void)
 }
 
 void
-prim_record_minimal_symbol (const char *name, CORE_ADDR address,
+prim_record_minimal_symbol_and_bfd_section (const char *name, CORE_ADDR address,
 			    enum minimal_symbol_type ms_type,
+			    asection *bfd_section,
 			    struct objfile *objfile)
 {
   int section;
@@ -631,9 +632,18 @@ prim_record_minimal_symbol (const char *
     }
 
   prim_record_minimal_symbol_and_info (name, address, ms_type,
-				       NULL, section, NULL, objfile);
+				       NULL, section, bfd_section, objfile);
 }
 
+void
+prim_record_minimal_symbol (const char *name, CORE_ADDR address,
+                                        enum minimal_symbol_type ms_type,
+                                        struct objfile *objfile)
+{
+    prim_record_minimal_symbol_and_bfd_section (name, address, ms_type, NULL, objfile);
+}
+
+
 /* Record a minimal symbol in the msym bunches.  Returns the symbol
    newly created.  */
 
Index: coffread.c
===================================================================
RCS file: /cvs/src/src/gdb/coffread.c,v
retrieving revision 1.63
diff -u -p -r1.63 coffread.c
--- coffread.c	17 Dec 2005 22:33:59 -0000	1.63
+++ coffread.c	17 Nov 2006 01:12:18 -0000
@@ -284,6 +284,19 @@ cs_to_section (struct coff_symbol *cs, s
   return off;
 }
 
+/* Return the bfd_section that CS points to.  */
+static struct bfd_section*
+cs_to_bfd_section (struct coff_symbol *cs, struct objfile *objfile)
+{
+  asection *sect = NULL;
+  struct find_targ_sec_arg args;
+
+  args.targ_index = cs->c_secnum;
+  args.resultp = &sect;
+  bfd_map_over_sections (objfile->obfd, find_targ_sec, &args);
+  return sect;
+}
+
 /* Return the address of the section of a COFF symbol.  */
 
 static CORE_ADDR cs_section_address (struct coff_symbol *, bfd *);
@@ -410,13 +423,14 @@ coff_end_symtab (struct objfile *objfile
 \f
 static void
 record_minimal_symbol (char *name, CORE_ADDR address,
-		       enum minimal_symbol_type type, struct objfile *objfile)
+		       enum minimal_symbol_type type, asection *bfd_section, 
+		       struct objfile *objfile)
 {
   /* We don't want TDESC entry points in the minimal symbol table */
   if (name[0] == '@')
     return;
 
-  prim_record_minimal_symbol (name, address, type, objfile);
+  prim_record_minimal_symbol_and_bfd_section (name, address, type, bfd_section, objfile);
 }
 \f
 /* coff_symfile_init ()
@@ -761,9 +775,11 @@ coff_symtab_read (long symtab_offset, un
       /* Typedefs should not be treated as symbol definitions.  */
       if (ISFCN (cs->c_type) && cs->c_sclass != C_TPDEF)
 	{
+	  struct bfd_section *bfd_section = cs_to_bfd_section (cs, objfile);
+
 	  /* Record all functions -- external and static -- in minsyms. */
 	  tmpaddr = cs->c_value + ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
-	  record_minimal_symbol (cs->c_name, tmpaddr, mst_text, objfile);
+	  record_minimal_symbol (cs->c_name, tmpaddr, mst_text, bfd_section, objfile);
 
 	  fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr;
 	  fcn_start_addr = tmpaddr;
@@ -926,9 +942,10 @@ coff_symtab_read (long symtab_offset, un
 	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
 	      {
 		struct minimal_symbol *msym;
+		struct bfd_section *bfd_section = cs_to_bfd_section (cs, objfile);
 		msym = prim_record_minimal_symbol_and_info
 		  (cs->c_name, tmpaddr, ms_type, NULL,
-		   sec, NULL, objfile);
+		   sec, bfd_section, objfile);
 		if (msym)
 		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
 	      }

Re: Crash in write_exp_msymbol for coff targets.

[Pedro Alves]

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

I realize I just dumped the previous patch without explaining it,
so maybe this will help reducing the workload from Daniel,
or maybe even finding a new reviewer (yes, that's you! :) ).

In a nutshell:
In coff based targets, there is a new segfault in
parse.c:write_exp_msymbol,
at:
  if (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL)

Easily triggered by just issuing p.ex.:
p globalvar

The problem is that minimal symbols may have a bfd_section set to
NULL at this point. (SYMBOL_BFD_SECTION (msymbol) == NULL).

This segfault doesn't happen in elf targets, because in elfread.c,
prim_record_minimal_symbol_and_info is always called with a non-NULL bfd_section*,
effectively always creating a minimal symbol with a bfd_section set.

In coffread.c, prim_record_minimal_symbol_and_info is always called with
bfd_section == NULL.

The attached patch, (functionally equivalent to the previous one,
with just a small cleanup), makes the coff reader match
the bfd_section from the coff_symbol, using the symbols' section number
and bfd_map_over_sections. This matching was already done in the
existing code, in cs_to_section, so it should be correct,
unlike the previous versions that used objfile->sections.

A few other functions are then adjusted to be able to pass bfd_section while
preserving the rest of the existing behavior.

I don't thing that the original workaround in the beginning of this thread ...:
- if (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL)
+ if (SYMBOL_BFD_SECTION (msymbol)
+     && (SYMBOL_BFD_SECTION (msymbol)->flags & SEC_THREAD_LOCAL))

... should still be applied. Having it segfault there for other
formats and fix them accordingly would be better, than hiding
the real bug, IMHO.


Hope this helped,

Cheers,
Pedro Alves

---

2006-11-18 Pedro Alves <pedro_alves@portugalmail.pt>

* symtab.h (prim_record_minimal_symbol_and_bfd_section): Declare.
* minsyms.c (prim_record_minimal_symbol_and_bfd_section): Rename from prim_record_minimal_symbol
and add bfd_section parameter.
(prim_record_minimal_symbol): New version; wraps prim_record_minimal_symbol_and_bfd_section.
* coffread.c (cs_to_bfd_section): New function.
(cs_to_section): Use cs_to_bfd_section.
(record_minimal_symbol): Add bfd_section parameter. Call prim_record_minimal_symbol_and_bfd_section.
(coff_symtab_read): Use cs_to_bfd_section.


[-- Attachment #2: patch5.diff --]
[-- Type: text/plain, Size: 4929 bytes --]

Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.98
diff -u -p -r1.98 symtab.h
--- symtab.h	17 Oct 2006 20:17:45 -0000	1.98
+++ symtab.h	18 Nov 2006 01:02:57 -0000
@@ -1147,6 +1147,12 @@ extern struct minimal_symbol *prim_recor
    enum minimal_symbol_type,
    char *info, int section, asection * bfd_section, struct objfile *);
 
+extern void prim_record_minimal_symbol_and_bfd_section
+  (const char *name, CORE_ADDR address,
+   enum minimal_symbol_type ms_type,
+   asection *bfd_section,
+   struct objfile *objfile);
+
 extern unsigned int msymbol_hash_iw (const char *);
 
 extern unsigned int msymbol_hash (const char *);
Index: minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.47
diff -u -p -r1.47 minsyms.c
--- minsyms.c	17 Oct 2006 20:17:44 -0000	1.47
+++ minsyms.c	18 Nov 2006 01:02:58 -0000
@@ -605,8 +605,9 @@ init_minimal_symbol_collection (void)
 }
 
 void
-prim_record_minimal_symbol (const char *name, CORE_ADDR address,
+prim_record_minimal_symbol_and_bfd_section (const char *name, CORE_ADDR address,
 			    enum minimal_symbol_type ms_type,
+			    asection *bfd_section,
 			    struct objfile *objfile)
 {
   int section;
@@ -631,9 +632,18 @@ prim_record_minimal_symbol (const char *
     }
 
   prim_record_minimal_symbol_and_info (name, address, ms_type,
-				       NULL, section, NULL, objfile);
+				       NULL, section, bfd_section, objfile);
 }
 
+void
+prim_record_minimal_symbol (const char *name, CORE_ADDR address,
+                                        enum minimal_symbol_type ms_type,
+                                        struct objfile *objfile)
+{
+  prim_record_minimal_symbol_and_bfd_section (name, address, ms_type, NULL, objfile);
+}
+
+
 /* Record a minimal symbol in the msym bunches.  Returns the symbol
    newly created.  */
 
Index: coffread.c
===================================================================
RCS file: /cvs/src/src/gdb/coffread.c,v
retrieving revision 1.63
diff -u -p -r1.63 coffread.c
--- coffread.c	17 Dec 2005 22:33:59 -0000	1.63
+++ coffread.c	18 Nov 2006 01:03:01 -0000
@@ -259,17 +259,25 @@ find_targ_sec (bfd *abfd, asection *sect
     *args->resultp = sect;
 }
 
-/* Return the section number (SECT_OFF_*) that CS points to.  */
-static int
-cs_to_section (struct coff_symbol *cs, struct objfile *objfile)
+/* Return the bfd_section that CS points to.  */
+static struct bfd_section*
+cs_to_bfd_section (struct coff_symbol *cs, struct objfile *objfile)
 {
   asection *sect = NULL;
   struct find_targ_sec_arg args;
-  int off = SECT_OFF_TEXT (objfile);
 
   args.targ_index = cs->c_secnum;
   args.resultp = &sect;
   bfd_map_over_sections (objfile->obfd, find_targ_sec, &args);
+  return sect;
+}
+
+/* Return the section number (SECT_OFF_*) that CS points to.  */
+static int
+cs_to_section (struct coff_symbol *cs, struct objfile *objfile)
+{
+  int off = SECT_OFF_TEXT (objfile);
+  asection *sect = cs_to_bfd_section (cs, objfile);
   if (sect != NULL)
     {
       /* This is the section.  Figure out what SECT_OFF_* code it is.  */
@@ -410,13 +418,14 @@ coff_end_symtab (struct objfile *objfile
 \f
 static void
 record_minimal_symbol (char *name, CORE_ADDR address,
-		       enum minimal_symbol_type type, struct objfile *objfile)
+		       enum minimal_symbol_type type, asection *bfd_section, 
+		       struct objfile *objfile)
 {
   /* We don't want TDESC entry points in the minimal symbol table */
   if (name[0] == '@')
     return;
 
-  prim_record_minimal_symbol (name, address, type, objfile);
+  prim_record_minimal_symbol_and_bfd_section (name, address, type, bfd_section, objfile);
 }
 \f
 /* coff_symfile_init ()
@@ -761,9 +770,11 @@ coff_symtab_read (long symtab_offset, un
       /* Typedefs should not be treated as symbol definitions.  */
       if (ISFCN (cs->c_type) && cs->c_sclass != C_TPDEF)
 	{
+	  struct bfd_section *bfd_section = cs_to_bfd_section (cs, objfile);
+
 	  /* Record all functions -- external and static -- in minsyms. */
 	  tmpaddr = cs->c_value + ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
-	  record_minimal_symbol (cs->c_name, tmpaddr, mst_text, objfile);
+	  record_minimal_symbol (cs->c_name, tmpaddr, mst_text, bfd_section, objfile);
 
 	  fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr;
 	  fcn_start_addr = tmpaddr;
@@ -926,9 +937,10 @@ coff_symtab_read (long symtab_offset, un
 	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
 	      {
 		struct minimal_symbol *msym;
+		struct bfd_section *bfd_section = cs_to_bfd_section (cs, objfile);
 		msym = prim_record_minimal_symbol_and_info
 		  (cs->c_name, tmpaddr, ms_type, NULL,
-		   sec, NULL, objfile);
+		   sec, bfd_section, objfile);
 		if (msym)
 		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
 	      }

Re: Crash in write_exp_msymbol for coff targets.

[Daniel Jacobowitz]

On Fri, Nov 17, 2006 at 01:13:14AM +0000, Pedro Alves wrote:
> * symtab.h (prim_record_minimal_symbol_and_bfd_section): Declare.

I don't think you need yet another variant of this function.  Can't you
use prim_record_minimal_symbol_and_info?  You're getting the section
from the cs, and there's cs_to_section, so you ought to have the
"section" integer available too.


-- 
Daniel Jacobowitz
CodeSourcery

Re: Crash in write_exp_msymbol for coff targets.

[Pedro Alves]

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

Daniel Jacobowitz escreveu:
> On Fri, Nov 17, 2006 at 01:13:14AM +0000, Pedro Alves wrote:
>   
>> * symtab.h (prim_record_minimal_symbol_and_bfd_section): Declare.
>>     
>
> I don't think you need yet another variant of this function.  Can't you
> use prim_record_minimal_symbol_and_info?  You're getting the section
> from the cs, and there's cs_to_section, so you ought to have the
> "section" integer available too.
>
>
>   
You are right. My thinking was that prim_record_minimal_symbol will only 
pass
.text .data or .bss section to prim_record_minimal_symbol_and_info, and
the way you say, and in the patch attached, we are passing a wider range 
of values.
I didn't want to change more than needed, but looking again, I don't 
think it harms
in this case.

The attached patch does that, and also, I took the opportunity to a 
little extra cleanup
on coffread.c by making all minimal symbol creations through 
record_minimal_symbol.

Cheers,
Pedro Alves

---

2006-11-19  Pedro Alves  <pedro_alves@portugalmail.pt>

        * coffread.c (cs_to_bfd_section): New function.
        (cs_to_section): Use cs_to_bfd_section.
        (record_minimal_symbol): Take the coff_symbol* parameter instead
        of the symbol's name as a char*.
        Add 'int section' parameter. Call 
prim_record_minimal_symbol_and_info
        instead of prim_record_minimal_symbol_and_info.
        Change return type to struct minimal_symbol *.
        (coff_symtab_read): Adapt to new record_minimal_symbol's signature.
        Make all minimal symbol creations go through record_minimal_symbol.


[-- Attachment #2: patch6.diff --]
[-- Type: text/plain, Size: 3341 bytes --]

Index: coffread.c
===================================================================
RCS file: /cvs/src/src/gdb/coffread.c,v
retrieving revision 1.63
diff -u -p -r1.63 coffread.c
--- coffread.c	17 Dec 2005 22:33:59 -0000	1.63
+++ coffread.c	19 Nov 2006 04:00:59 -0000
@@ -259,17 +259,25 @@ find_targ_sec (bfd *abfd, asection *sect
     *args->resultp = sect;
 }
 
-/* Return the section number (SECT_OFF_*) that CS points to.  */
-static int
-cs_to_section (struct coff_symbol *cs, struct objfile *objfile)
+/* Return the bfd_section that CS points to.  */
+static struct bfd_section*
+cs_to_bfd_section (struct coff_symbol *cs, struct objfile *objfile)
 {
   asection *sect = NULL;
   struct find_targ_sec_arg args;
-  int off = SECT_OFF_TEXT (objfile);
 
   args.targ_index = cs->c_secnum;
   args.resultp = &sect;
   bfd_map_over_sections (objfile->obfd, find_targ_sec, &args);
+  return sect;
+}
+
+/* Return the section number (SECT_OFF_*) that CS points to.  */
+static int
+cs_to_section (struct coff_symbol *cs, struct objfile *objfile)
+{
+  asection *sect = cs_to_bfd_section (cs, objfile);
+  int off = SECT_OFF_TEXT (objfile);
   if (sect != NULL)
     {
       /* This is the section.  Figure out what SECT_OFF_* code it is.  */
@@ -408,15 +416,19 @@ coff_end_symtab (struct objfile *objfile
   last_source_file = NULL;
 }
 \f
-static void
-record_minimal_symbol (char *name, CORE_ADDR address,
-		       enum minimal_symbol_type type, struct objfile *objfile)
+static struct minimal_symbol *
+record_minimal_symbol (struct coff_symbol* cs, CORE_ADDR address,
+		       enum minimal_symbol_type type, int section, 
+		       struct objfile *objfile)
 {
+  struct bfd_section *bfd_section;
   /* We don't want TDESC entry points in the minimal symbol table */
-  if (name[0] == '@')
-    return;
+  if (cs->c_name[0] == '@')
+    return NULL;
 
-  prim_record_minimal_symbol (name, address, type, objfile);
+  bfd_section = cs_to_bfd_section (cs, objfile);
+  return prim_record_minimal_symbol_and_info (cs->c_name, address, type,
+    NULL, section, bfd_section, objfile);
 }
 \f
 /* coff_symfile_init ()
@@ -762,8 +774,9 @@ coff_symtab_read (long symtab_offset, un
       if (ISFCN (cs->c_type) && cs->c_sclass != C_TPDEF)
 	{
 	  /* Record all functions -- external and static -- in minsyms. */
+	  int section = cs_to_section (cs, objfile);
 	  tmpaddr = cs->c_value + ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
-	  record_minimal_symbol (cs->c_name, tmpaddr, mst_text, objfile);
+	  record_minimal_symbol (cs, tmpaddr, mst_text, section, objfile);
 
 	  fcn_line_ptr = main_aux.x_sym.x_fcnary.x_fcn.x_lnnoptr;
 	  fcn_start_addr = tmpaddr;
@@ -923,15 +936,13 @@ coff_symtab_read (long symtab_offset, un
 		  ms_type = mst_unknown;
 	      }
 
-	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
-	      {
-		struct minimal_symbol *msym;
-		msym = prim_record_minimal_symbol_and_info
-		  (cs->c_name, tmpaddr, ms_type, NULL,
-		   sec, NULL, objfile);
-		if (msym)
-		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
-	      }
+	    {
+	      struct minimal_symbol *msym;
+	      msym = record_minimal_symbol (cs, tmpaddr, ms_type, sec, objfile);
+	      if (msym)
+	        COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
+	    }
+
 	    if (SDB_TYPE (cs->c_type))
 	      {
 		struct symbol *sym;

Re: Crash in write_exp_msymbol for coff targets.

[Daniel Jacobowitz]

On Sun, Nov 19, 2006 at 04:05:33AM +0000, Pedro Alves wrote:
> The attached patch does that, and also, I took the opportunity to a 
> little extra cleanup
> on coffread.c by making all minimal symbol creations through 
> record_minimal_symbol.

Looks good to me; I committed it on HEAD.  I made only one minor
formatting change (struct coff_symbol* cs versus struct coff_symbol *cs).

Thanks!

-- 
Daniel Jacobowitz
CodeSourcery

Re: Crash in write_exp_msymbol for coff targets.

[Daniel Jacobowitz]

On Thu, Nov 16, 2006 at 08:53:01PM +0000, Pedro Alves wrote:
> 2006-11-16  Pedro Alves  <pedro_alves@portugalmail.pt>
> 
>    * parse.c (write_exp_msymbol): Check if SYMBOL_BFD_SECTION (msymbol) 
> is NULL
>    before dereferencing it.

While I think we should fix coffread to set this too, meanwhile, let's
avoid the crash.  I've checked this in.  Careful of wrapping in the
ChangeLog entry; that line was too long.  And thanks for the patch!

-- 
Daniel Jacobowitz
CodeSourcery

Re: Crash in write_exp_msymbol for coff targets.

[Joel Brobecker]

Hello,

> > 2006-11-16  Pedro Alves  <pedro_alves@portugalmail.pt>
> > 
> >    * parse.c (write_exp_msymbol): Check if SYMBOL_BFD_SECTION (msymbol) 
> > is NULL
> >    before dereferencing it.
> 
> While I think we should fix coffread to set this too, meanwhile, let's
> avoid the crash.  I've checked this in.  Careful of wrapping in the
> ChangeLog entry; that line was too long.  And thanks for the patch!

I also checked this patch in the branch. It's obvious enough and
avoids a crash.

-- 
Joel