On Mon, Mar 10, 2014 at 11:27 AM, Uros Bizjak <ubiz...@gmail.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. Attached patch is what I plan to commit to mainline soon. Ian, does it look OK to you? 2014-03-10 Uros Bizjak <ubiz...@gmail.com> * crtstuff.c (frame_dummy): Use void **jcr_list temporary variable to avoid array subscript is above array bounds warnings. Use __builtin_expect when checking *jcr_list for NULL. Re-bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32} and alpha-linux-gnu. Jakub, is it OK for mainline from RMs POV? Shall we backport this patch to other release branches? Uros.
Index: crtstuff.c =================================================================== --- crtstuff.c (revision 208448) +++ 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 (__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 +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 (__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