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

[Uros Bizjak]

On Mon, Mar 10, 2014 at 8:49 AM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Sun, Mar 09, 2014 at 10:38:25PM +0100, Uros Bizjak wrote:
>> On Sun, Mar 9, 2014 at 6:31 PM, Jakub Jelinek <ja...@redhat.com> wrote:
>> > On Sun, Mar 09, 2014 at 09:41:59AM -0700, Ian Lance Taylor wrote:
>> >> >>> Attached patch avoids a bunch of:
>> >> >>>
>> >> >>> ../../../gcc-svn/trunk/libgcc/crtstuff.c: In function 'frame_dummy':
>> >> >>> ../../../gcc-svn/trunk/libgcc/crtstuff.c:463:19: warning: array
>> >> >>> subscript is above array bounds [-Warray-bounds]
>> >> >>>    if (__JCR_LIST__[0])
>> >> >>>                    ^
>> >> >>>
>> >> >>> when compiling libgcc.
>> >> >>>
>> >> >>> 2014-03-08  Uros Bizjak  <ubiz...@gmail.com>
>> >> >>>
>> >> >>>     * crtstuff.c (__JCR_LIST__): Declare as zero-length array.
>> >
>> > I guess the only thing to avoid the warning (and potential miscompilation)
>> > is to hide the access from the optimizers through something like:
>> >   void *jcr_list;
>> >   __asm ("" : "=g" (jcr_list) : "0" (__JCR_LIST__));
>> > and then use jcr_list instead of __JCR_LIST__.
>>
>> Attached patch builds on your idea, but jcr_list temporary has to be
>> declared as void ** to allow proper dereference of pointer to void
>> array.
>>
>> The resulting code is also a bit better, as shown by following test:
>
> 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:

--cut here--
Index: crtstuff.c
===================================================================
--- crtstuff.c  (revision 208448)
+++ crtstuff.c  (working copy)
@@ -460,12 +460,13 @@
 #endif /* USE_EH_FRAME_REGISTRY */

 #ifdef JCR_SECTION_NAME
-  if (__JCR_LIST__[0])
+  void **jcr_list = __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 +566,13 @@
 #endif

 #ifdef JCR_SECTION_NAME
-  if (__JCR_LIST__[0])
+  void **jcr_list = __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

--cut here--

Uros.