Re: [PATCH] Fix ARM TLS handling (PR target/58595)

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi,

It seems the new test fails at execution on aarch64-none-elf (and
aarch64_be-none-elf) using the ARM Foundation Model as simulator.

Sorry, I don't have access to more detail right now.

Christophe.


On 6 March 2014 06:44, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
> On Wed, Mar 5, 2014 at 9:12 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hi!
>>
>> arm_legitimize_address may be called with a TLS symbol referenced, even when
>> x is not itself a SYMBOL_REF.  Most often it is something like:
>> (const:SI (plus:SI (symbol_ref:SI "tlsvar") (const_int NNN)))
>> but generally it can be something else (e.g. from expansion of
>> TARGET_MEM_REF).  Unfortunately this function legitimizes only the
>> SYMBOL_REF TLS case as TLS load, but the more complex forms with e.g. -fpic
>> can e.g. fall thru into legitimize_pic_address -> gen_calculate_pic_address,
>> which means either wrong-code or ICE later on.
>> Fixed by legitimizing both the SYMBOL_REF and
>> CONST+PLUS+SYMBOL_REF+CONST_INT case, and for more complex TLS containing
>> expressions just returning x (in that case the caller will split the PLUS
>> on its own).
>>
>> Kyrill has kindly bootstrapped/regtested this on Chromebook, ok for
>> trunk/4.8?
>>
>> 2014-03-05  Jakub Jelinek  <jakub@redhat.com>
>>             Meador Inge  <meadori@codesourcery.com>
>>
>>         PR target/58595
>>         * config/arm/arm.c (arm_tls_symbol_p): Remove.
>>         (arm_legitimize_address): Call legitimize_tls_address for any
>>         arm_tls_referenced_p expression, handle constant addend.  Call it
>>         before testing for !TARGET_ARM.
>>         (thumb_legitimize_address): Don't handle arm_tls_symbol_p here.
>>
>>         * gcc.dg/tls/pr58595.c: New test.
>
>
> OK if no regressions. Please let any auto testers catch any issues and
> back port to 4.8 in about 48 hours.
>
> Regards,
> Ramana
>>
>> --- gcc/config/arm/arm.c.jj     2014-03-04 08:51:39.620081210 +0100
>> +++ gcc/config/arm/arm.c        2014-03-04 14:06:26.776277688 +0100
>> @@ -235,7 +235,6 @@ static tree arm_gimplify_va_arg_expr (tr
>>  static void arm_option_override (void);
>>  static unsigned HOST_WIDE_INT arm_shift_truncation_mask (enum machine_mode);
>>  static bool arm_cannot_copy_insn_p (rtx);
>> -static bool arm_tls_symbol_p (rtx x);
>>  static int arm_issue_rate (void);
>>  static void arm_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
>>  static bool arm_output_addr_const_extra (FILE *, rtx);
>> @@ -7336,6 +7335,32 @@ legitimize_tls_address (rtx x, rtx reg)
>>  rtx
>>  arm_legitimize_address (rtx x, rtx orig_x, enum machine_mode mode)
>>  {
>> +  if (arm_tls_referenced_p (x))
>> +    {
>> +      rtx addend = NULL;
>> +
>> +      if (GET_CODE (x) == CONST && GET_CODE (XEXP (x, 0)) == PLUS)
>> +       {
>> +         addend = XEXP (XEXP (x, 0), 1);
>> +         x = XEXP (XEXP (x, 0), 0);
>> +       }
>> +
>> +      if (GET_CODE (x) != SYMBOL_REF)
>> +       return x;
>> +
>> +      gcc_assert (SYMBOL_REF_TLS_MODEL (x) != 0);
>> +
>> +      x = legitimize_tls_address (x, NULL_RTX);
>> +
>> +      if (addend)
>> +       {
>> +         x = gen_rtx_PLUS (SImode, x, addend);
>> +         orig_x = x;
>> +       }
>> +      else
>> +       return x;
>> +    }
>> +
>>    if (!TARGET_ARM)
>>      {
>>        /* TODO: legitimize_address for Thumb2.  */
>> @@ -7344,9 +7369,6 @@ arm_legitimize_address (rtx x, rtx orig_
>>        return thumb_legitimize_address (x, orig_x, mode);
>>      }
>>
>> -  if (arm_tls_symbol_p (x))
>> -    return legitimize_tls_address (x, NULL_RTX);
>> -
>>    if (GET_CODE (x) == PLUS)
>>      {
>>        rtx xop0 = XEXP (x, 0);
>> @@ -7459,9 +7481,6 @@ arm_legitimize_address (rtx x, rtx orig_
>>  rtx
>>  thumb_legitimize_address (rtx x, rtx orig_x, enum machine_mode mode)
>>  {
>> -  if (arm_tls_symbol_p (x))
>> -    return legitimize_tls_address (x, NULL_RTX);
>> -
>>    if (GET_CODE (x) == PLUS
>>        && CONST_INT_P (XEXP (x, 1))
>>        && (INTVAL (XEXP (x, 1)) >= 32 * GET_MODE_SIZE (mode)
>> @@ -7756,20 +7775,6 @@ thumb_legitimize_reload_address (rtx *x_
>>
>>  /* Test for various thread-local symbols.  */
>>
>> -/* Return TRUE if X is a thread-local symbol.  */
>> -
>> -static bool
>> -arm_tls_symbol_p (rtx x)
>> -{
>> -  if (! TARGET_HAVE_TLS)
>> -    return false;
>> -
>> -  if (GET_CODE (x) != SYMBOL_REF)
>> -    return false;
>> -
>> -  return SYMBOL_REF_TLS_MODEL (x) != 0;
>> -}
>> -
>>  /* Helper for arm_tls_referenced_p.  */
>>
>>  static int
>> --- gcc/testsuite/gcc.dg/tls/pr58595.c.jj       2014-03-04 12:56:48.337915114 +0100
>> +++ gcc/testsuite/gcc.dg/tls/pr58595.c  2014-03-04 12:56:48.337915114 +0100
>> @@ -0,0 +1,28 @@
>> +/* PR target/58595 */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +/* { dg-additional-options "-fpic" { target fpic } } */
>> +/* { dg-require-effective-target tls } */
>> +/* { dg-require-effective-target sync_int_long } */
>> +
>> +struct S { unsigned long a, b; };
>> +__thread struct S s;
>> +void bar (unsigned long *);
>> +
>> +__attribute__((noinline)) void
>> +foo (void)
>> +{
>> +  int i;
>> +  for (i = 0; i < 10; i++)
>> +    __sync_fetch_and_add (&s.b, 1L);
>> +}
>> +
>> +int
>> +main ()
>> +{
>> +  s.b = 12;
>> +  foo ();
>> +  if (s.b != 22)
>> +    __builtin_abort ();
>> +  return 0;
>> +}
>>
>>         Jakub