Message ID | CAFULd4a+pxpUGT8EPv0ZjRO2CROPGiRJ53LM1pFdvtc1h98nBw@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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.
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
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
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