Get rid of prefixes in General Dynamic TLS code for x86_64-nacl

Get rid of prefixes in GD TLS for x86_64-nacl

Standard GD TLS sequence contains commands prefixed with 0x6666, inserted by compiler with .byte directive. Our asm does not know how to move it around, and thus 0x6666 may cross bundle boundary and confuse the validator.

As we do not rewrite GD TLS access pattern at the moment, we can afford changing the standard sequence. Simply removing prefixes make still working code that does not suffer from the issues described above.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=1685
TEST=disassemble object file from any source accessing __thread variable - it should not have 0x6666 any more

Committed: http://git.chromium.org/gitweb/?p=nacl-gcc.git;a=commit;h=df9e2bf

[Brad Chen, 2011-04-19 15:21:46]

What is GD?

Brad

On Tue, Apr 19, 2011 at 7:53 AM, <eaeltsin@google.com> wrote:

> Reviewers: ,
>
> Description:
> Get rid of prefixes in GD TLS for x86_64-nacl
>
> Standard GD TLS sequence contains commands prefixed with 0x6666, inserted
> by
> compiler with .byte directive. Our asm does not know how to move it around,
> and
> thus 0x6666 may cross bundle boundary and confuse the validator.
>
> As we do not rewrite GD TLS access pattern at the moment, we can afford
> changing
> the standard sequence. Simply removing prefixes make still working code
> that
> does not suffer from the issues described above.
>
> BUG=http://code.google.com/p/nativeclient/issues/detail?id=1685
> TEST=disassemble object file from any source accessing __thread variable -
> it
> should not have 0x6666 any more
>
>
> Please review this at http://codereview.chromium.org/6878043/
>
> SVN Base: http://git.chromium.org/git/nacl-gcc@master
>
> Affected files:
>  M gcc/config/i386/i386.md
>
>
> Index: gcc/config/i386/i386.md
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index
>
59c02d088eda0544088f959cae833cd28ad15915..8dc1bdde0c8f1285b4ac072bceec79f67f0c0fe5
> 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -16450,7 +16450,11 @@
>    (unspec:DI [(match_operand:DI 1 "tls_symbolic_operand" "")]
>              UNSPEC_TLS_GD)]
>   "TARGET_64BIT"
> -  { return ".byte\t0x66\n\tlea{q}\t{%a1@TLSGD(%%rip), %%rdi|rdi,
> %a1@TLSGD[rip]}\n" ASM_SHORT "0x6666\n\trex64\n\tcall\t%P2"; }
> +{
> +  if (TARGET_NACL)
> +    return "\tlea{q}\t{%a1@TLSGD(%%rip), %%rdi|rdi, %a1@TLSGD
> [rip]}\n\tcall\t%P2";
> +  return ".byte\t0x66\n\tlea{q}\t{%a1@TLSGD(%%rip), %%rdi|rdi,
%a1@TLSGD[rip]}\n"
> ASM_SHORT "0x6666\n\trex64\n\tcall\t%P2";
> +}
>   [(set_attr "type" "multi")
>    (set_attr "length" "16")])
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to
> native-client-reviews@googlegroups.com.
> To unsubscribe from this group, send email to
> native-client-reviews+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/native-client-reviews?hl=en.
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[khim, 2011-04-19 15:26:23]

One nit, o/w LGTM

http://codereview.chromium.org/6878043/diff/1002/gcc/config/i386/i386.md
File gcc/config/i386/i386.md (right):

http://codereview.chromium.org/6878043/diff/1002/gcc/config/i386/i386.md#newc...
gcc/config/i386/i386.md:16456: return ".byte\t0x66\n\tlea{q}\t{%a1@TLSGD(%%rip),
%%rdi|rdi, %a1@TLSGD[rip]}\n" ASM_SHORT "0x6666\n\trex64\n\tcall\t%P2";
Why not if )...) return ...; else return ... ?

There are significantly more cases where this file uses patters with 

if (check)
  return some_value;
else
  return another_value;

rather then

if (check)
  return some_value;
return another_value;

Like this:

      if (REG_P (operands[2]))
        return "sal{q}\t{%b2, %0|%0, %b2}";
      else if (operands[2] == const1_rtx
               && (TARGET_SHIFT1 || optimize_function_for_size_p (cfun)))
        return "sal{q}\t%0";
      else
        return "sal{q}\t{%2, %0|%0, %2}";

Returns are aligned and this makes the code easier to read and understand...

[eaeltsin, 2011-04-19 15:27:39]

Sorry for using ambiguous acronym. Here GD stands for General Dynamic TLS model.

Renamed the issue.

[Roland McGrath, 2011-04-19 16:27:21]

I do have reservations about too many tweaks to the code generation, especially
when they are magic stock sequences like these (specified in ABI documents and
specially recognized by the linker)--even when we know we are changing that part
of the linker too.  And, these prefixes are clearly harmless on non-memory
instructions like lea and call, so validation accepting them seems fine to me.

But, given that we will eventually revisit all the code generation changes in
later cleanups of the toolchain work, LGTM.

[eaeltsin, 2011-04-19 17:37:22]

I think I'll rollback this as soon as we fix the assembler so it does not insert
nops between 0x6666 and the rest of the instruction and prevents all that stuff
from crossing the bundle boundary.

Fixing asm is somewhat harder task, so I'd like to have this in for now. Will
commit soon.

On 2011/04/19 16:27:21, Roland McGrath wrote:
> I do have reservations about too many tweaks to the code generation,
especially
> when they are magic stock sequences like these (specified in ABI documents and
> specially recognized by the linker)--even when we know we are changing that
part
> of the linker too.  And, these prefixes are clearly harmless on non-memory
> instructions like lea and call, so validation accepting them seems fine to me.
> 
> But, given that we will eventually revisit all the code generation changes in
> later cleanups of the toolchain work, LGTM.

[Use chromium.org instead, 2011-04-19 18:47:36]

Agreed.  I have thoughts about cleaning up the assembler picture, but I
don't think it is something to get into very soon.

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.