diff mbox

[libgcc] : Avoid warning: array subscript is above array bounds when compiling crtstuff.c

Message ID CAFULd4YJqWYkEJjhE0aQOGHzAV496j44qkqy6wtDBvzoXGWLiQ@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak March 10, 2014, 5:42 p.m. UTC 
On Mon, Mar 10, 2014 at 11:27 AM, Uros Bizjak <ubizjak@gmail.com> wrote:

>>> > Well, better is non-obvious, while it is smaller (which is good for
>>> > initialization and thus rarely executed code), the common case is that
>>> > *jcr_list is 0 (gcj is used rarely these days) and for the common case it is
>>> > one instruction longer.
>>> > Perhaps at least use if (__builtin_expect (*jcr_list != NULL, 0))?
>>> > Otherwise looks good to me.
>>>
>>> Following source:
>>>
>>> void frame_dummy (void)
>>> {
>>>   void **jcr_list = __JCR_LIST__;
>>>   if (__builtin_expect (*jcr_list != 0, 0))
>>>     register_classes (jcr_list);
>>> }
>>>
>>> generates exactly the same code while avoiding the warning. So,
>>> following your concern, I am testing following patch:
>>
>> But then the asm is gone and it can start to break any time soon.
>> For GCC __JCR_LIST__ is simply a zero sized local array and thus
>> dereferencing it's first element is invalid.  It doesn't know that we use
>> linker magic to populate the array.
>
> OK, then we have to live with additional instruction (which isn't bad
> either, since the r/i compare can be fused with the cjump, where m/i
> compare can't be).
>
> I will bootstrap/regtest with additional __built_expect.

Attached patch is what I plan to commit to mainline soon. Ian, does it
look OK to you?

2014-03-10  Uros Bizjak  <ubizjak@gmail.com>

    * crtstuff.c (frame_dummy): Use void **jcr_list temporary
    variable to avoid array subscript is above array bounds warnings.
    Use __builtin_expect when checking *jcr_list for NULL.

Re-bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}
and alpha-linux-gnu.

Jakub, is it OK for mainline from RMs POV? Shall we backport this
patch to other release branches?

Uros.

Comments

Ian Lance Taylor March 10, 2014, 5:53 p.m. UTC | #1 
On Mon, Mar 10, 2014 at 10:42 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> Attached patch is what I plan to commit to mainline soon. Ian, does it
> look OK to you?
>
> 2014-03-10  Uros Bizjak  <ubizjak@gmail.com>
>
>     * crtstuff.c (frame_dummy): Use void **jcr_list temporary
>     variable to avoid array subscript is above array bounds warnings.
>     Use __builtin_expect when checking *jcr_list for NULL.

This looks OK to me.  Thanks.

> Jakub, is it OK for mainline from RMs POV? Shall we backport this
> patch to other release branches?

I gather that the purpose of this patch is to avoid a warning.  I
don't see a reason to backport this to working release branches.

Ian
Jakub Jelinek March 10, 2014, 5:54 p.m. UTC | #2 
On Mon, Mar 10, 2014 at 10:53:27AM -0700, Ian Lance Taylor wrote:
> On Mon, Mar 10, 2014 at 10:42 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > Attached patch is what I plan to commit to mainline soon. Ian, does it
> > look OK to you?
> >
> > 2014-03-10  Uros Bizjak  <ubizjak@gmail.com>
> >
> >     * crtstuff.c (frame_dummy): Use void **jcr_list temporary
> >     variable to avoid array subscript is above array bounds warnings.
> >     Use __builtin_expect when checking *jcr_list for NULL.
> 
> This looks OK to me.  Thanks.
> 
> > Jakub, is it OK for mainline from RMs POV? Shall we backport this
> > patch to other release branches?
> 
> I gather that the purpose of this patch is to avoid a warning.  I
> don't see a reason to backport this to working release branches.

Yeah.  Unless we are aware of miscompilation on some target, I think it
shouldn't be backported.

	Jakub
diff mbox

Patch

Index: crtstuff.c
===================================================================
--- crtstuff.c	(revision 208448)
+++ crtstuff.c	(working copy)
@@ -460,12 +460,14 @@  frame_dummy (void)
 #endif /* USE_EH_FRAME_REGISTRY */
 
 #ifdef JCR_SECTION_NAME
-  if (__JCR_LIST__[0])
+  void **jcr_list;
+  __asm ("" : "=g" (jcr_list) : "0" (__JCR_LIST__));
+  if (__builtin_expect (*jcr_list != NULL, 0))
     {
       void (*register_classes) (void *) = _Jv_RegisterClasses;
       __asm ("" : "+r" (register_classes));
       if (register_classes)
-	register_classes (__JCR_LIST__);
+	register_classes (jcr_list);
     }
 #endif /* JCR_SECTION_NAME */
 
@@ -565,12 +567,14 @@  __do_global_ctors_1(void)
 #endif
 
 #ifdef JCR_SECTION_NAME
-  if (__JCR_LIST__[0])
+  void **jcr_list
+  __asm ("" : "=g" (jcr_list) : "0" (__JCR_LIST__));
+  if (__builtin_expect (*jcr_list != NULL, 0))
     {
       void (*register_classes) (void *) = _Jv_RegisterClasses;
       __asm ("" : "+r" (register_classes));
       if (register_classes)
-	register_classes (__JCR_LIST__);
+	register_classes (jcr_list);
     }
 #endif