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

login
register
mail settings
Submitter Uros Bizjak
Date March 10, 2014, 10:10 a.m.
Message ID <CAFULd4a+pxpUGT8EPv0ZjRO2CROPGiRJ53LM1pFdvtc1h98nBw@mail.gmail.com>
Download mbox | patch
Permalink /patch/328527/
State New
Headers show

Comments

Uros Bizjak - March 10, 2014, 10:10 a.m.
On Mon, Mar 10, 2014 at 8:49 AM, Jakub Jelinek <jakub@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 <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.

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--
--cut here--

Uros.
Jakub Jelinek - March 10, 2014, 10:14 a.m.
On Mon, Mar 10, 2014 at 11:10:05AM +0100, Uros Bizjak 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.

	Jakub
Uros Bizjak - March 10, 2014, 10:27 a.m.
On Mon, Mar 10, 2014 at 11:14 AM, Jakub Jelinek <jakub@redhat.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.

Uros.
Richard Guenther - March 11, 2014, 10:58 a.m.
On Mon, Mar 10, 2014 at 11:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Mar 10, 2014 at 11:10:05AM +0100, Uros Bizjak 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.

You can also declare it extern and define the symbol in a global asm ...

Richard.

>         Jakub
Jakub Jelinek - March 11, 2014, 11:02 a.m.
On Tue, Mar 11, 2014 at 11:58:01AM +0100, Richard Biener wrote:
> On Mon, Mar 10, 2014 at 11:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Mar 10, 2014 at 11:10:05AM +0100, Uros Bizjak 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.
> 
> You can also declare it extern and define the symbol in a global asm ...

But then you need to know how (which is quite target dependent).
Or define it in some other CU and ld -r it together, but supposedly not all
linkers can do ld -r.

	Jakub

Patch

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