diff mbox

S390: Fix relocation of _nl_current_LC_CATETORY_used in static build. [BZ #19860]

Message ID nktdsm$oqu$1@ger.gmane.org
State Committed
Headers show

Commit Message

Stefan Liebler June 28, 2016, 8:52 a.m. UTC
Hi,

with shared libc, all locale categories are always loaded.
For static libc they aren't, but there exist a weak
_nl_current_LC_CATEGORY_used symbol for each category.
If the category is used, the locale/lc-CATEGORY.o is linked in
where _NL_CURRENT_DEFINE (LC_CATEGORY) defines and sets the
_nl_current_LC_CATEGORY_used symbol to one.

As reported by Marcin
"Bug 18960 - s390: _nl_locale_subfreeres uses larl opcode on misaligned
symbol" (https://sourceware.org/bugzilla/show_bug.cgi?id=18960)
In function _nl_locale_subfreeres (locale/setlocale.c) for each category
a check - &_nl_current_LC_CATEGORY_used != 0 - decides whether the
category is used or not.
There is also a second usage with the same mechanism in function 
__uselocale (locale/uselocale.c).

On s390 a larl instruction with R_390_PC32DBL relocation is used to
get the address of _nl_current_LC_CATEGORY_used symbols.
As larl loads the address relative in halfwords and the code is always
2-byte aligned, larl can only load even addresses.
At the end, the relocated address is always zero and never one.

Marcins patch (see bugzilla) uses the following declaration in
locale/setlocale.c:
extern char _nl_current_##category##_used __attribute__((__aligned__(1)));
In function _nl_locale_subfreeres all categories are checked
and therefore gcc is now building an array of addresses in
rodata section with an R_390_64 relocation for every address.
This array is loaded with larl instruction and each address
is accessed by index.

This fixes only the usage in _nl_locale_subfreeres. Each user has
to add the alignment attribute. Thus I propose to set the 
_nl_current_LC_CATEGORY_used symbols to two instead of one.
This way gcc can use larl instruction and the check against zero
works on every usage.

Okay to commit?

ChangeLog:

	[BZ #19860]
	* locale/localeinfo.h (_NL_CURRENT_DEFINE):
	Set _nl_current_LC_CATEGORY_used to two instead of one.

Comments

Andreas Schwab June 28, 2016, 10:13 a.m. UTC | #1
Stefan Liebler <stli@linux.vnet.ibm.com> writes:

> diff --git a/locale/localeinfo.h b/locale/localeinfo.h
> index 94627f3..d3e575f 100644
> --- a/locale/localeinfo.h
> +++ b/locale/localeinfo.h
> @@ -260,12 +260,15 @@ extern __thread struct __locale_data *const *_nl_current_##category \
>  #define _NL_CURRENT_WORD(category, item) \
>    ((uint32_t) (*_nl_current_##category)->values[_NL_ITEM_INDEX (item)].word)
>  
> -/* This is used in lc-CATEGORY.c to define _nl_current_CATEGORY.  */
> +/* This is used in lc-CATEGORY.c to define _nl_current_CATEGORY. The symbol
> +   _nl_current_CATEGORY_used is set to a value unequal to zero to mark this
> +   category as used. On S390 the used relocation to load the symbol address
> +   can only handle even addresses.  */

Ok, but two spaces after period.

Andreas.
Stefan Liebler June 28, 2016, 10:34 a.m. UTC | #2
On 06/28/2016 12:13 PM, Andreas Schwab wrote:
> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>
>> diff --git a/locale/localeinfo.h b/locale/localeinfo.h
>> index 94627f3..d3e575f 100644
>> --- a/locale/localeinfo.h
>> +++ b/locale/localeinfo.h
>> @@ -260,12 +260,15 @@ extern __thread struct __locale_data *const *_nl_current_##category \
>>   #define _NL_CURRENT_WORD(category, item) \
>>     ((uint32_t) (*_nl_current_##category)->values[_NL_ITEM_INDEX (item)].word)
>>
>> -/* This is used in lc-CATEGORY.c to define _nl_current_CATEGORY.  */
>> +/* This is used in lc-CATEGORY.c to define _nl_current_CATEGORY. The symbol
>> +   _nl_current_CATEGORY_used is set to a value unequal to zero to mark this
>> +   category as used. On S390 the used relocation to load the symbol address
>> +   can only handle even addresses.  */
>
> Ok, but two spaces after period.
>
> Andreas.
>

Commited with the two spaces.

Thanks.
Stefan
diff mbox

Patch

commit f3eb7c631df0cfe600d35661419427a3b7c878aa
Author: Stefan Liebler <stli@linux.vnet.ibm.com>
Date:   Mon Jun 27 15:56:24 2016 +0200

    S390: Fix relocation of _nl_current_LC_CATETORY_used in static build. [BZ #19860]
    
    Hi,
    
    with shared libc, all locale categories are always loaded.
    For static libc they aren't, but there exist a weak
    _nl_current_LC_CATEGORY_used symbol for each category.
    If the category is used, the locale/lc-CATEGORY.o is linked in
    where _NL_CURRENT_DEFINE (LC_CATEGORY) defines and sets the
    _nl_current_LC_CATEGORY_used symbol to one.
    
    As reported by Marcin
    "Bug 18960 - s390: _nl_locale_subfreeres uses larl opcode on misaligned symbol"
    (https://sourceware.org/bugzilla/show_bug.cgi?id=18960)
    In function _nl_locale_subfreeres (locale/setlocale.c) for each category
    a check - &_nl_current_LC_CATEGORY_used != 0 - decides whether the category
    is used or not.
    There is also a second usage with the same mechanism in function __uselocale
    (locale/uselocale.c).
    
    On s390 a larl instruction with R_390_PC32DBL relocation is used to
    get the address of _nl_current_LC_CATEGORY_used symbols. As larl loads the
    address relative in halfwords and the code is always 2-byte aligned,
    larl can only load even addresses.
    At the end, the relocated address is always zero and never one.
    
    Marcins patch (see bugzilla) uses the following declaration in locale/setlocale.c:
    extern char _nl_current_##category##_used __attribute__((__aligned__(1)));
    In function _nl_locale_subfreeres all categories are checked and therefore gcc
    is now building an array of addresses in rodata section with an R_390_64
    relocation for every address. This array is loaded with larl instruction and
    each address is accessed by index.
    
    This fixes only the usage in _nl_locale_subfreeres. Each user has to add the
    alignment attribute. Thus I propose to set the _nl_current_LC_CATEGORY_used
    symbols to two instead of one. This way gcc can use larl instruction and the
    check against zero works on every usage.
    
    Okay to commit?
    
    ChangeLog:
    
    	[BZ #19860]
    	* locale/localeinfo.h (_NL_CURRENT_DEFINE):
    	Set _nl_current_LC_CATEGORY_used to two instead of one.

diff --git a/locale/localeinfo.h b/locale/localeinfo.h
index 94627f3..d3e575f 100644
--- a/locale/localeinfo.h
+++ b/locale/localeinfo.h
@@ -260,12 +260,15 @@  extern __thread struct __locale_data *const *_nl_current_##category \
 #define _NL_CURRENT_WORD(category, item) \
   ((uint32_t) (*_nl_current_##category)->values[_NL_ITEM_INDEX (item)].word)
 
-/* This is used in lc-CATEGORY.c to define _nl_current_CATEGORY.  */
+/* This is used in lc-CATEGORY.c to define _nl_current_CATEGORY. The symbol
+   _nl_current_CATEGORY_used is set to a value unequal to zero to mark this
+   category as used. On S390 the used relocation to load the symbol address
+   can only handle even addresses.  */
 #define _NL_CURRENT_DEFINE(category) \
   __thread struct __locale_data *const *_nl_current_##category \
     attribute_hidden = &_nl_global_locale.__locales[category]; \
   asm (".globl " __SYMBOL_PREFIX "_nl_current_" #category "_used\n" \
-       _NL_CURRENT_DEFINE_ABS (_nl_current_##category##_used, 1));
+       _NL_CURRENT_DEFINE_ABS (_nl_current_##category##_used, 2));
 #ifdef HAVE_ASM_SET_DIRECTIVE
 # define _NL_CURRENT_DEFINE_ABS(sym, val) ".set " #sym ", " #val
 #else