diff mbox

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

Message ID CAFULd4a+pxpUGT8EPv0ZjRO2CROPGiRJ53LM1pFdvtc1h98nBw@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak March 10, 2014, 10:10 a.m. UTC 
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.

Comments

Jakub Jelinek March 10, 2014, 10:14 a.m. UTC | #1 
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. UTC | #2 
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 Biener March 11, 2014, 10:58 a.m. UTC | #3 
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. UTC | #4 
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
diff mbox

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