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

login
register
mail settings
Submitter Uros Bizjak
Date March 9, 2014, 9:38 p.m.
Message ID <CAFULd4apzqyXy+t8uGt=rwR+rQGYxrVDY+-UVWc+wa-3iw3fcw@mail.gmail.com>
Download mbox | patch
Permalink /patch/328397/
State New
Headers show

Comments

Uros Bizjak - March 9, 2014, 9:38 p.m.
On Sun, Mar 9, 2014 at 6:31 PM, Jakub Jelinek <jakub@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  <ubizjak@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:

--cut here--
void register_classes (void *);

static void *__JCR_LIST__[]  __attribute__ ((used)) = { };

void frame_dummy (void)
{
  void **jcr_list;
  __asm ("" : "=g" (jcr_list) : "0" (__JCR_LIST__));
  if (*jcr_list)
    register_classes (jcr_list);
}

void _frame_dummy (void)
{
  if (__JCR_LIST__[0])
    register_classes (__JCR_LIST__);
}
--cut here--

gcc -O2 -Wall:

frame_dummy:
        movl    $__JCR_LIST__, %edi
        cmpq    $0, (%rdi)
        je      .L1
        jmp     register_classes
        .p2align 4,,10
        .p2align 3
.L1:
        rep ret

_frame_dummy:
        cmpq    $0, __JCR_LIST__(%rip)
        je      .L4
        movl    $__JCR_LIST__, %edi
        jmp     register_classes
        .p2align 4,,10
        .p2align 3
.L4:
        rep ret

The new code preloads __JCR_LIST__ into a temporary register that is
reused in the call to register_classes.

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

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

Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
{,-m32} with all default languages + go.

OK for mainline?

Uros.
Jakub Jelinek - March 10, 2014, 7:49 a.m.
On Sun, Mar 09, 2014 at 10:38:25PM +0100, Uros Bizjak wrote:
> On Sun, Mar 9, 2014 at 6:31 PM, Jakub Jelinek <jakub@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  <ubizjak@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.

	Jakub

Patch

Index: crtstuff.c
===================================================================
--- crtstuff.c	(revision 208441)
+++ 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 (*jcr_list)
     {
       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 (*jcr_list)
     {
       void (*register_classes) (void *) = _Jv_RegisterClasses;
       __asm ("" : "+r" (register_classes));
       if (register_classes)
-	register_classes (__JCR_LIST__);
+	register_classes (jcr_list);
     }
 #endif