PR4886: weaken build-id failure semantics

6 files changed, 56 insertions, 49 deletions

diff --git a/runtime/ChangeLog b/runtime/ChangeLog
index 96b37dbd..f40ff6ee 100644
--- a/runtime/ChangeLog
+++ b/runtime/ChangeLog
@@ -1,3 +1,10 @@
+2008-10-07  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 4886.
+	* sym.c (_stp_module_check): Tweak & simplify build-id checking.
+	Weaken consequences of mismatch temporarily due to fedora bug.
+	* sym.h: Eliminate special cased values of module->notes_sect.
+
 2008-10-06  Mark Wielaard  <mjw@redhat.com>
 
 	* utrace_compatibility.h: Add workaround for fedora 9 2.6.26 kernels.
diff --git a/runtime/sym.c b/runtime/sym.c
index 97243929..d7b079d2 100644
--- a/runtime/sym.c
+++ b/runtime/sym.c
@@ -177,21 +177,10 @@ static int _stp_module_check(void)
 
 	for (i = 0; i < _stp_num_modules; i++)
 	{
-		m = _stp_modules[i];			
-
-		/* unloaded module */
-		if (m->notes_sect == 0) {
-              	     _stp_warn("skip checking %s\n", m->name);
-                    continue;
-                }
-		if (m->build_id_len > 0) { /* build-id in debuginfo file */
-		    dbug_sym(1, "validate %s based on build-id\n", m->name);
-
-		    /* loaded module/kernel, but without build-id */
-		    if (m->notes_sect == 1) {
-		   	_stp_error("missing build-id in %s\n", m->name);
-			return 1;
-		    } 
+		m = _stp_modules[i];
+		if (m->build_id_len > 0 && m->notes_sect != 0) {
+		    dbug_sym(1, "build-id validation [%s]\n", m->name);
+
 		    /* notes end address */
 		    if (!strcmp(m->name, "kernel")) {
 		  	  notes_addr = m->build_id_offset;
@@ -201,28 +190,33 @@ static int _stp_module_check(void)
 			  notes_addr = m->notes_sect + m->build_id_offset;
 			  base_addr = m->notes_sect;
 		    }
-		    /* notes start address */
+
+		    /* build-id note payload start address */
+                    /* XXX: But see https://bugzilla.redhat.com/show_bug.cgi?id=465872;
+                       dwfl_module_build_id was not intended to return the end address. */
 		    notes_addr -= m->build_id_len;
+
 		    if (notes_addr > base_addr) {
 		      for (j = 0; j < m->build_id_len; j++)	 
-		        if (*((unsigned char *) notes_addr+j) != 
-							*(m->build_id_bits+j)) 
-			{
-			  _stp_error("inconsistent bit (0x%x [%s] vs 0x%x [debuginfo]) of build-id\n", *((unsigned char *) notes_addr+j), m->name, *(m->build_id_bits+j));
-			  return 1;
+                        {
+                          unsigned char theory, practice;
+                          theory = m->build_id_bits [j];
+                          practice = ((unsigned char*) notes_addr) [j];
+                          /* XXX: consider using kread() instead of above. */
+                          if (theory != practice)
+                            {
+                              printk(KERN_WARNING
+                                     "%s: inconsistent %s build-id byte #%d "
+                                     "(0x%x [actual] vs. 0x%x [debuginfo])\n",
+                                     THIS_MODULE->name, m->name, j,
+                                     practice, theory);
+                              break; /* Note just the first mismatch. */
+                              /* XXX: If it were not for Fedora bug #465873,
+                                 we could "return 1;" here to abort the script. */
+                            }
 			} 
-		    } else { /* bug, shouldn't come here */
-			     _stp_error("unknown failure in checking %s\n", 
-								m->name);
-			     return 1;
-		    	   } /* end comparing */
-		} else { 
-			  /* build-id in module/kernel, absent in debuginfo */
-			  if (m->notes_sect > 1) {
-			    _stp_error("unexpected build-id in %s\n", m->name);
-			    return 1;
-			  }
-		 } /* end checking */
+		    }
+		} /* end checking */
 	} /* end loop */
 	return 0;
 }
