Re: Crash in write_exp_msymbol for coff targets.

[Pedro Alves <pedro_alves@portugalmail.pt>]

[-- 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);
 	      }