Build problem with ToT GCC
diff mbox

Message ID 20150417213118.08ECE2C3B3B@topped-with-meat.com
State New
Headers show

Commit Message

Roland McGrath April 17, 2015, 9:31 p.m. UTC
> Weird, I assumed that the dl-close.c issue was the same as the dl-open.c
> problem.  But it looks different.  After cutting it down with delta I
> get the following small test case and error.  I do not see how GCC can
> know that nsid is not 0.

It's because of the call to a noreturn function ("bad"):

>   struct link_namespaces *ns = &_dl_ns[nsid];
>   (nsid != 0) ? (void) (0) : bad ("nsid != 0");
>   --ns->_ns_nloaded;

If 'nsid != 0' is not true, then you never reach the last line,
which is the only one actually dereferencing _dl_ns[nsid].

It's rather confusing that it only reports the error at the site of
the address calculation and does not show the site of the dereference,
let alone the site of the preceding code path that made the compiler
believe the value of 'nsid' was constrained (which is a handful of
lines earlier: 'assert (nsid != LM_ID_BASE)').  In your minimized
example is easy enough to see the relationship.  But in the original
code, 'ns = &_dl_ns[nsid]' appears over 500 lines before
'--ns->_ns_nloaded', which itself is several lines after the assert.
The message "subscript is outside" is also somewhat misleading for
this case, because the assert that testifies the value cannot be zero
is only in one branch of an if test--in the other branch, there is no
such assert and so the cited dereference might be just fine (which is
the case here, as it's dynamically impossible for the if test to fail,
for reasons nobody would expect the compiler to figure out).  The
ideal message would be something like "subscript is outside array
bounds in some code paths" followed by a "note: code paths passing
through here" for the line with the assert.  You should file a GCC bug
about improving the diagnostics for this case.

Please try this patch (branch roland/dl-nns):

	* elf/dl-close.c (_dl_close_worker) [DL_NNS == 1]: Just assert that
	IMAP->l_prev cannot be null, and #if out the code for the contrary
	case, avoiding 'assert (nsid != LM_ID_BASE)' making the compiler
	believe that NS (&_dl_ns[NSID]) could point outside the array.



Thanks,
Roland

Comments

Steve Ellcey April 17, 2015, 9:52 p.m. UTC | #1
On Fri, 2015-04-17 at 14:31 -0700, Roland McGrath wrote:

> Please try this patch (branch roland/dl-nns):
> 
> 	* elf/dl-close.c (_dl_close_worker) [DL_NNS == 1]: Just assert that
> 	IMAP->l_prev cannot be null, and #if out the code for the contrary
> 	case, avoiding 'assert (nsid != LM_ID_BASE)' making the compiler
> 	believe that NS (&_dl_ns[NSID]) could point outside the array.

This patch allowed me to compile elf/dl-close.c and my glibc build
completed without any other problems.

Steve Ellcey
sellcey@imgtec.com
Roland McGrath April 17, 2015, 10:01 p.m. UTC | #2
> This patch allowed me to compile elf/dl-close.c and my glibc build
> completed without any other problems.

Thanks.  I verified that it caused no 'make check' regressions for
me on x86_64-linux-gnu (GCC 4.8.2).  I've committed it now.


Thanks,
Roland

Patch
diff mbox

diff --git a/elf/dl-close.c b/elf/dl-close.c
index cf8f9e0..412f71d 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -641,9 +641,16 @@  _dl_close_worker (struct link_map *map)
 	  DL_UNMAP (imap);
 
 	  /* Finally, unlink the data structure and free it.  */
-	  if (imap->l_prev != NULL)
-	    imap->l_prev->l_next = imap->l_next;
-	  else
+#if DL_NNS == 1
+	  /* The assert in the (imap->l_prev == NULL) case gives
+	     the compiler license to warn that NS points outside
+	     the dl_ns array bounds in that case (as nsid != LM_ID_BASE
+	     is tantamount to nsid >= DL_NNS).  That should be impossible
+	     in this configuration, so just assert about it instead.  */
+	  assert (nsid == LM_ID_BASE);
+	  assert (imap->l_prev != NULL);
+#else
+	  if (imap->l_prev == NULL)
 	    {
 	      assert (nsid != LM_ID_BASE);
 	      ns->_ns_loaded = imap->l_next;
@@ -652,6 +659,9 @@  _dl_close_worker (struct link_map *map)
 		 we leave for debuggers to examine.  */
 	      r->r_map = (void *) ns->_ns_loaded;
 	    }
+	  else
+#endif
+	    imap->l_prev->l_next = imap->l_next;
 
 	  --ns->_ns_nloaded;
 	  if (imap->l_next != NULL)