diff --git a/runtime/sym.h b/runtime/sym.h
index d23c1632..9d6a4ded 100644
--- a/runtime/sym.h
+++ b/runtime/sym.h
@@ -48,12 +48,7 @@ struct _stp_module {
 	/* build-id information */
 	unsigned char *build_id_bits;
 	unsigned long  build_id_offset;
-	unsigned long  notes_sect; /* kernel: 1 - no build-id 
-				    *  	      2 - has build-id 
-				    * module: 0 - unloaded
-				    *	      1 - loaded and no build-id
-				    *	      Other - note section address  
-				   */
+	unsigned long  notes_sect;
 	int build_id_len;
 };
 
diff --git a/runtime/transport/ChangeLog b/runtime/transport/ChangeLog
index 42c6fc2a..02f73992 100644
--- a/runtime/transport/ChangeLog
+++ b/runtime/transport/ChangeLog
@@ -1,3 +1,9 @@
+2008-10-07  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 4886
+	* symbols.c (_stp_do_relocation): Simplify processing of build-id
+	note address.
+
 2008-09-17  Frank Ch. Eigler  <fche@elastic.org>
 
 	PR 6487, 6504.
diff --git a/runtime/transport/symbols.c b/runtime/transport/symbols.c
index faba0986..1cd78724 100644
--- a/runtime/transport/symbols.c
+++ b/runtime/transport/symbols.c
@@ -51,20 +51,19 @@ static void _stp_do_relocation(const char __user *buf, size_t count)
       if (strcmp (_stp_modules[mi]->name, msg.module))
         continue;
 
-      /* update note section to represent loaded */
-      if (_stp_modules[mi]->notes_sect == 0)
-	 _stp_modules[mi]->notes_sect = 1;	
+      if (!strcmp (".note.gnu.build-id", msg.reloc)) {
+        _stp_modules[mi]->notes_sect = msg.address;   /* cache this particular address  */
+      }
+
       for (si=0; si<_stp_modules[mi]->num_sections; si++)
         {
-  	   if (!strcmp (".note.gnu.build-id", msg.reloc)) {
-		_stp_modules[mi]->notes_sect = msg.address;	
-		continue;
-	   }
-
           if (strcmp (_stp_modules[mi]->sections[si].name, msg.reloc))
             continue;
-
-          _stp_modules[mi]->sections[si].addr = msg.address;
+          else
+            {
+              _stp_modules[mi]->sections[si].addr = msg.address;
+              break;
+            }
         } /* loop over sections */
     } /* loop over modules */
 }
diff --git a/translate.cxx b/translate.cxx
index 0ee51792..bbd8a01b 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -4423,6 +4423,8 @@ dump_unwindsyms (Dwfl_Module *m,
                                         (const unsigned char **)&build_id_bits,
                                          &build_id_vaddr)) > 0)
   {
+    /* XXX: But see https://bugzilla.redhat.com/show_bug.cgi?id=465872;
+       dwfl_module_build_id was not intended to return the end address. */
         if (c->session.verbose > 1) {
            clog << "Found build-id in " << name
                 << ", length " << build_id_len;
@@ -4608,6 +4610,10 @@ dump_unwindsyms (Dwfl_Module *m,
        c->output << "\", " << endl;
        c->output << ".build_id_len = " << build_id_len << ", " << endl;
 
+       /* XXX: kernel data boot-time relocation works differently from text.
+          This hack disables relocation altogether, but that's not necessarily
+          correct either.  We may instead need a relocation basis different
+          from _stext, such as __start_notes.  */
        if (modname == "kernel")
                 c->output << ".build_id_offset = 0x" << hex << build_id_vaddr
 			  << dec << ", " << endl;